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

[Chris Lattner]

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