Rework packed-ness of IR aggregates

Base it on the IR alignments of the fields only.

Also fix the alignment of variables captured by ref (=> pointer
alignment, not the pointee's) and captured lazy params (=>
delegate alignment) in nested frames, something that cropped up with
a new assertion.

gen/nested.cpp

@@ -381,62 +381,66 @@ static void DtoCreateNestedContextType(FuncDeclaration *fd) {
   IF_LOG Logger::cout() << "Function " << fd->toChars() << " has depth "
                         << depth << '\n';
 
-  AggrTypeBuilder builder(false);
+  AggrTypeBuilder builder;
 
   if (depth != 0) {
     assert(innerFrameType);
-    unsigned ptrSize = gDataLayout->getPointerSize();
     // Add frame pointer types for all but last frame
     for (unsigned i = 0; i < (depth - 1); ++i) {
-      builder.addType(innerFrameType->getElementType(i), ptrSize);
+      builder.addType(innerFrameType->getElementType(i), target.ptrsize);
     }
     // Add frame pointer type for last frame
-    builder.addType(LLPointerType::getUnqual(innerFrameType), ptrSize);
+    builder.addType(LLPointerType::getUnqual(innerFrameType), target.ptrsize);
   }
 
   // Add the direct nested variables of this function, and update their
   // indices to match.
   // TODO: optimize ordering for minimal space usage?
+  unsigned maxAlignment = 1;
   for (auto vd : fd->closureVars) {
-    unsigned alignment = DtoAlignment(vd);
-    if (alignment > 1) {
-      builder.alignCurrentOffset(alignment);
-    }
-
     const bool isParam = vd->isParameter();
 
-    IrLocal &irLocal =
-        *(isParam ? getIrParameter(vd, true) : getIrLocal(vd, true));
-    irLocal.nestedIndex = builder.currentFieldIndex();
-    irLocal.nestedDepth = depth;
-
     LLType *t = nullptr;
+    unsigned alignment = 0;
     if (captureByRef(vd)) {
       t = DtoType(vd->type->pointerTo());
+      alignment = target.ptrsize;
     } else if (isParam && (vd->storage_class & STClazy)) {
       // the type is a delegate (LL struct)
       auto tf = TypeFunction::create(nullptr, vd->type, VARARGnone, LINK::d);
       auto td = TypeDelegate::create(tf);
       t = DtoType(td);
+      alignment = target.ptrsize;
     } else {
       t = DtoMemType(vd->type);
+      alignment = DtoAlignment(vd);
     }
 
+    if (alignment > 1) {
+      builder.alignCurrentOffset(alignment);
+      maxAlignment = std::max(maxAlignment, alignment);
+    }
+
+    IrLocal &irLocal =
+        *(isParam ? getIrParameter(vd, true) : getIrLocal(vd, true));
+    irLocal.nestedIndex = builder.currentFieldIndex();
+    irLocal.nestedDepth = depth;
+
     builder.addType(t, getTypeAllocSize(t));
 
     IF_LOG Logger::cout() << "Nested var '" << vd->toChars() << "' of type "
                           << *t << "\n";
   }
 
-  LLStructType *frameType =
+  LLStructType *frameType = LLStructType::create(
-      LLStructType::create(gIR->context(), builder.defaultTypes(),
+      gIR->context(), builder.defaultTypes(),
-                           std::string("nest.") + fd->toChars());
+      std::string("nest.") + fd->toChars(), builder.isPacked());
 
   IF_LOG Logger::cout() << "frameType = " << *frameType << '\n';
 
   // Store type in IrFunction
   irFunc.frameType = frameType;
-  irFunc.frameTypeAlignment = builder.overallAlignment();
+  irFunc.frameTypeAlignment = maxAlignment;
 }
 
 void DtoCreateNestedContext(FuncGenState &funcGen) {

ir/iraggr.cpp

@@ -205,8 +205,9 @@ IrAggr::createInitializerConstant(const VarInitMap &explicitInitializers) {
 
   // Add the initializers for the member fields.
   unsigned dummy = 0;
-  addFieldInitializers(constants, explicitInitializers, aggrdecl, offset,
+  bool isPacked = false;
-                       dummy);
+  addFieldInitializers(constants, explicitInitializers, aggrdecl, offset, dummy,
+                       isPacked);
 
   // tail padding?
   const size_t structsize = aggrdecl->size(Loc());
@@ -219,7 +220,7 @@ IrAggr::createInitializerConstant(const VarInitMap &explicitInitializers) {
   for (auto c : constants)
     types.push_back(c->getType());
 
-  auto llStructType = getLLStructType();
+  const auto llStructType = getLLStructType();
   bool isCompatible = (types.size() == llStructType->getNumElements());
   if (isCompatible) {
     for (size_t i = 0; i < types.size(); i++) {
@@ -231,10 +232,16 @@ IrAggr::createInitializerConstant(const VarInitMap &explicitInitializers) {
   }
 
   // build constant
-  const bool isPacked = getIrType(type)->isAggr()->packed;
+  LLStructType *llType = llStructType;
-  LLStructType *llType =
+  if (!isCompatible) {
-      isCompatible ? llStructType
+    // Note: isPacked only reflects whether there are misaligned IR fields,
-                   : LLStructType::get(gIR->context(), types, isPacked);
+    // not checking whether the aggregate contains appropriate tail padding.
+    // So the resulting constant might contain more (implicit) tail padding than
+    // the actual type, which should be harmless.
+    llType = LLStructType::get(gIR->context(), types, isPacked);
+    assert(getTypeAllocSize(llType) >= structsize);
+  }
+
   llvm::Constant *c = LLConstantStruct::get(llType, constants);
   IF_LOG Logger::cout() << "final initializer: " << *c << std::endl;
   return c;
@@ -243,12 +250,12 @@ IrAggr::createInitializerConstant(const VarInitMap &explicitInitializers) {
 void IrAggr::addFieldInitializers(
     llvm::SmallVectorImpl<llvm::Constant *> &constants,
     const VarInitMap &explicitInitializers, AggregateDeclaration *decl,
-    unsigned &offset, unsigned &interfaceVtblIndex) {
+    unsigned &offset, unsigned &interfaceVtblIndex, bool &isPacked) {
 
   if (ClassDeclaration *cd = decl->isClassDeclaration()) {
     if (cd->baseClass) {
       addFieldInitializers(constants, explicitInitializers, cd->baseClass,
-                           offset, interfaceVtblIndex);
+                           offset, interfaceVtblIndex, isPacked);
     }
 
     // has interface vtbls?
@@ -271,11 +278,13 @@ void IrAggr::addFieldInitializers(
     }
   }
 
-  AggrTypeBuilder b(false, offset);
+  AggrTypeBuilder b(offset);
   b.addAggregate(decl,
                  explicitInitializers.empty() ? nullptr : &explicitInitializers,
                  AggrTypeBuilder::Aliases::Skip);
   offset = b.currentOffset();
+  if (!isPacked)
+    isPacked = b.isPacked();
 
   const size_t baseLLFieldIndex = constants.size();
   const size_t numNewLLFields = b.defaultTypes().size();

ir/iraggr.h

@@ -106,7 +106,7 @@ private:
   void addFieldInitializers(llvm::SmallVectorImpl<llvm::Constant *> &constants,
                             const VarInitMap &explicitInitializers,
                             AggregateDeclaration *decl, unsigned &offset,
-                            unsigned &interfaceVtblIndex);
+                            unsigned &interfaceVtblIndex, bool &isPacked);
 };
 
 /// Represents a struct.

ir/irtypeaggr.cpp

@@ -37,15 +37,18 @@ bool var_offset_sort_cb(const VarDeclaration *v1, const VarDeclaration *v2) {
   return v1->offset < v2->offset;
 }
 
-AggrTypeBuilder::AggrTypeBuilder(bool packed, unsigned offset)
+AggrTypeBuilder::AggrTypeBuilder(unsigned offset) : m_offset(offset) {
-    : m_offset(offset), m_packed(packed) {
   m_defaultTypes.reserve(32);
 }
 
 void AggrTypeBuilder::addType(llvm::Type *type, unsigned size) {
+  const unsigned fieldAlignment = getABITypeAlign(type);
+  assert(fieldAlignment);
+  assert((m_offset & (fieldAlignment - 1)) == 0 && "Field is misaligned");
   m_defaultTypes.push_back(type);
   m_offset += size;
   m_fieldIndex++;
+  m_maxFieldIRAlignment = std::max(m_maxFieldIRAlignment, fieldAlignment);
 }
 
 void AggrTypeBuilder::addAggregate(AggregateDeclaration *ad) {
@@ -189,16 +192,24 @@ void AggrTypeBuilder::addAggregate(
     LLType *fieldType = getFieldType(vd);
     m_defaultTypes.push_back(fieldType);
 
-    // advance offset to right past this field
+    unsigned fieldAlignment, fieldSize;
     if (!fieldType->isSized()) {
       // forward reference in a cycle or similar, we need to trust the D type
-      m_offset += vd->type->size();
+      fieldAlignment = DtoAlignment(vd->type);
+      fieldSize = vd->type->size();
     } else {
-      const auto llSize = getTypeAllocSize(fieldType);
+      fieldAlignment = getABITypeAlign(fieldType);
-      assert(llSize <= vd->type->size() || vd->isBitFieldDeclaration());
+      fieldSize = getTypeAllocSize(fieldType);
-      m_offset += llSize;
+      assert(fieldSize <= vd->type->size() || vd->isBitFieldDeclaration());
     }
 
+    // advance offset to right past this field
+    if (!m_packed) {
+      assert(fieldAlignment);
+      m_packed = ((m_offset & (fieldAlignment - 1)) != 0);
+    }
+    m_offset += fieldSize;
+
     // set the field index
     m_varGEPIndices[vd] = m_fieldIndex;
 
@@ -209,12 +220,12 @@ void AggrTypeBuilder::addAggregate(
     }
 
     ++m_fieldIndex;
+
+    m_maxFieldIRAlignment = std::max(m_maxFieldIRAlignment, fieldAlignment);
   }
 }
 
 void AggrTypeBuilder::alignCurrentOffset(unsigned alignment) {
-  m_overallAlignment = std::max(alignment, m_overallAlignment);
-
   unsigned aligned = (m_offset + alignment - 1) & ~(alignment - 1);
   if (m_offset < aligned) {
     m_fieldIndex += add_zeros(m_defaultTypes, m_offset, aligned);
@@ -227,6 +238,10 @@ void AggrTypeBuilder::addTailPadding(unsigned aggregateSize) {
          "IR aggregate type is larger than the corresponding D type");
   if (m_offset < aggregateSize)
     add_zeros(m_defaultTypes, m_offset, aggregateSize);
+
+  // check if the aggregate size makes it packed in IR terms
+  if (!m_packed && (aggregateSize & (m_maxFieldIRAlignment - 1)))
+    m_packed = true;
 }
 
 //////////////////////////////////////////////////////////////////////////////
@@ -237,41 +252,6 @@ IrTypeAggr::IrTypeAggr(AggregateDeclaration *ad)
              LLStructType::create(gIR->context(), ad->toPrettyChars())),
       aggr(ad) {}
 
-bool IrTypeAggr::isPacked(AggregateDeclaration *ad) {
-  // If the aggregate's size is unknown, any field with type alignment > 1 will
-  // make it packed.
-  unsigned aggregateSize = ~0u;
-  unsigned aggregateAlignment = 1;
-  if (ad->sizeok == Sizeok::done) {
-    aggregateSize = ad->structsize;
-    aggregateAlignment = ad->alignsize;
-
-    if (auto sd = ad->isStructDeclaration()) {
-      if (!sd->alignment.isDefault() && !sd->alignment.isPack())
-        aggregateAlignment = sd->alignment.get();
-    }
-  }
-
-  // Classes apparently aren't padded; their size may not match the alignment.
-  assert((ad->isClassDeclaration() ||
-          (aggregateSize & (aggregateAlignment - 1)) == 0) &&
-         "Size not a multiple of alignment?");
-
-  // For unions, only a subset of the fields are actually used for the IR type -
-  // don't care (about a few potentially needlessly packed IR structs).
-  for (const auto field : ad->fields) {
-    // The aggregate size, aggregate alignment and the field offset need to be
-    // multiples of the field type's alignment, otherwise the aggregate type is
-    // unnaturally aligned, and LLVM would insert padding.
-    const unsigned fieldTypeAlignment = DtoAlignment(field->type);
-    const auto mask = fieldTypeAlignment - 1;
-    if ((aggregateSize | aggregateAlignment | field->offset) & mask)
-      return true;
-  }
-
-  return false;
-}
-
 void IrTypeAggr::getMemberLocation(VarDeclaration *var, unsigned &fieldIndex,
                                    unsigned &byteOffset) const {
   // Note: The interface is a bit more general than what we actually return.

ir/irtypeaggr.h

@@ -30,7 +30,7 @@ public:
   using VarInitMap = std::map<VarDeclaration *, llvm::Constant *>;
   enum class Aliases { Skip, AddToVarGEPIndices };
 
-  explicit AggrTypeBuilder(bool packed, unsigned offset = 0);
+  explicit AggrTypeBuilder(unsigned offset = 0);
   void addType(llvm::Type *type, unsigned size);
   void addAggregate(AggregateDeclaration *ad);
   void addAggregate(AggregateDeclaration *ad, const VarInitMap *explicitInits,
@@ -42,7 +42,7 @@ public:
     return m_defaultTypes;
   }
   const VarGEPIndices &varGEPIndices() const { return m_varGEPIndices; }
-  unsigned overallAlignment() const { return m_overallAlignment; }
+  bool isPacked() const { return m_packed; }
   unsigned currentOffset() const { return m_offset; }
 
 protected:
@@ -50,8 +50,8 @@ protected:
   VarGEPIndices m_varGEPIndices;
   unsigned m_offset = 0;
   unsigned m_fieldIndex = 0;
-  unsigned m_overallAlignment = 0;
+  unsigned m_maxFieldIRAlignment = 1;
-  bool m_packed = false;
+  bool m_packed = false; // in IR terms
 };
 
 /// Base class of IrTypes for aggregate types.
@@ -66,17 +66,10 @@ public:
   void getMemberLocation(VarDeclaration *var, unsigned &fieldIndex,
                          unsigned &byteOffset) const;
 
-  /// true, if the LLVM struct type for the aggregate is declared as packed
-  bool packed = false;
-
 protected:
   ///
   explicit IrTypeAggr(AggregateDeclaration *ad);
 
-  /// Returns true, if the LLVM struct type for the aggregate must be declared
-  /// as packed.
-  static bool isPacked(AggregateDeclaration *ad);
-
   /// AggregateDeclaration this type represents.
   AggregateDeclaration *aggr = nullptr;
 

ir/irtypeclass.cpp

@@ -70,13 +70,7 @@ IrTypeClass *IrTypeClass::get(ClassDeclaration *cd) {
   const unsigned instanceSize = cd->structsize;
   IF_LOG Logger::println("Instance size: %u", instanceSize);
 
-  // This class may contain an align declaration. See GitHub #726.
+  AggrTypeBuilder builder;
-  t->packed = false;
-  for (auto base = cd; base != nullptr && !t->packed; base = base->baseClass) {
-    t->packed = isPacked(base);
-  }
-
-  AggrTypeBuilder builder(t->packed);
 
   // add vtbl
   builder.addType(llvm::PointerType::get(t->vtbl_type, 0), target.ptrsize);
@@ -89,9 +83,7 @@ IrTypeClass *IrTypeClass::get(ClassDeclaration *cd) {
     // classes have monitor and fields
     if (!cd->isCPPclass() && !cd->isCPPinterface()) {
       // add monitor
-      builder.addType(
+      builder.addType(getVoidPtrType(), target.ptrsize);
-          llvm::PointerType::get(llvm::Type::getInt8Ty(gIR->context()), 0),
-          target.ptrsize);
     }
 
     // add data members recursively
@@ -103,7 +95,7 @@ IrTypeClass *IrTypeClass::get(ClassDeclaration *cd) {
   }
 
   // set struct body and copy GEP indices
-  isaStruct(t->type)->setBody(builder.defaultTypes(), t->packed);
+  isaStruct(t->type)->setBody(builder.defaultTypes(), builder.isPacked());
   t->varGEPIndices = builder.varGEPIndices();
 
   IF_LOG Logger::cout() << "class type: " << *t->type << std::endl;

ir/irtypestruct.cpp

@@ -65,8 +65,6 @@ IrTypeStruct *IrTypeStruct::get(StructDeclaration *sd) {
     return t;
   }
 
-  t->packed = isPacked(sd);
-
   if(isFromLDC_DCompute(sd)) {
     dcomputeTypes.push_back(t);
   }
@@ -83,15 +81,15 @@ IrTypeStruct *IrTypeStruct::get(StructDeclaration *sd) {
     llvm::SmallVector<LLType *, 1> body;
     body.push_back(DtoMemType(p->type)->getPointerTo(realAS));
 
-    isaStruct(t->type)->setBody(body, t->packed);
+    isaStruct(t->type)->setBody(body, false);
     VarGEPIndices v;
     v[sd->fields[0]] = 0;
     t->varGEPIndices = v;
   } else {
-    AggrTypeBuilder builder(t->packed);
+    AggrTypeBuilder builder;
     builder.addAggregate(sd);
     builder.addTailPadding(sd->structsize);
-    isaStruct(t->type)->setBody(builder.defaultTypes(), t->packed);
+    isaStruct(t->type)->setBody(builder.defaultTypes(), builder.isPacked());
     t->varGEPIndices = builder.varGEPIndices();
   }
 

tests/codegen/align_class.d

@@ -1,17 +1,17 @@
 // RUN: %ldc -output-ll -of=%t.ll %s && FileCheck %s < %t.ll
 // RUN: %ldc -run %s
 
-struct S16 { align(16) int a; }
+struct S16 { align(16) short a; }
 
-class D     { align(32) int d = 0xDD; }
+class D     { align(32) short d = 0xDD; }
-// CHECK: %align_class.D = type { [5 x i8*]*, i8*, [{{(16|24)}} x i8], i32 }
+// CHECK: %align_class.D = type <{ [5 x i8*]*, i8*, [{{(16|24)}} x i8], i16 }>
 class E : D { S16 s16 = S16(0xEE); }
-// CHECK: %align_class.E = type { [5 x i8*]*, i8*, [{{(16|24)}} x i8], i32, [12 x i8], %align_class.S16 }
+// CHECK: %align_class.E = type { [5 x i8*]*, i8*, [{{(16|24)}} x i8], i16, [14 x i8], %align_class.S16 }
-class F : D { align(64) int f = 0xFF; }
+class F : D { align(64) short f = 0xFF; }
-// CHECK: %align_class.F = type { [5 x i8*]*, i8*, [{{(16|24)}} x i8], i32, [28 x i8], i32 }
+// CHECK: %align_class.F = type <{ [5 x i8*]*, i8*, [{{(16|24)}} x i8], i16, [30 x i8], i16 }>
 
-extern(C++) class CppClass { align(32) int a = 0xAA; }
+extern(C++) class CppClass { align(32) short a = 0xAA; }
-// CHECK: %align_class.CppClass = type { [0 x i8*]*, [{{(24|28)}} x i8], i32 }
+// CHECK: %align_class.CppClass = type <{ [0 x i8*]*, [{{(24|28)}} x i8], i16 }>
 
 void main()
 {

tests/codegen/gh2346.d

@@ -1,15 +1,14 @@
 // RUN: %ldc -output-ll -of=%t.ll %s && FileCheck %s < %t.ll
 
-// Make sure the LL struct is packed due to an unnatural overall alignment of 1.
+// Make sure the LL struct isn't packed.
-// CHECK-DAG: %gh2346.UnalignedUInt = type <{ i32 }>
+// CHECK-DAG: %gh2346.UnalignedUInt = type { i32 }
 struct UnalignedUInt {
     align(1) uint a;
 }
 static assert(UnalignedUInt.alignof == 1);
 static assert(UnalignedUInt.sizeof == 4);
 
-// Then there's no need to pack naturally aligned containers.
+// CHECK-DAG: %gh2346.Container = type <{ i8, %gh2346.UnalignedUInt }>
-// CHECK-DAG: %gh2346.Container = type { i8, %gh2346.UnalignedUInt }
 struct Container {
     ubyte one;
     UnalignedUInt two;
@@ -18,14 +17,14 @@ static assert(Container.alignof == 1);
 static assert(Container.sizeof == 5);
 static assert(Container.two.offsetof == 1);
 
-// CHECK-DAG: %gh2346.UnalignedUInt2 = type <{ i32 }>
+// CHECK-DAG: %gh2346.UnalignedUInt2 = type { i32 }
 struct UnalignedUInt2 {
     align(2) uint a;
 }
 static assert(UnalignedUInt2.alignof == 2);
 static assert(UnalignedUInt2.sizeof == 4);
 
-// CHECK-DAG: %gh2346.Container2 = type { i8, [1 x i8], %gh2346.UnalignedUInt2 }
+// CHECK-DAG: %gh2346.Container2 = type <{ i8, [1 x i8], %gh2346.UnalignedUInt2 }>
 struct Container2 {
     ubyte one;
     UnalignedUInt2 two;
@@ -43,7 +42,7 @@ static assert(PackedContainer2.alignof == 1);
 static assert(PackedContainer2.sizeof == 5);
 static assert(PackedContainer2.two.offsetof == 1);
 
-// CHECK-DAG: %gh2346.WeirdContainer = type { i8, [1 x i8], %gh2346.UnalignedUInt, [2 x i8] }
+// CHECK-DAG: %gh2346.WeirdContainer = type <{ i8, [1 x i8], %gh2346.UnalignedUInt, [2 x i8] }>
 align(4) struct WeirdContainer {
     ubyte one;
     align(2) UnalignedUInt two;
@@ -52,14 +51,14 @@ static assert(WeirdContainer.alignof == 4);
 static assert(WeirdContainer.sizeof == 8);
 static assert(WeirdContainer.two.offsetof == 2);
 
-// CHECK-DAG: %gh2346.ExplicitlyUnalignedUInt2 = type <{ i32 }>
+// CHECK-DAG: %gh2346.ExplicitlyUnalignedUInt2 = type { i32 }
 align(2) struct ExplicitlyUnalignedUInt2 {
     uint a;
 }
 static assert(ExplicitlyUnalignedUInt2.alignof == 2);
 static assert(ExplicitlyUnalignedUInt2.sizeof == 4);
 
-// CHECK-DAG: %gh2346.AnotherContainer = type { i8, [1 x i8], %gh2346.ExplicitlyUnalignedUInt2 }
+// CHECK-DAG: %gh2346.AnotherContainer = type <{ i8, [1 x i8], %gh2346.ExplicitlyUnalignedUInt2 }>
 struct AnotherContainer {
     ubyte one;
     ExplicitlyUnalignedUInt2 two;