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