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
I've made all the suggested changes - however, I'm having a bit of problem running the tests. I started "make check" and several hours later it had only made it through about 1/3 of the tests. I'm not sure what the deal is. 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 -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20100127/0d62099a/attachment.html>
OK I figured out the problems with the tests (was a bug in my code.) On Wed, Jan 27, 2010 at 5:18 PM, Talin <viridia at gmail.com> wrote:> I've made all the suggested changes - however, I'm having a bit of problem > running the tests. I started "make check" and several hours later it had > only made it through about 1/3 of the tests. I'm not sure what the deal is. > > > 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/20100128/10d3a2b8/attachment.html>
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 -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20100128/3c542d2e/attachment.html> -------------- next part -------------- A non-text attachment was scrubbed... Name: union.patch Type: application/octet-stream Size: 49213 bytes Desc: not available URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20100128/3c542d2e/attachment.obj>
One area of confusion - are vector types considered aggregate or not? 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/20100128/3fdad2fb/attachment.html>
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>