[LLVMdev] [patch] Union Types - work in progress

[Talin]

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>