[GlobalMerge] Allow merging globals with arbitrary alignment.

[efriedma]

Summary

Instead of depending on implicit padding from the structure layout code, use a packed struct and emit the padding explicitly.

[greened]

lib/CodeGen/GlobalMerge.cpp

464 ↗

(On Diff #156936)

The eliminated check below used DL.getPreferredAlignment. Is this equivalent? If I understand getPointerAlignment correctly, it is more conservative than DL.getPreferredAlignment. So it seems the eliminated check was too conservative. It eliminated things that might not have had a problematic alignment.

Just making sure I'm understanding this correctly.

[greened]

LGTM, assuming my understanding of the alignment question is correct. Thanks!

[efriedma]

lib/CodeGen/GlobalMerge.cpp

464 ↗

(On Diff #156936)

For a global with a strong definition, getPointerAlignment() is basically something like GVar->getAlignment() == 0 ? DL.getPreferredAlignment(GVar) : GVar->getAlignment(). That isn't exactly consistent with what the backend would do; getGVAlignmentLog2 seems to do something more like std::max(DL.getPreferredAlignment(GVar), GVar->getAlignment()). This doesn't really matter because getPreferredAlignment itself returns GVar->getAlignment() for globals with explicit alignment... unless the explicit alignment of the global is less than the ABI alignment of the global's type, in which case the alignment is forced to the ABI alignment of the type. But this result is ignored by getGVAlignmentLog2 if the global has a section marking, in favor of the explicit alignment. (This was introduced in r129428/r129432 for reasons I don't really understand.)

So it's a mess, but getPointerAlignment() should return the same result except for the weird edge case involving an explicitly under-aligned global.

[dmgreen]

+1. Looks sensible, and the benchmark numbers agree. Thanks for improving this.

[greened]

lib/CodeGen/GlobalMerge.cpp

464 ↗

(On Diff #156936)

Ok, and for that weird edge case it would force an alignment greater than the explicit alignment. That seems fine given the other benefits of this change. Thanks!

[efriedma]

Use DataLayout::getPreferredAlignment so we end up with the same alignment the AsmPrinter would have computed. Not sure if the AsmPrinter is actually behaving the way we want it to, but we should consider that separately.

llvm/trunk/lib/CodeGen/GlobalMerge.cpp

 }
 
 bool GlobalMerge::doMerge(const SmallVectorImpl<GlobalVariable *> &Globals,
                           const BitVector &GlobalSet, Module &M, bool isConst,
                           unsigned AddrSpace) const {
   assert(Globals.size() > 1);
 
   Type *Int32Ty = Type::getInt32Ty(M.getContext());
+  Type *Int8Ty = Type::getInt8Ty(M.getContext());
   auto &DL = M.getDataLayout();
 
   LLVM_DEBUG(dbgs() << " Trying to merge set, starts with #"
                     << GlobalSet.find_first() << "\n");
 
   bool Changed = false;
   ssize_t i = GlobalSet.find_first();
   while (i != -1) {
     ssize_t j = 0;
     uint64_t MergedSize = 0;
     std::vector<Type*> Tys;
     std::vector<Constant*> Inits;
+    std::vector<unsigned> StructIdxs;
 
     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();
+      unsigned Align = DL.getPreferredAlignment(Globals[j]);
+      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));
+        Inits.push_back(ConstantAggregateZero::get(Tys.back()));
+        ++CurIdx;
+      }
       Tys.push_back(Ty);
       Inits.push_back(Globals[j]->getInitializer());
+      StructIdxs.push_back(CurIdx++);
+
+      MaxAlign = std::max(MaxAlign, Align);
 
       if (Globals[j]->hasExternalLinkage() && !HasExternal) {
         HasExternal = true;
         FirstExternalName = Globals[j]->getName();
       }
     }
 
     // Exit early if there is only one global to merge.
     if (Tys.size() < 2) {
       i = j;
       continue;
     }
 
     // If merged variables doesn't have external linkage, we needn't to expose
     // the symbol after merging.
     GlobalValue::LinkageTypes Linkage = HasExternal
                                             ? GlobalValue::ExternalLinkage
                                             : GlobalValue::InternalLinkage;
-    StructType *MergedTy = StructType::get(M.getContext(), Tys);
+    // Use a packed struct so we can control alignment.
+    StructType *MergedTy = StructType::get(M.getContext(), Tys, true);
     Constant *MergedInit = ConstantStruct::get(MergedTy, Inits);
 
     // On Darwin external linkage needs to be preserved, otherwise
     // dsymutil cannot preserve the debug info for the merged
     // variables.  If they have external linkage, use the symbol name
     // of the first variable merged as the suffix of global symbol
     // name.  This avoids a link-time naming conflict for the
     // _MergedGlobals symbols.
     Twine MergedName =
         (IsMachO && HasExternal)
             ? "_MergedGlobals_" + FirstExternalName
             : "_MergedGlobals";
     auto MergedLinkage = IsMachO ? Linkage : GlobalValue::PrivateLinkage;
     auto *MergedGV = new GlobalVariable(
         M, MergedTy, isConst, MergedLinkage, MergedInit, MergedName, nullptr,
         GlobalVariable::NotThreadLocal, AddrSpace);
 
-    const StructLayout *MergedLayout = DL.getStructLayout(MergedTy);
-    // Set the alignment of the merged struct as the maximum alignment of the
-    // globals to prevent over-alignment. We don't handle globals that are not
-    // default aligned, so the alignment of the MergedLayout struct is
-    // equivalent.
-    MergedGV->setAlignment(MergedLayout->getAlignment());
+    MergedGV->setAlignment(MaxAlign);
 
+    const StructLayout *MergedLayout = DL.getStructLayout(MergedTy);
     for (ssize_t k = i, idx = 0; k != j; k = GlobalSet.find_next(k), ++idx) {
       GlobalValue::LinkageTypes Linkage = Globals[k]->getLinkage();
       std::string Name = Globals[k]->getName();
       GlobalValue::DLLStorageClassTypes DLLStorage =
           Globals[k]->getDLLStorageClass();
 
       // Copy metadata while adjusting any debug info metadata by the original
       // global's offset within the merged global.
-      MergedGV->copyMetadata(Globals[k], MergedLayout->getElementOffset(idx));
+      MergedGV->copyMetadata(Globals[k],
+                             MergedLayout->getElementOffset(StructIdxs[idx]));
 
       Constant *Idx[2] = {
           ConstantInt::get(Int32Ty, 0),
-        ConstantInt::get(Int32Ty, idx),
+          ConstantInt::get(Int32Ty, StructIdxs[idx]),
       };
       Constant *GEP =
           ConstantExpr::getInBoundsGetElementPtr(MergedTy, MergedGV, Idx);
       Globals[k]->replaceAllUsesWith(GEP);
       Globals[k]->eraseFromParent();
 
       // When the linkage is not internal we must emit an alias for the original
       // variable name as it may be accessed from another object. On non-Mach-O
       // we can also emit an alias for internal linkage as it's safe to do so.
       // It's not safe on Mach-O as the alias (and thus the portion of the
       // MergedGlobals variable) may be dead stripped at link time.
       if (Linkage != GlobalValue::InternalLinkage || !IsMachO) {
-        GlobalAlias *GA =
-            GlobalAlias::create(Tys[idx], AddrSpace, Linkage, Name, GEP, &M);
+        GlobalAlias *GA = GlobalAlias::create(Tys[StructIdxs[idx]], AddrSpace,
+                                              Linkage, Name, GEP, &M);
         GA->setDLLStorageClass(DLLStorage);
       }
 
       NumMerged++;
     }
     Changed = true;
     i = j;
   }
if (!(MergeExternalGlobals && GV.hasExternalLinkage()) &&
         !GV.hasInternalLinkage())
       continue;
 
     PointerType *PT = dyn_cast<PointerType>(GV.getType());
     assert(PT && "Global variable is not a pointer!");
 
     unsigned AddressSpace = PT->getAddressSpace();
 
-    // Ignore fancy-aligned globals for now.
-    unsigned Alignment = DL.getPreferredAlignment(&GV);
-    Type *Ty = GV.getValueType();
-    if (Alignment > DL.getABITypeAlignment(Ty))
-      continue;
-
     // Ignore all 'special' globals.
     if (GV.getName().startswith("llvm.") ||
         GV.getName().startswith(".llvm."))
       continue;
 
     // Ignore all "required" globals:
     if (isMustKeepGlobalVariable(&GV))
       continue;
 
+    Type *Ty = GV.getValueType();
     if (DL.getTypeAllocSize(Ty) < MaxOffset) {
       if (TM &&
           TargetLoweringObjectFile::getKindForGlobal(&GV, *TM).isBSSLocal())
         BSSGlobals[AddressSpace].push_back(&GV);
       else if (GV.isConstant())
         ConstGlobals[AddressSpace].push_back(&GV);
       else
         Globals[AddressSpace].push_back(&GV);

llvm/trunk/test/Transforms/GlobalMerge/alignment-2.ll

+; RUN: opt -global-merge -global-merge-max-offset=100 -S -o - %s | FileCheck %s
+
+target datalayout = "e-p:64:64"
+target triple = "x86_64-unknown-linux-gnu"
+
+; This produces align 4, not the obvious align 1, to be consistent with what
+; the AsmPrinter would do.
+; CHECK: @_MergedGlobals = private global <{ [2 x i32], [2 x i32] }> <{ [2 x i32] [i32 1, i32 1], [2 x i32] [i32 2, i32 2] }>, align 4
+
+; CHECK: @a = internal alias [2 x i32], getelementptr inbounds (<{ [2 x i32], [2 x i32] }>, <{ [2 x i32], [2 x i32] }>* @_MergedGlobals, i32 0, i32 0)
+@a = internal global [2 x i32] [i32 1, i32 1], align 1
+
+; CHECK: @b = internal alias [2 x i32], getelementptr inbounds (<{ [2 x i32], [2 x i32] }>, <{ [2 x i32], [2 x i32] }>* @_MergedGlobals, i32 0, i32 1)
+@b = internal global [2 x i32] [i32 2, i32 2], align 1
+
+define void @use() {
+  ; CHECK: load i32, i32* getelementptr inbounds (<{ [2 x i32], [2 x i32] }>, <{ [2 x i32], [2 x i32] }>* @_MergedGlobals, i32 0, i32 0, i32 0)
+  %x = load i32, i32* bitcast ([2 x i32]* @a to i32*)
+  ; CHECK: load i32, i32* getelementptr inbounds (<{ [2 x i32], [2 x i32] }>, <{ [2 x i32], [2 x i32] }>* @_MergedGlobals, i32 0, i32 1, i32 0)
+  %y = load i32, i32* bitcast ([2 x i32]* @b to i32*)
+  ret void
+}

llvm/trunk/test/Transforms/GlobalMerge/alignment.ll

+; RUN: opt -global-merge -global-merge-max-offset=100 -S -o - %s | FileCheck %s
+
+target datalayout = "e-p:64:64"
+target triple = "x86_64-unknown-linux-gnu"
+
+; CHECK: @_MergedGlobals = private global <{ [5 x i8], [3 x i8], [2 x i32] }> <{ [5 x i8] c"\01\01\01\01\01", [3 x i8] zeroinitializer, [2 x i32] [i32 2, i32 2] }>, align 4
+
+; CHECK: @a = internal alias [5 x i8], getelementptr inbounds (<{ [5 x i8], [3 x i8], [2 x i32] }>, <{ [5 x i8], [3 x i8], [2 x i32] }>* @_MergedGlobals, i32 0, i32 0)
+@a = internal global [5 x i8] [i8 1, i8 1, i8 1, i8 1, i8 1], align 4
+
+; CHECK: @b = internal alias [2 x i32], getelementptr inbounds (<{ [5 x i8], [3 x i8], [2 x i32] }>, <{ [5 x i8], [3 x i8], [2 x i32] }>* @_MergedGlobals, i32 0, i32 2)
+@b = internal global [2 x i32] [i32 2, i32 2]
+
+define void @use() {
+  ; CHECK: load i32, i32* bitcast (<{ [5 x i8], [3 x i8], [2 x i32] }>* @_MergedGlobals to i32*)
+  %x = load i32, i32* bitcast ([5 x i8]* @a to i32*)
+  ; CHECK: load i32, i32* getelementptr inbounds (<{ [5 x i8], [3 x i8], [2 x i32] }>, <{ [5 x i8], [3 x i8], [2 x i32] }>* @_MergedGlobals, i32 0, i32 2, i32 0)
+  %y = load i32, i32* bitcast ([2 x i32]* @b to i32*)
+  ret void
+}

llvm/trunk/test/Transforms/GlobalMerge/basic.ll

 ; RUN: opt -global-merge -global-merge-max-offset=100 -S -o - %s | FileCheck %s
 
 target datalayout = "e-p:64:64"
 target triple = "x86_64-unknown-linux-gnu"
 
-; CHECK: @_MergedGlobals = private global { i32, i32 } { i32 1, i32 2 }
+; CHECK: @_MergedGlobals = private global <{ i32, i32 }> <{ i32 1, i32 2 }>, align 4
 
-; CHECK: @a = internal alias i32, getelementptr inbounds ({ i32, i32 }, { i32, i32 }* @_MergedGlobals, i32 0, i32 0)
+; CHECK: @a = internal alias i32, getelementptr inbounds (<{ i32, i32 }>, <{ i32, i32 }>* @_MergedGlobals, i32 0, i32 0)
 @a = internal global i32 1
 
-; CHECK: @b = internal alias i32, getelementptr inbounds ({ i32, i32 }, { i32, i32 }* @_MergedGlobals, i32 0, i32 1)
+; CHECK: @b = internal alias i32, getelementptr inbounds (<{ i32, i32 }>, <{ i32, i32 }>* @_MergedGlobals, i32 0, i32 1)
 @b = internal global i32 2
 
 define void @use() {
-  ; CHECK: load i32, i32* getelementptr inbounds ({ i32, i32 }, { i32, i32 }* @_MergedGlobals, i32 0, i32 0)
+  ; CHECK: load i32, i32* getelementptr inbounds (<{ i32, i32 }>, <{ i32, i32 }>* @_MergedGlobals, i32 0, i32 0)
   %x = load i32, i32* @a
-  ; CHECK: load i32, i32* getelementptr inbounds ({ i32, i32 }, { i32, i32 }* @_MergedGlobals, i32 0, i32 1)
+  ; CHECK: load i32, i32* getelementptr inbounds (<{ i32, i32 }>, <{ i32, i32 }>* @_MergedGlobals, i32 0, i32 1)
   %y = load i32, i32* @b
   ret void
 }

llvm/trunk/test/Transforms/GlobalMerge/debug-info.ll

 ; RUN: opt -global-merge -global-merge-max-offset=100 -S -o - %s | FileCheck %s
 
 source_filename = "test/Transforms/GlobalMerge/debug-info.ll"
 target datalayout = "e-p:64:64"
 target triple = "x86_64-unknown-linux-gnu"
-; CHECK: @_MergedGlobals = private global { i32, i32 } { i32 1, i32 2 }, align 4, !dbg [[A:![0-9]+]], !dbg [[B:![0-9]+]]
+; CHECK: @_MergedGlobals = private global <{ i32, i32 }> <{ i32 1, i32 2 }>, align 4, !dbg [[A:![0-9]+]], !dbg [[B:![0-9]+]]
 
 @a = internal global i32 1, !dbg !0
 @b = internal global i32 2, !dbg !2
 
 define void @use1() {
   %x = load i32, i32* @a
   %y = load i32, i32* @b
   ret void