Currently, Structure layout is left to targets, which implement them
according to the ABI of that platform. While this is fine for most
structures, it makes packed structures very ugly. All fields in a
packed type must be converted to byte arrays with casts to access
fields, which bloats accesses and obsfucates the types. First class
support for packed types would clean up the generated code from these
(lowering bytecode sizes) as well as having minimal impact on
optimizations.
Consider:
extern struct { char a; int x; int y; int z; } __attribute((packed)) foos;;
int foo() { return foos.x + foos.y + foos.z; }
struct { int x; char a;} __attribute((packed)) bara[2];
int bar() { return bara[0].x + bara[1].x;}
which compiles to:
%struct.anon = type { sbyte, [4 x ubyte], [4 x ubyte], [4 x ubyte] }
%foos = external global %struct.anon
%bara = weak global [2 x { [4 x ubyte], sbyte }] zeroinitializer
implementation ; Functions:
int %foo() {
entry:
%tmp = load int* bitcast ([4 x ubyte]* getelementptr (%struct.anon*
%foos, int 0, uint 1) to int*)
%tmp3 = load int* bitcast ([4 x ubyte]* getelementptr (%struct.anon*
%foos, int 0, uint 2) to int*)
%tmp6 = load int* bitcast ([4 x ubyte]* getelementptr (%struct.anon*
%foos, int 0, uint 3) to int*)
%tmp4 = add int %tmp3, %tmp
%tmp7 = add int %tmp4, %tmp6
ret int %tmp7
}
int %bar() {
entry:
%tmp = load int* bitcast ([2 x { [4 x ubyte], sbyte }]* %bara to int*)
%tmp4 = load int* bitcast ([4 x ubyte]* getelementptr ([2 x { [4 x
ubyte], sbyte }]* %bara, int 0, int 1, uint 0) to int*)
%tmp5 = add int %tmp4, %tmp
ret int %tmp5
}
Packed structure types would let us say:
%struct.anon = type <{ sbyte, int, int, int }>
%foos = external global %struct.anon
%bara = weak global [2 x <{ int, sbyte }>] zeroinitializer
implementation ; Functions:
int %foo() {
entry:
%tmp = load int* getelementptr (%struct.anon* %foos, int 0, uint 1)
%tmp3 = load int* getelementptr (%struct.anon* %foos, int 0, uint 2)
%tmp6 = load int* getelementptr (%struct.anon* %foos, int 0, uint 3)
%tmp4 = add int %tmp3, %tmp
%tmp7 = add int %tmp4, %tmp6
ret int %tmp7
}
int %bar() {
entry:
%tmp = load int* getelementptr([2 x <{ int, sbyte }>]* %bara, int 0,
int 0, uint 0 )
%tmp4 = load int* getelementptr ([2 x <{ int, sbyte }>]* %bara, int
0, int 1, uint 0)
%tmp5 = add int %tmp4, %tmp
ret int %tmp5
}
The attached patch implements this (with the above syntax). Let me
know what you all think.
The bytecodes reduce for the above example from 342B -> 325B. This
also would fix bug 931.
Andrew
-------------- next part --------------
A non-text attachment was scrubbed...
Name: packed.patch.raw
Type: application/octet-stream
Size: 19418 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20061206/5fb8ac2d/attachment.obj>
Attached is a cleaned up patch. Andrew -------------- next part -------------- A non-text attachment was scrubbed... Name: packed.patch Type: text/x-patch Size: 12700 bytes Desc: not available URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20061206/26110f77/attachment.bin>
The attached patch (gcc.patch) implements the gcc portion of packed types. I have only tested this on a few examples, so I may be missing some type conversions somewhere. The gcc patch requires llvm_extra.patch, not included in the previous emails. Andrew -------------- next part -------------- A non-text attachment was scrubbed... Name: llvm_extra.patch Type: text/x-patch Size: 1677 bytes Desc: not available URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20061206/1c88dea4/attachment.bin> -------------- next part -------------- A non-text attachment was scrubbed... Name: gcc.patch Type: text/x-patch Size: 2714 bytes Desc: not available URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20061206/1c88dea4/attachment-0001.bin>
On Dec 6, 2006, at 10:44 AM, Andrew Lenharth wrote:> Currently, Structure layout is left to targets, which implement them > according to the ABI of that platform. While this is fine for most > structures, it makes packed structures very ugly. All fields in a > packed type must be converted to byte arrays with casts to access > fields, which bloats accesses and obsfucates the types. First class > support for packed types would clean up the generated code from these > (lowering bytecode sizes) as well as having minimal impact on > optimizations....> The attached patch implements this (with the above syntax). Let me > know what you all think. > The bytecodes reduce for the above example from 342B -> 325B. This > also would fix bug 931.I like this approach a *lot*. The syntax is even pretty clever :). I attached some nit-picky comments below about your revised patch: please address the issues then commit the patch. However, there is a issue that this patch brings up: pointers to packed fields are not necessarily aligned correctly. On targets with transparent unaligned load/store support (x86) this isn't an issue, but other targets (alpha, ppc, arm, ia64, etc) will be seriously impacted by this. The code generator has the ability to represent unaligned load/stores (in LoadSDNode and StoreSDNode) but this capability is currently unused. We need to extend load/store instructions to have an alignment field that can be set on them, and this alignment should be propagated into the code generator. This is http://llvm.org/PR400. It would be great if you were interested in tackling this too, but your patch isn't a regression, so it can go in without it. Comments: *** You need to update LangRef.html. @@ -154,24 +154,28 @@ public: class StructType : public CompositeType { friend class TypeMap<StructValType, StructType>; StructType(const StructType &); // Do not implement const StructType &operator=(const StructType &); // Do not implement + // is this a packed structure? + bool packed; + Instead of adding 4 bytes to each struct instance, please make use of empty bits in Type (immediately after the Abstract field). The best way to do this is to emulate 'Value's 'SubclassData' field, which is a protected member that subclasses can do whatever they want with. Just expose the free bits in Type as 'SubclassData' that StructType stores this bit in. @@ -196,10 +200,12 @@ public: // Methods for support type inquiry through isa, cast, and dyn_cast: static inline bool classof(const StructType *T) { return true; } static inline bool classof(const Type *T) { return T->getTypeID() == StructTyID; } + + virtual bool isPacked() const { return packed; } This doesn't need to be virtual. --- lib/VMCore/AsmWriter.cpp 6 Dec 2006 06:40:49 -0000 1.226 +++ lib/VMCore/AsmWriter.cpp 6 Dec 2006 18:03:06 -0000 @@ -286,18 +286,22 @@ static void calcTypeName(const Type *Ty, Result += ")"; break; } case Type::StructTyID: { const StructType *STy = cast<StructType>(Ty); + if (STy->isPacked()) + Result += "<"; Result += "{ "; for (StructType::element_iterator I = STy->element_begin(), E = STy->element_end(); I != E; ++I) { if (I != STy->element_begin()) Result += ", "; calcTypeName(*I, TypeStack, TypeNames, Result); } Result += " }"; + if (STy->isPacked()) + Result += ">"; break; Please use '>' instead of ">", likewise '<'. Index: lib/VMCore/Type.cpp ==================================================================RCS file: /home/vadve/shared/PublicCVS/llvm/lib/VMCore/Type.cpp,v retrieving revision 1.151 diff -t -d -u -p -5 -r1.151 Type.cpp --- lib/VMCore/Type.cpp 27 Nov 2006 01:05:10 -0000 1.151 +++ lib/VMCore/Type.cpp 6 Dec 2006 18:07:50 -0000 @@ -1158,24 +1167,26 @@ public: static ManagedStatic<TypeMap<StructValType, StructType> > StructTypes; -StructType *StructType::get(const std::vector<const Type*> &ETypes) { - StructValType STV(ETypes); +StructType *StructType::get(const std::vector<const Type*> &ETypes, bool isPacked) { + StructValType STV(ETypes, isPacked); StructType *ST = StructTypes->get(STV); if (ST) return ST; Please keep the code within 80 columns. ==================================================================RCS file: /home/vadve/shared/PublicCVS/llvm/lib/AsmParser/ llvmAsmParser.y,v retrieving revision 1.287 diff -t -d -u -p -5 -r1.287 llvmAsmParser.y --- lib/AsmParser/llvmAsmParser.y 5 Dec 2006 23:46:41 -0000 1.287 +++ lib/AsmParser/llvmAsmParser.y 6 Dec 2006 20:02:18 -0000 @@ -1207,10 +1207,20 @@ UpRTypes : '\\' EUINT64VAL { It looks like you need to handle <{}> in the same places that handle {}, for empty packed structs. ==================================================================RCS file: /home/vadve/shared/PublicCVS/llvm/lib/Bytecode/Writer/ Writer.cpp,v retrieving revision 1.134 diff -t -d -u -p -5 -r1.134 Writer.cpp --- lib/Bytecode/Writer/Writer.cpp 6 Dec 2006 04:27:07 -0000 1.134 +++ lib/Bytecode/Writer/Writer.cpp 6 Dec 2006 17:25:08 -0000 @@ -247,11 +247,11 @@ void BytecodeWriter::outputType(const Ty Reid can chime in here, but it seems like there should be a more efficient way to encode the packed'ness than using a whole byte per struct type. Perhaps it doesn't matter, because we will hopefully be moving to per-bit encoding in the near future. -Chris
On Thu, 2006-12-07 at 22:00 -0800, Chris Lattner wrote:> On Dec 6, 2006, at 10:44 AM, Andrew Lenharth wrote: > > > Currently, Structure layout is left to targets, which implement them > > according to the ABI of that platform. While this is fine for most > > structures, it makes packed structures very ugly. All fields in a > > packed type must be converted to byte arrays with casts to access > > fields, which bloats accesses and obsfucates the types. First class > > support for packed types would clean up the generated code from these > > (lowering bytecode sizes) as well as having minimal impact on > > optimizations.Cool!> Reid can chime in here, but it seems like there should be a more > efficient way to encode the packed'ness than using a whole byte per > struct type. Perhaps it doesn't matter, because we will hopefully be > moving to per-bit encoding in the near future.I don't thinkits worth it at this point. We have way more serious things going on than an extra byte for a type. Consider that all ICmp/FCmp instructions will use format0 (the largest) in anything but the smallest of functions (less than 64 instructions). This will get naturally cleaned up when we move to bit streams. If you *really* want to be efficient, the *right* way to do this is to create a new TypeID and the corresponding class (class PackedStructureType : public StructureType). That way it costs nothing (the value of the TypeID indicates that its PackedStructure) and there's no need to mess with an additional byte in the StructureType class. But, that might be a little heavy weight :) Reid.