[GlobalMerge] Allow merging globals with explicit section markings.

[efriedma]

Summary

At least on ELF, it's impossible to tell without disassembling the code whether two globals with the same section marking were merged: the merged global uses "private" linkage to hide its symbol, and the aliases look like regular symbols. I can't think of any other reason to disallow it. (Of course, we can only merge globals in the same section.)

The weird alignment handling matches AsmPrinter; our alignment handling for global variables should probably be refactored.

This transform is very profitable for one proprietary codebase I've been looking at, which uses a lot of globals with section markings.

[greened]

Not sure I'm qualified to comment on the correctness/desirability of this. I don't really deal with ELF or linkage issues. I don't know if some tool might want to know about two symbols with the same section marking.

lib/CodeGen/GlobalMerge.cpp

466 ↗

(On Diff #157356)

Comment here about why this overrides getPreferredAlignment?

[dmgreen]

We (Arm) have also received cases that track back to this issue too. I triaged one just recently where a change in testbench caused large changes in code performance. So I agree with this, looks like a sensible idea.

[greened]

lib/CodeGen/GlobalMerge.cpp

466 ↗

(On Diff #157356)

Or more specifically, why this overrides getPreferredAlignent only if it has a section but not otherwise.

[efriedma]

Add a big comment explaining the alignment computation.

[efriedma]

Not sure who else to add as a reviewer.

[dmgreen]

This looks good to me.

[peter.smith]

I think this looks fine from an ELF perspective. As I understand it the default section names like .data and .bss are more convention than a requirement so it shouldn't matter what the name of the section is.

[tobiasvk]

There is this vague, unspecified idea across the code base not to "touch" anything that hasSection(). We should be more deliberate and decide on a case-by-case basis, so this looks like the right thing to do for this pass.

llvm/trunk/lib/CodeGen/GlobalMerge.cpp

while (i != -1) {
     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();
+
+      // 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.
       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));
Twine MergedName =
             ? "_MergedGlobals_" + FirstExternalName
             : "_MergedGlobals";
     auto MergedLinkage = IsMachO ? Linkage : GlobalValue::PrivateLinkage;
     auto *MergedGV = new GlobalVariable(
         M, MergedTy, isConst, MergedLinkage, MergedInit, MergedName, nullptr,
         GlobalVariable::NotThreadLocal, AddrSpace);
 
     MergedGV->setAlignment(MaxAlign);
+    MergedGV->setSection(Globals[i]->getSection());
 
     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();
 

 
 bool GlobalMerge::doInitialization(Module &M) {
   if (!EnableGlobalMerge)
     return false;
 
   IsMachO = Triple(M.getTargetTriple()).isOSBinFormatMachO();
 
   auto &DL = M.getDataLayout();
-  DenseMap<unsigned, SmallVector<GlobalVariable *, 16>> Globals, ConstGlobals,
-                                                        BSSGlobals;
+  DenseMap<std::pair<unsigned, StringRef>, SmallVector<GlobalVariable *, 16>>
+      Globals, ConstGlobals, BSSGlobals;
   bool Changed = false;
   setMustKeepGlobalVariables(M);
 
   // Grab all non-const globals.
   for (auto &GV : M.globals()) {
     // Merge is safe for "normal" internal or external globals only
-    if (GV.isDeclaration() || GV.isThreadLocal() ||
-        GV.hasSection() || GV.hasImplicitSection())
+    if (GV.isDeclaration() || GV.isThreadLocal() || GV.hasImplicitSection())
       continue;
 
     // It's not safe to merge globals that may be preempted
     if (TM && !TM->shouldAssumeDSOLocal(M, &GV))
       continue;
 
     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();
+    StringRef Section = GV.getSection();
 
     // 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);
+        BSSGlobals[{AddressSpace, Section}].push_back(&GV);
       else if (GV.isConstant())
-        ConstGlobals[AddressSpace].push_back(&GV);
+        ConstGlobals[{AddressSpace, Section}].push_back(&GV);
       else
-        Globals[AddressSpace].push_back(&GV);
+        Globals[{AddressSpace, Section}].push_back(&GV);
     }
   }
 
   for (auto &P : Globals)
     if (P.second.size() > 1)
-      Changed |= doMerge(P.second, M, false, P.first);
+      Changed |= doMerge(P.second, M, false, P.first.first);
 
   for (auto &P : BSSGlobals)
     if (P.second.size() > 1)
-      Changed |= doMerge(P.second, M, false, P.first);
+      Changed |= doMerge(P.second, M, false, P.first.first);
 
   if (EnableGlobalMergeOnConst)
     for (auto &P : ConstGlobals)
       if (P.second.size() > 1)
-        Changed |= doMerge(P.second, M, true, P.first);
+        Changed |= doMerge(P.second, M, true, P.first.first);
 
   return Changed;
 }
 
 bool GlobalMerge::runOnFunction(Function &F) {
   return false;
 }
 

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 }>, align 4
+; CHECK: @_MergedGlobals = private global <{ i32, i32 }> <{ i32 3, i32 4 }>, section "foo", align 4
+; CHECK: @_MergedGlobals.1 = 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-DAG: @a = internal alias i32, getelementptr inbounds (<{ i32, i32 }>, <{ i32, i32 }>* @_MergedGlobals.1, 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-DAG: @b = internal alias i32, getelementptr inbounds (<{ i32, i32 }>, <{ i32, i32 }>* @_MergedGlobals.1, i32 0, i32 1)
 @b = internal global i32 2
 
+; CHECK-DAG: @c = internal alias i32, getelementptr inbounds (<{ i32, i32 }>, <{ i32, i32 }>* @_MergedGlobals, i32 0, i32 0)
+@c = internal global i32 3, section "foo"
+
+; CHECK-DAG: @d = internal alias i32, getelementptr inbounds (<{ i32, i32 }>, <{ i32, i32 }>* @_MergedGlobals, i32 0, i32 1)
+@d = internal global i32 4, section "foo"
+
 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.1, 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.1, i32 0, i32 1)
   %y = load i32, i32* @b
+  ; CHECK: load i32, i32* getelementptr inbounds (<{ i32, i32 }>, <{ i32, i32 }>* @_MergedGlobals, i32 0, i32 0)
+  %z1 = load i32, i32* @c
+  ; CHECK: load i32, i32* getelementptr inbounds (<{ i32, i32 }>, <{ i32, i32 }>* @_MergedGlobals, i32 0, i32 1)
+  %z2 = load i32, i32* @d
   ret void
 }