One area of confusion - are vector types considered aggregate or not?<br><br><div class="gmail_quote">On Thu, Jan 28, 2010 at 12:25 PM, Talin <span dir="ltr"><<a href="mailto:viridia@gmail.com">viridia@gmail.com</a>></span> wrote:<br>

<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">OK here's a new version of the patch - and the unions.ll test actually passes :)<br><br><div class="gmail_quote">
<div class="im">
On Mon, Jan 18, 2010 at 1:40 PM, Chris Lattner <span dir="ltr"><<a href="mailto:clattner@apple.com" target="_blank">clattner@apple.com</a>></span> wrote:<br>
</div><div><div></div><div class="h5"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div></div><div><br>
On Jan 16, 2010, at 11:15 AM, Talin wrote:<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
OK here's the patch for real this time :)<br>
<br>
On Fri, Jan 15, 2010 at 4:36 PM, Talin <<a href="mailto:viridia@gmail.com" target="_blank">viridia@gmail.com</a>> wrote:<br>
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.<br>



</blockquote>
<br></div></div>
Looking good so far, some thoughts:<br>
<br>
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.<br>



<br>
The ConstantUnion ctor or ConstantUnion::get should assert that the constant has type that matches one of the elements of the union.<br>
<br>
@@ -928,7 +949,7 @@<br>
 /// if the elements of the array are all ConstantInt's.<br>
 bool ConstantArray::isString() const {<br>
   // Check the element type for i8...<br>
-  if (!getType()->getElementType()->isInteger(8))<br>
+  if (getType()->getElementType() != Type::getInt8Ty(getContext()))<br>
     return false;<br>
   // Check the elements to make sure they are all integers, not constant<br>
   // expressions.<br>
<br>
You have a couple of these things which revert a recent patch, please don't :)<br>
<br>
<br>
Funky indentation in ConstantUnion::replaceUsesOfWithOnConstant and implementation missing :)<br>
<br>
In UnionValType methods, please use "UT" instead of "ST" as an acronym.<br>
<br>
+bool UnionType::isValidElementType(const Type *ElemTy) {<br>
+  return ElemTy->getTypeID() != VoidTyID && ElemTy->getTypeID() != LabelTyID &&<br>
+         ElemTy->getTypeID() != MetadataTyID && !isa<FunctionType>(ElemTy);<br>
+}<br>
<br>
Please use "!ElemTy->isVoidTy()" etc.<br>
<br>
--- lib/VMCore/ConstantsContext.h       (revision 93451)<br>
<br>
+template<><br>
+struct ConstantKeyData<ConstantUnion> {<br>
+  typedef Constant* ValType;<br>
+  static ValType getValType(ConstantUnion *CS) {<br>
<br>
CU not CS.<br>
<br>
<br>
LLParser.cpp:<br>
<br>
In LLParser::ParseUnionType, you can use SmallVector instead of std::vector for ParamsList & ParamsListTy.<br>
<br>
@@ -2135,7 +2173,8 @@<br>
         ParseToken(lltok::rparen, "expected ')' in extractvalue constantexpr"))<br>
       return true;<br>
<br>
-    if (!isa<StructType>(Val->getType()) && !isa<ArrayType>(Val->getType()))<br>
+    if (!isa<StructType>(Val->getType()) && !isa<ArrayType>(Val->getType()) &&<br>
+        !isa<UnionType>(Val->getType()))<br>
       return Error(ID.Loc, "extractvalue operand must be array or struct");<br>
     if (!ExtractValueInst::getIndexedType(Val->getType(), Indices.begin(),<br>
                                           Indices.end()))<br>
@@ -2156,7 +2195,8 @@<br>
         ParseIndexList(Indices) ||<br>
         ParseToken(lltok::rparen, "expected ')' in insertvalue constantexpr"))<br>
       return true;<br>
-    if (!isa<StructType>(Val0->getType()) && !isa<ArrayType>(Val0->getType()))<br>
+    if (!isa<StructType>(Val0->getType()) && !isa<ArrayType>(Val0->getType()) &&<br>
+        !isa<UnionType>(Val0->getType()))<br>
<br>
How about changing this to use Type::isAggregateType() instead of enumerating?  This happens a few times in LLParser.cpp<br>
<br>
<br>
+    if (ID.ConstantVal->getType() != Ty) {<br>
+      // Allow a constant struct with a single member to be converted<br>
+      // to a union, if the union has a member which is the same type<br>
+      // as the struct member.<br>
+      if (const UnionType* utype = dyn_cast<UnionType>(Ty)) {<br>
+        if (const StructType* stype = dyn_cast<StructType>(<br>
+            ID.ConstantVal->getType())) {<br>
+          if (stype->getNumContainedTypes() == 1) {<br>
+            int index = utype->getElementTypeIndex(stype->getContainedType(0));<br>
+            if (index >= 0) {<br>
+              V = ConstantUnion::get(<br>
+                  utype, cast<Constant>(ID.ConstantVal->getOperand(0)));<br>
+              return false;<br>
+            }<br>
+          }<br>
+        }<br>
+      }<br>
+<br>
<br>
Please split this out to a static helper function that uses early exits.  In this code you should be able to do something like:<br>
<br>
    if (ID.ConstantVal->getType() != Ty)<br>
      if (Constant *Elt = TryConvertingSingleElementStructToUnion(...))<br>
        return Elt;<br>
<br>
<br>
<br>
+++ lib/Bitcode/Reader/BitcodeReader.cpp        (working copy)<br>
@@ -584,6 +584,13 @@<br>
       ResultTy = StructType::get(Context, EltTys, Record[0]);<br>
       break;<br>
     }<br>
+    case bitc::TYPE_CODE_UNION: {  // UNION: [eltty x N]<br>
+      std::vector<const Type*> EltTys;<br>
+      for (unsigned i = 0, e = Record.size(); i != e; ++i)<br>
+        EltTys.push_back(getTypeByID(Record[i], true));<br>
+      ResultTy = UnionType::get(&EltTys[0], EltTys.size());<br>
+      break;<br>
+    }<br>
<br>
This can use SmallVector.<br>
<br>
Otherwise, the patch is looking great to me!<br><font color="#888888">
<br>
-Chris<br>
<br>
<br>
</font></blockquote></div></div></div><br><br clear="all"><br>-- <br><font color="#888888">-- Talin<br>
</font></blockquote></div><br><br clear="all"><br>-- <br>-- Talin<br>