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

[Talin]

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>