Merge of global constants does not happen when constants have common linkage

[ramred01]

Summary

Merge of global constants with common linkage does not happen even when the constants are used in successive instructions. It generates two separate labels for the two constants and hence materializes the two label addresses separately before loading the two constants, when merging the two constants and having a common label would have meant loading the label address once in a base register and then loading the constants using an offset from that label.

The problem here is that when the constants neither have external linkage nor internal linkage but have a common linkage, they are being skipped from the Global Merge analysis. This is causing such constants to be emitted separately with separate labels and the generated code materializes those multiple labels separately instead of materializing one label and then accessing the other constants with an offset from that label.

This fix checks for the values to have common linkage in addition to the external or internal linkages.

[dexonsmith]

+ab, do you still know this code?

[john.brawn]

The title and summary of this patch describe a problem, but they don't describe how this patch fixes that problem (which makes it hard to figure out if this patch is correctly fixing the problem). It should be more like "[GlobalMerge] Do XYZ: We have problem ABC, doing XYZ in GlobalMerge fixes it because ...". Also this patch needs tests.

[ramred01]

Updated the summary.

Added a test case.

[efriedma]

This isn't correct; GlobalMerge is not legal for symbols with weak linkage, and there isn't any way to correctly emit an alias with "common" linkage in ELF.

(If you want to avoid common linkage for C code, you can build with -fno-common; this is probably a good idea for codebases which are compatible with it.)

[ramred01]

Updated the patch with a fix that does not try to merge globals with common linkage as the earlier patch did.

The call to createGlobalMergePass was not passing the mergeExternal parameter which should be set to True for all non-Mach-O ports. This was causing the merge of global constants to fail even when -fno-common was being passed.

[efriedma]

Could you post this as a new patch? This seems very different from the original description/discussion.

include/llvm/IR/GlobalAlias.h

const Constant *getAliasee() const {
     return getIndirectSymbol();
   }
   Constant *getAliasee() {
     return getIndirectSymbol();
   }
 
   static bool isValidLinkage(LinkageTypes L) {
     return isExternalLinkage(L) || isLocalLinkage(L) ||
-      isWeakLinkage(L) || isLinkOnceLinkage(L);
+      isWeakLinkage(L) || isLinkOnceLinkage(L) || 
+      isCommonLinkage(L);
   }
 
   // Methods for support type inquiry through isa, cast, and dyn_cast:
   static bool classof(const Value *V) {
     return V->getValueID() == Value::GlobalAliasVal;
   }
 };
 
 } // end namespace llvm
 
 #endif // LLVM_IR_GLOBALALIAS_H

lib/CodeGen/AsmPrinter/AsmPrinter.cpp

void AsmPrinter::emitGlobalGOTEquivs() {
   for (auto *GV : FailedCandidates)
     EmitGlobalVariable(GV);
 }
 
 void AsmPrinter::emitGlobalIndirectSymbol(Module &M,
                                           const GlobalIndirectSymbol& GIS) {
   MCSymbol *Name = getSymbol(&GIS);
 
-  if (GIS.hasExternalLinkage() || !MAI->getWeakRefDirective())
+  if (GIS.hasExternalLinkage() || GIS.hasCommonLinkage() ||
+      !MAI->getWeakRefDirective())
     OutStreamer->EmitSymbolAttribute(Name, MCSA_Global);
   else if (GIS.hasWeakLinkage() || GIS.hasLinkOnceLinkage())
     OutStreamer->EmitSymbolAttribute(Name, MCSA_WeakReference);
   else
-    assert(GIS.hasLocalLinkage() && "Invalid alias or ifunc linkage");
+    assert((GIS.hasLocalLinkage()) &&
+           "Invalid alias or ifunc linkage");
 
   // Set the symbol type to function if the alias has a function type.
   // This affects codegen when the aliasee is not a function.
   if (GIS.getType()->getPointerElementType()->isFunctionTy()) {
     OutStreamer->EmitSymbolAttribute(Name, MCSA_ELF_TypeFunction);
     if (isa<GlobalIFunc>(GIS))
       OutStreamer->EmitSymbolAttribute(Name, MCSA_ELF_TypeIndFunction);
   }

lib/CodeGen/GlobalMerge.cpp

for (auto &GV : M.globals()) {
     // Merge is safe for "normal" internal or external globals only
     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())
+    if (!(MergeExternalGlobals && GV.hasExternalLinkage())&&
+        !GV.hasInternalLinkage() && !GV.hasCommonLinkage())
       continue;
 
     PointerType *PT = dyn_cast<PointerType>(GV.getType());
     assert(PT && "Global variable is not a pointer!");
 
     unsigned AddressSpace = PT->getAddressSpace();
     StringRef Section = GV.getSection();
 

test/CodeGen/AArch64/global_merge_ac6_aarch64.ll

+;RUN: llc -O3 %s -o - -verify-machineinstrs | FileCheck %s
+
+target datalayout = "e-m:e-i8:8:32-i16:16:32-i64:64-i128:128-n32:64-S128"
+target triple = "aarch64-arm-none-eabi"
+
+@global0 = common dso_local local_unnamed_addr global i32 0, align 4
+@global1 = common dso_local local_unnamed_addr global i32 0, align 4
+
+; Function Attrs: norecurse nounwind readonly
+;CHECK-LABEL: @func
+;CHECK: adrp
+;CHECK-NEXT: add
+;CHECK-NEXT: ldp
+;CHECK-NEXT: add
+;CHECK-NEXT: ret
+define dso_local i32 @func() local_unnamed_addr #0 {
+  %1 = load i32, i32* @global0, align 4, !tbaa !2
+  %2 = load i32, i32* @global1, align 4, !tbaa !2
+  %3 = add nsw i32 %2, %1
+  ret i32 %3
+}
+
+attributes #0 = { norecurse nounwind readonly "correctly-rounded-divide-sqrt-fp-math"="false" "disable-tail-calls"="false" "less-precise-fpmad"="false" "min-legal-vector-width"="0" "no-frame-pointer-elim"="true" "no-frame-pointer-elim-non-leaf" "no-infs-fp-math"="false" "no-jump-tables"="false" "no-nans-fp-math"="false" "no-signed-zeros-fp-math"="false" "no-trapping-math"="false" "stack-protector-buffer-size"="8" "target-cpu"="generic" "target-features"="+neon" "unsafe-fp-math"="false" "use-soft-float"="false" }
+
+!llvm.module.flags = !{!0}
+!llvm.ident = !{!1}
+
+!0 = !{i32 1, !"wchar_size", i32 4}
+!1 = !{!"clang version 9.0.0 (https://git.llvm.org/git/clang.git/ 112a7a3c34df3dc506b52cd89e155287cdbcea55) (https://git.llvm.org/git/llvm.git/ feeb3cebe7a586a8e1b12735024389dd8d3bab53)"}
+!2 = !{!3, !3, i64 0}
+!3 = !{!"int", !4, i64 0}
+!4 = !{!"omnipotent char", !5, i64 0}
+!5 = !{!"Simple C/C++ TBAA"}