[NFC] Make getPreferredAlignment honor section markings.

[efriedma]

Summary

This should more accurately reflect what the AsmPrinter will actually do.

This is NFC, as far as I can tell; all the places that might be affected already have an extra check to avoid using the result of getPreferredAlignment in this situation.

[echristo]

Yeah, that works for me. If we start getting something wrong it's an edge case we should probably document :)

[dmgreen]

Thanks for this.

lib/IR/DataLayout.cpp

834 ↗

(On Diff #162934)

About this, it keeps adding very visible codesize (technially datasize) to our images, which because our linker reports all the padding is very visible to customers, and unnecissarily bloats the images.

I ran some benchmarks and couldn't find any places it was improving performance, at least on ARM/AArch64. Remove this actually improved things in a few places.

The over-aligning globals dates back to a long time ago, I'm just not sure it's worth doing on many architectures. From what I remember, removing it does fail some X86 tests though, with worse looking code.

We may need be smarter than that and come up with some way to remove it for targets (or a way to mark globals as optsize :-/ )

[efriedma]

lib/IR/DataLayout.cpp

834 ↗

(On Diff #162934)

Are you sure it's this code, specifically, causing the bloat? As far as I know, clang explicitly marks the alignment of all globals it creates, which makes this code irrelevant.

[dmgreen]

lib/IR/DataLayout.cpp

834 ↗

(On Diff #162934)

Yes but...

The customer reported case I remember is the RTTI string are not aligned. Something like this:

class MyNormalClass {
    public:
            virtual int foo() const { return 1; };
};

class OveralignClass {
    public:
            virtual int foo() const { return 1; };
};

MyNormalClass a;
OveralignClass b;

"14OveralignClass", being 17 characters long, gets over aligned.

The other I found from looking around is the printf->puts conversion doesn't put and alignment on the strings it creates. I've just created D51410 for that one.

Anyway, none of that is really related to this patch. +1 from me, thanks for making this less easy to get wrong.

llvm/trunk/lib/CodeGen/GlobalMerge.cpp

while (i != -1) {
 
     bool HasExternal = false;
     StringRef FirstExternalName;
     unsigned MaxAlign = 1;
     unsigned CurIdx = 0;
     for (j = i; j != -1; j = GlobalSet.find_next(j)) {
       Type *Ty = Globals[j]->getValueType();
 
-      // Make sure we use the same alignment AsmPrinter would use. There
-      // currently isn't any helper to compute that, so we compute it
-      // explicitly here.
-      //
-      // getPreferredAlignment will sometimes return an alignment higher
-      // than the explicitly specified alignment; we must ignore that
-      // if the section is explicitly specified, to avoid inserting extra
-      // padding into that section.
+      // Make sure we use the same alignment AsmPrinter would use.
       unsigned Align = DL.getPreferredAlignment(Globals[j]);
-      if (Globals[j]->hasSection() && Globals[j]->getAlignment())
-        Align = Globals[j]->getAlignment();
       unsigned Padding = alignTo(MergedSize, Align) - MergedSize;
       MergedSize += Padding;
       MergedSize += DL.getTypeAllocSize(Ty);
       if (MergedSize > MaxOffset) {
         break;
       }
       if (Padding) {
         Tys.push_back(ArrayType::get(Int8Ty, Padding));

llvm/trunk/lib/IR/DataLayout.cpp

int64_t DataLayout::getIndexedOffsetInType(Type *ElemTy,
 
   return Result;
 }
 
 /// getPreferredAlignment - Return the preferred alignment of the specified
 /// global.  This includes an explicitly requested alignment (if the global
 /// has one).
 unsigned DataLayout::getPreferredAlignment(const GlobalVariable *GV) const {
+  unsigned GVAlignment = GV->getAlignment();
+  // If a section is specified, always precisely honor explicit alignment,
+  // so we don't insert padding into a section we don't control.
+  if (GVAlignment && GV->hasSection())
+    return GVAlignment;
+
+  // If no explicit alignment is specified, compute the alignment based on
+  // the IR type. If an alignment is specified, increase it to match the ABI
+  // alignment of the IR type.
+  //
+  // FIXME: Not sure it makes sense to use the alignment of the type if
+  // there's already an explicit alignment specification.
   Type *ElemType = GV->getValueType();
   unsigned Alignment = getPrefTypeAlignment(ElemType);
-  unsigned GVAlignment = GV->getAlignment();
   if (GVAlignment >= Alignment) {
     Alignment = GVAlignment;
   } else if (GVAlignment != 0) {
     Alignment = std::max(GVAlignment, getABITypeAlignment(ElemType));
   }
 
+  // If no explicit alignment is specified, and the global is large, increase
+  // the alignment to 16.
+  // FIXME: Why 16, specifically?
   if (GV->hasInitializer() && GVAlignment == 0) {
     if (Alignment < 16) {
       // If the global is not external, see if it is large.  If so, give it a
       // larger alignment.
       if (getTypeSizeInBits(ElemType) > 128)
         Alignment = 16;    // 16-byte alignment.
     }
   }