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.
OK here’s the patch for real this time 
union.patch (43 KB)
OK here's the patch for real this time
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;
+ }
+ }
+ }
+ }
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.
OK I figured out the problems with the tests (was a bug in my code.)
OK here’s a new version of the patch - and the unions.ll test actually passes
union.patch (48.1 KB)
One area of confusion - are vector types considered aggregate or not?
ping…
Hi Talin, sorry for the delay. FWIW, it’s usually best to trickle pieces of a feature in and build it up over time, otherwise your patch just gets larger and larger.
LangRef.html:
-
- Union constants
-
- Union constants are represented with notation similar to a structure with
- a single element - that is, a single typed element surrounded
- by braces ({})). For example: “{ i32 4 }”. A
- single-element constant struct can be implicitly converted to a
- union type as long as the type of the struct
- element matches the type of one of the union members.
I’ll work on this later today. One note: I’m not intentionally reverting anything, rather it’s that the code is changing out from under me (this patch is already over a month old, a lot of code has changed in that time.)
OK here’s a new patch. Additional comments below.
LangRef.html:
- Union constants
- Union constants are represented with notation similar to a structure with
- a single element - that is, a single typed element surrounded
- by braces ({})). For example: “{ i32 4 }”. A
- single-element constant struct can be implicitly converted to a
- union type as long as the type of the struct
- element matches the type of one of the union members.
It’s a minor point, but I’d avoid the term “implicitly converted”.
done
Constants.cpp:
- assert(Idx->getType()->isInteger(32) &&
- assert(Idx->getType() == Type::getInt32Ty(Val->getContext()) &&
You’re reverting these changes, please don’t. There are a couple instances of this.
Didn’t mean to Hope that the train hasn’t moved too far while I was editing this.
+void ConstantUnion::replaceUsesOfWithOnConstant(Value *From, Value *To,
- Use *U) {
- assert(false && “Implement replaceUsesOfWithOnConstant for unions”);
+}Still not implemented?
Not in this patch - as you say, it’s too large already.
+UnionType *UnionType::get(const Type *type, …) {
- va_list ap;
- std::vector<const llvm::Type*> UnionFields;
- va_start(ap, type);
Please use smallvector.
Done - although I was just copying from what Struct does.
+bool UnionType::isValidElementType(const Type *ElemTy) {
- return !ElemTy->isVoidTy() && !ElemTy->isLabelTy() &&
- !ElemTy->isMetadataTy() && !isa(ElemTy);
+}Isn’t there a better predicate somewhere?
Apparently there is now. Done.
+LLVMTypeRef LLVMUnionTypeInContext(LLVMContextRef C, LLVMTypeRef *ElementTypes,
- unsigned ElementCount) {
- std::vector<const Type*> Tys;
- for (LLVMTypeRef *I = ElementTypes,
indentation of unsigned and use smallvector.
Done.
+/// ParseUnionType
+/// TypeRec
+/// ::= ‘union’ ‘{’ ‘}’
+/// ::= ‘union’ ‘{’ TypeRec (‘,’ TypeRec)* ‘}’
+bool LLParser::ParseUnionType(PATypeHolder &Result) {Unions can’t be empty, so the first grammar production isn’t valid.
Done.
Otherwise, it looks good, please send these updates and I will commit it for you.
As much as it pains me to say anything that would delay this getting committed, it might make sense to apply this patch after the code freeze - since the feature isn’t going to be finished in time for 2.7.
union.patch (47.1 KB)
OK here’s a new patch. Additional comments below.
+void ConstantUnion::replaceUsesOfWithOnConstant(Value *From, Value *To,
- Use *U) {
- assert(false && “Implement replaceUsesOfWithOnConstant for unions”);
+}Still not implemented?
Not in this patch - as you say, it’s too large already.
Ok, please work on this as a follow-up patch.
Otherwise, it looks good, please send these updates and I will commit it for you.
As much as it pains me to say anything that would delay this getting committed, it might make sense to apply this patch after the code freeze - since the feature isn’t going to be finished in time for 2.7.
I committed the patch in r96011. However, please do follow up with the replaceUsesOfWithOnConstant thing above, and please add a note to the release notes mentioning the feature and saying that it may change in the future.
Also, please read the llvm developer policy doc and send me the requested info and I’ll set you up with commit access, thanks!
-Chris