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.

lib/CodeGen/GlobalMerge.cpp

bool GlobalMerge::doInitialization(Module &M) {
   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())
       continue;
 
     PointerType *PT = dyn_cast<PointerType>(GV.getType());
     assert(PT && "Global variable is not a pointer!");
 
     unsigned AddressSpace = PT->getAddressSpace();

lib/Target/AArch64/AArch64TargetMachine.cpp

bool AArch64PassConfig::addPreISel() {
   // FIXME: On AArch64, this depends on the type.
   // Basically, the addressable offsets are up to 4095 * Ty.getSizeInBytes().
   // and the offset has to be a multiple of the related size in bytes.
   if ((TM->getOptLevel() != CodeGenOpt::None &&
        EnableGlobalMerge == cl::BOU_UNSET) ||
       EnableGlobalMerge == cl::BOU_TRUE) {
     bool OnlyOptimizeForSize = (TM->getOptLevel() < CodeGenOpt::Aggressive) &&
                                (EnableGlobalMerge == cl::BOU_UNSET);
-    addPass(createGlobalMergePass(TM, 4095, OnlyOptimizeForSize));
+    
+    // Merging of extern globals is enabled by default on non-Mach-O as we
+    // expect it to be generally either beneficial or harmless. On Mach-O it
+    // is disabled as we emit the .subsections_via_symbols directive which
+    // means that merging extern globals is not safe.
+    bool MergeExternalByDefault = !TM->getTargetTriple().isOSBinFormatMachO();
+    addPass(createGlobalMergePass(TM, 4095, OnlyOptimizeForSize, 
+                                  MergeExternalByDefault));
   }
 
   return false;
 }
 
 bool AArch64PassConfig::addInstSelector() {
   addPass(createAArch64ISelDag(getAArch64TargetMachine(), getOptLevel()));
 

test/CodeGen/AArch64/global_merge_aarc64_ac6.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 = dso_local local_unnamed_addr global i32 0, align 4
+@global1 = dso_local local_unnamed_addr global i32 0, align 4
+
+; Function Attrs: minsize norecurse nounwind optsize readonly
+
+;CHECK-LABEL: @func
+;CHECK: adrp x8, .L_MergedGlobals 
+;CHECK: add x8, x8, :lo12:.L_MergedGlobals
+;CHECK-NEXT: ldp
+;CHECK-NEXT: add
+;CHECK-NEXT: ret
+define dso_local i32 @func() local_unnamed_addr #0 {
+entry:
+  %0 = load i32, i32* @global0, align 4, !tbaa !2
+  %1 = load i32, i32* @global1, align 4, !tbaa !2
+  %add = add nsw i32 %1, %0
+  ret i32 %add
+}
+
+attributes #0 = { minsize norecurse nounwind optsize 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"="cortex-a53" "target-features"="+aes,+crc,+crypto,+fp-armv8,+neon,+sha2" "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/ 7a6d690bb52df60e979a7c7f9d03a774009bb443) (https://git.llvm.org/git/llvm.git/ 71abe189dddb95b0d5782e0866058b9a92ff2402)"}
+!2 = !{!3, !3, i64 0}
+!3 = !{!"int", !4, i64 0}
+!4 = !{!"omnipotent char", !5, i64 0}
+!5 = !{!"Simple C/C++ TBAA"}