ping... On Thu, Jan 28, 2010 at 12:25 PM, Talin <viridia at gmail.com> wrote:> OK here's a new version of the patch - and the unions.ll test actually > passes :) > > On Mon, Jan 18, 2010 at 1:40 PM, Chris Lattner <clattner at apple.com> wrote: > >> >> On Jan 16, 2010, at 11:15 AM, Talin wrote: >> >> OK here's the patch for real this time :) >>> >>> On Fri, Jan 15, 2010 at 4:36 PM, Talin <viridia at gmail.com> wrote: >>> Here's a work in progress of the union patch. Note that the test >>> "union.ll" does not work, so you probably don't want to check this in as is. >>> However, I'd be interested in any feedback you're willing to give. >>> >> >> Looking good so far, some thoughts: >> >> The LangRef.html patch looks great. One thing that I notice is that the >> term 'aggregate' is not defined anywhere. Please add it to the >> #t_classifications section and change the insert/extractvalue instructions >> to refer to that type classification instead of enumerating the options. >> >> The ConstantUnion ctor or ConstantUnion::get should assert that the >> constant has type that matches one of the elements of the union. >> >> @@ -928,7 +949,7 @@ >> /// if the elements of the array are all ConstantInt's. >> bool ConstantArray::isString() const { >> // Check the element type for i8... >> - if (!getType()->getElementType()->isInteger(8)) >> + if (getType()->getElementType() != Type::getInt8Ty(getContext())) >> return false; >> // Check the elements to make sure they are all integers, not constant >> // expressions. >> >> You have a couple of these things which revert a recent patch, please >> don't :) >> >> >> Funky indentation in ConstantUnion::replaceUsesOfWithOnConstant and >> implementation missing :) >> >> In UnionValType methods, please use "UT" instead of "ST" as an acronym. >> >> +bool UnionType::isValidElementType(const Type *ElemTy) { >> + return ElemTy->getTypeID() != VoidTyID && ElemTy->getTypeID() !>> LabelTyID && >> + ElemTy->getTypeID() != MetadataTyID && >> !isa<FunctionType>(ElemTy); >> +} >> >> Please use "!ElemTy->isVoidTy()" etc. >> >> --- lib/VMCore/ConstantsContext.h (revision 93451) >> >> +template<> >> +struct ConstantKeyData<ConstantUnion> { >> + typedef Constant* ValType; >> + static ValType getValType(ConstantUnion *CS) { >> >> CU not CS. >> >> >> LLParser.cpp: >> >> In LLParser::ParseUnionType, you can use SmallVector instead of >> std::vector for ParamsList & ParamsListTy. >> >> @@ -2135,7 +2173,8 @@ >> ParseToken(lltok::rparen, "expected ')' in extractvalue >> constantexpr")) >> return true; >> >> - if (!isa<StructType>(Val->getType()) && >> !isa<ArrayType>(Val->getType())) >> + if (!isa<StructType>(Val->getType()) && >> !isa<ArrayType>(Val->getType()) && >> + !isa<UnionType>(Val->getType())) >> return Error(ID.Loc, "extractvalue operand must be array or >> struct"); >> if (!ExtractValueInst::getIndexedType(Val->getType(), Indices.begin(), >> Indices.end())) >> @@ -2156,7 +2195,8 @@ >> ParseIndexList(Indices) || >> ParseToken(lltok::rparen, "expected ')' in insertvalue >> constantexpr")) >> return true; >> - if (!isa<StructType>(Val0->getType()) && >> !isa<ArrayType>(Val0->getType())) >> + if (!isa<StructType>(Val0->getType()) && >> !isa<ArrayType>(Val0->getType()) && >> + !isa<UnionType>(Val0->getType())) >> >> How about changing this to use Type::isAggregateType() instead of >> enumerating? This happens a few times in LLParser.cpp >> >> >> + if (ID.ConstantVal->getType() != Ty) { >> + // Allow a constant struct with a single member to be converted >> + // to a union, if the union has a member which is the same type >> + // as the struct member. >> + if (const UnionType* utype = dyn_cast<UnionType>(Ty)) { >> + if (const StructType* stype = dyn_cast<StructType>( >> + ID.ConstantVal->getType())) { >> + if (stype->getNumContainedTypes() == 1) { >> + int index >> utype->getElementTypeIndex(stype->getContainedType(0)); >> + if (index >= 0) { >> + V = ConstantUnion::get( >> + utype, cast<Constant>(ID.ConstantVal->getOperand(0))); >> + return false; >> + } >> + } >> + } >> + } >> + >> >> Please split this out to a static helper function that uses early exits. >> In this code you should be able to do something like: >> >> if (ID.ConstantVal->getType() != Ty) >> if (Constant *Elt = TryConvertingSingleElementStructToUnion(...)) >> return Elt; >> >> >> >> +++ lib/Bitcode/Reader/BitcodeReader.cpp (working copy) >> @@ -584,6 +584,13 @@ >> ResultTy = StructType::get(Context, EltTys, Record[0]); >> break; >> } >> + case bitc::TYPE_CODE_UNION: { // UNION: [eltty x N] >> + std::vector<const Type*> EltTys; >> + for (unsigned i = 0, e = Record.size(); i != e; ++i) >> + EltTys.push_back(getTypeByID(Record[i], true)); >> + ResultTy = UnionType::get(&EltTys[0], EltTys.size()); >> + break; >> + } >> >> This can use SmallVector. >> >> Otherwise, the patch is looking great to me! >> >> -Chris >> >> >> > > > -- > -- Talin >-- -- Talin -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20100209/9e7734c0/attachment.html>
On Feb 9, 2010, at 4:28 PM, Talin wrote:> ping...Hi Talin, sorry for the delay. FWIW, it's usually best to trickle pieces of a feature in and build it up over time, otherwise your patch just gets larger and larger. LangRef.html: + <dt><b>Union constants</b></dt> + <dd>Union constants are represented with notation similar to a structure with + a single element - that is, a single typed element surrounded + by braces (<tt>{}</tt>)). For example: "<tt>{ i32 4 }</tt>". A + single-element constant struct can be implicitly converted to a + <a href="#t_union">union type</a> as long as the type of the struct + element matches the type of one of the union members.</dd> + It's a minor point, but I'd avoid the term "implicitly converted". Constants.cpp: - assert(Idx->getType()->isInteger(32) && + assert(Idx->getType() == Type::getInt32Ty(Val->getContext()) && You're reverting these changes, please don't. There are a couple instances of this. +void ConstantUnion::replaceUsesOfWithOnConstant(Value *From, Value *To, + Use *U) { + assert(false && "Implement replaceUsesOfWithOnConstant for unions"); +} + Still not implemented? +UnionType *UnionType::get(const Type *type, ...) { + va_list ap; + std::vector<const llvm::Type*> UnionFields; + va_start(ap, type); Please use smallvector. +bool UnionType::isValidElementType(const Type *ElemTy) { + return !ElemTy->isVoidTy() && !ElemTy->isLabelTy() && + !ElemTy->isMetadataTy() && !isa<FunctionType>(ElemTy); +} Isn't there a better predicate somewhere? +LLVMTypeRef LLVMUnionTypeInContext(LLVMContextRef C, LLVMTypeRef *ElementTypes, + unsigned ElementCount) { + std::vector<const Type*> Tys; + for (LLVMTypeRef *I = ElementTypes, + indentation of unsigned and use smallvector. +/// ParseUnionType +/// TypeRec +/// ::= 'union' '{' '}' +/// ::= 'union' '{' TypeRec (',' TypeRec)* '}' +bool LLParser::ParseUnionType(PATypeHolder &Result) { Unions can't be empty, so the first grammar production isn't valid. Otherwise, it looks good, please send these updates and I will commit it for you. -Chris -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20100210/a94c1c32/attachment.html>
I'll work on this later today. One note: I'm not intentionally reverting anything, rather it's that the code is changing out from under me (this patch is already over a month old, a lot of code has changed in that time.) On Wed, Feb 10, 2010 at 10:57 AM, Chris Lattner <clattner at apple.com> wrote:> > On Feb 9, 2010, at 4:28 PM, Talin wrote: > > ping... > > > Hi Talin, sorry for the delay. FWIW, it's usually best to trickle pieces > of a feature in and build it up over time, otherwise your patch just gets > larger and larger. > > LangRef.html: > > + <dt><b>Union constants</b></dt> > + <dd>Union constants are represented with notation similar to a structure > with > + a single element - that is, a single typed element surrounded > + by braces (<tt>{}</tt>)). For example: "<tt>{ i32 4 }</tt>". A > + single-element constant struct can be implicitly converted to a > + <a href="#t_union">union type</a> as long as the type of the struct > + element matches the type of one of the union members.</dd> > + > > It's a minor point, but I'd avoid the term "implicitly converted". > > Constants.cpp: > > - assert(Idx->getType()->isInteger(32) && > + assert(Idx->getType() == Type::getInt32Ty(Val->getContext()) && > > You're reverting these changes, please don't. There are a couple instances > of this. > > +void ConstantUnion::replaceUsesOfWithOnConstant(Value *From, Value *To, > + Use *U) { > + assert(false && "Implement replaceUsesOfWithOnConstant for unions"); > +} > + > > Still not implemented? > > +UnionType *UnionType::get(const Type *type, ...) { > + va_list ap; > + std::vector<const llvm::Type*> UnionFields; > + va_start(ap, type); > > Please use smallvector. > > +bool UnionType::isValidElementType(const Type *ElemTy) { > + return !ElemTy->isVoidTy() && !ElemTy->isLabelTy() && > + !ElemTy->isMetadataTy() && !isa<FunctionType>(ElemTy); > +} > > Isn't there a better predicate somewhere? > > +LLVMTypeRef LLVMUnionTypeInContext(LLVMContextRef C, LLVMTypeRef > *ElementTypes, > + unsigned ElementCount) { > + std::vector<const Type*> Tys; > + for (LLVMTypeRef *I = ElementTypes, > + > > indentation of unsigned and use smallvector. > > > +/// ParseUnionType > +/// TypeRec > +/// ::= 'union' '{' '}' > +/// ::= 'union' '{' TypeRec (',' TypeRec)* '}' > +bool LLParser::ParseUnionType(PATypeHolder &Result) { > > Unions can't be empty, so the first grammar production isn't valid. > > Otherwise, it looks good, please send these updates and I will commit it > for you. > > -Chris > >-- -- Talin -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20100210/9b858ec0/attachment.html>
OK here's a new patch. Additional comments below. On Wed, Feb 10, 2010 at 10:57 AM, Chris Lattner <clattner at apple.com> wrote:> > LangRef.html: > > + <dt><b>Union constants</b></dt> > + <dd>Union constants are represented with notation similar to a structure > with > + a single element - that is, a single typed element surrounded > + by braces (<tt>{}</tt>)). For example: "<tt>{ i32 4 }</tt>". A > + single-element constant struct can be implicitly converted to a > + <a href="#t_union">union type</a> as long as the type of the struct > + element matches the type of one of the union members.</dd> > + > > It's a minor point, but I'd avoid the term "implicitly converted". >done> > Constants.cpp: > > - assert(Idx->getType()->isInteger(32) && > + assert(Idx->getType() == Type::getInt32Ty(Val->getContext()) && > > You're reverting these changes, please don't. There are a couple instances > of this. >Didn't mean to :) Hope that the train hasn't moved too far while I was editing this.> > +void ConstantUnion::replaceUsesOfWithOnConstant(Value *From, Value *To, > + Use *U) { > + assert(false && "Implement replaceUsesOfWithOnConstant for unions"); > +} > + > > Still not implemented? > > Not in this patch - as you say, it's too large already.> +UnionType *UnionType::get(const Type *type, ...) { > + va_list ap; > + std::vector<const llvm::Type*> UnionFields; > + va_start(ap, type); > > Please use smallvector. >Done - although I was just copying from what Struct does.> > +bool UnionType::isValidElementType(const Type *ElemTy) { > + return !ElemTy->isVoidTy() && !ElemTy->isLabelTy() && > + !ElemTy->isMetadataTy() && !isa<FunctionType>(ElemTy); > +} > > Isn't there a better predicate somewhere? >Apparently there is now. Done.> > +LLVMTypeRef LLVMUnionTypeInContext(LLVMContextRef C, LLVMTypeRef > *ElementTypes, > + unsigned ElementCount) { > + std::vector<const Type*> Tys; > + for (LLVMTypeRef *I = ElementTypes, > + > > indentation of unsigned and use smallvector. > > Done.> > +/// ParseUnionType > +/// TypeRec > +/// ::= 'union' '{' '}' > +/// ::= 'union' '{' TypeRec (',' TypeRec)* '}' > +bool LLParser::ParseUnionType(PATypeHolder &Result) { > > Unions can't be empty, so the first grammar production isn't valid. >Done.> > Otherwise, it looks good, please send these updates and I will commit it > for you. > > As much as it pains me to say anything that would delay this gettingcommitted, it might make sense to apply this patch after the code freeze - since the feature isn't going to be finished in time for 2.7. -- -- Talin -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20100211/44b3d0a8/attachment.html> -------------- next part -------------- A non-text attachment was scrubbed... Name: union.patch Type: application/octet-stream Size: 48234 bytes Desc: not available URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20100211/44b3d0a8/attachment.obj>