[LLVMdev] Proposed: first class packed structures

[Chris Lattner]

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