[ARM] Make GlobalMerge merge extern globals by default

[john.brawn]

Summary

Enabling merging of extern globals appears to be generally either beneficial or harmless. On some benchmarks suites (on Cortex-M4F, Cortex-A9, and Cortex-A57) it gives improvements in the 1-5% range, but in the rest the overall effect is zero.

[ab]

So, if this is positive performance-wise, I'm fine with it, but I don't think you can enable it on all platforms (at least not without testing).

For instance, this won't work on Mach-O, most importantly because aliases are just regular symbols, and symbols are (usually) assumed atomic; __MergedGlobals wouldn't be in this case.

On a related note, on targets where aliases aren't an issue, I think we should use them all the time, even when merging internal globals. It lets us

• get rid of cryptic _MergedGlobals symbols
• use the source symbols instead
• simplify some DebugInfo weirdness (you won't need a 'plus' expression if you can just reference a symbol).

That's where I discovered the Mach-O symbol alias issue!

lib/Target/ARM/ARMTargetMachine.cpp

354 ↗

(On Diff #29100)

Even with the comment, can you add:

/*MergeExternalByDefault=*/true

[john.brawn]

In D10966#199852, @ab wrote:

For instance, this won't work on Mach-O, most importantly because aliases are just regular symbols, and symbols are (usually) assumed atomic; __MergedGlobals wouldn't be in this case.

Do you know where I could find out more about this? Looking at the Apple Mach-O documentation (https://developer.apple.com/library/mac/documentation/DeveloperTools/Conceptual/MachORuntime/index.html) I don't see anything like this. What exactly do you mean by 'atomic' here?

Looking at the Mach-O object output of global-merge-external.ll I do see a couple of oddities (based on my understanding of the above document), but they appear to happen whether extern global merging is enabled or not:

• Both zero-initialized variables and a zero-initialized MergedGlobals go in a section named __common, but I would expect them to go in a section named __data: https://developer.apple.com/library/mac/documentation/DeveloperTools/Reference/Assembler/040-Assembler_Directives/asm_directives.html#//apple_ref/doc/uid/TP30000823-CJBDFFDF says of __data "The compiler places all non-const initialized data (even initialized to zero) in this section".
• Global variables and global aliases end up with symbol flag REFERENCE_FLAG_UNDEFINED_NON_LAZY but I would expect REFERENCE_FLAG_DEFINED.

[john.brawn]

Ping.

[john.brawn]

New patch makes the suggested change. I've not yet added anything to disable it on Mach-O because it would be nice to know first why exactly it wouldn't work on Mach-O.

[john.brawn]

Ping.

[ab]

In D10966#200255, @john.brawn wrote:

In D10966#199852, @ab wrote:

For instance, this won't work on Mach-O, most importantly because aliases are just regular symbols, and symbols are (usually) assumed atomic; __MergedGlobals wouldn't be in this case.

Do you know where I could find out more about this? Looking at the Apple Mach-O documentation (https://developer.apple.com/library/mac/documentation/DeveloperTools/Conceptual/MachORuntime/index.html) I don't see anything like this.
What exactly do you mean by 'atomic' here?

With the ".subsections_via_symbols" directive, each section is split up into "subsections", each one associated with a single symbol. The linker is free to strip or reorder these subsections. Since aliases are just regular symbols, aliases into _MergedGlobals could potentially cause it to be broken apart in the final binary, thus making the offset-addressing code invalid (since the code should only reference the first symbol, the rest might get dead-stripped).

Hope that helps, sorry for the delay!

[john.brawn]

In D10966#215452, @ab wrote:

In D10966#200255, @john.brawn wrote:

In D10966#199852, @ab wrote:

For instance, this won't work on Mach-O, most importantly because aliases are just regular symbols, and symbols are (usually) assumed atomic; __MergedGlobals wouldn't be in this case.

Do you know where I could find out more about this? Looking at the Apple Mach-O documentation (https://developer.apple.com/library/mac/documentation/DeveloperTools/Conceptual/MachORuntime/index.html) I don't see anything like this.
What exactly do you mean by 'atomic' here?

With the ".subsections_via_symbols" directive, each section is split up into "subsections", each one associated with a single symbol. The linker is free to strip or reorder these subsections. Since aliases are just regular symbols, aliases into _MergedGlobals could potentially cause it to be broken apart in the final binary, thus making the offset-addressing code invalid (since the code should only reference the first symbol, the rest might get dead-stripped).

Hope that helps, sorry for the delay!

Ah, I see, and that directive gets emitted by ARMAsmPrinter::EmitEndOfAsmFile. I'll upload a new patch that disables merging of globals on Mach-O.

[john.brawn]

New patch that disables merging of extern globals on Mach-O.

[ab]

LGTM, thanks!

llvm/trunk/lib/Target/ARM/ARMTargetMachine.cpp

if ((TM->getOptLevel() != CodeGenOpt::None &&
       EnableGlobalMerge == cl::BOU_TRUE) {
     // FIXME: This is using the thumb1 only constant value for
     // maximal global offset for merging globals. We may want
     // to look into using the old value for non-thumb1 code of
     // 4095 based on the TargetMachine, but this starts to become
     // tricky when doing code gen per function.
     bool OnlyOptimizeForSize = (TM->getOptLevel() < CodeGenOpt::Aggressive) &&
                                (EnableGlobalMerge == cl::BOU_UNSET);
-    addPass(createGlobalMergePass(TM, 127, 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, 127, OnlyOptimizeForSize,
+                                  MergeExternalByDefault));
   }
 
   return false;
 }
 
 bool ARMPassConfig::addInstSelector() {
   addPass(createARMISelDag(getARMTargetMachine(), getOptLevel()));
 

llvm/trunk/test/CodeGen/ARM/global-merge-external.ll

+; RUN: llc < %s -mtriple=arm-eabi  -arm-global-merge                                 | FileCheck %s --check-prefix=CHECK-MERGE
+; RUN: llc < %s -mtriple=arm-eabi  -arm-global-merge -global-merge-on-external=true  | FileCheck %s --check-prefix=CHECK-MERGE
+; RUN: llc < %s -mtriple=arm-eabi  -arm-global-merge -global-merge-on-external=false | FileCheck %s --check-prefix=CHECK-NO-MERGE
+; RUN: llc < %s -mtriple=arm-macho -arm-global-merge                                 | FileCheck %s --check-prefix=CHECK-NO-MERGE
+
+@x = global i32 0, align 4
+@y = global i32 0, align 4
+@z = global i32 0, align 4
+
+define void @f1(i32 %a1, i32 %a2) {
+;CHECK:          f1:
+;CHECK:          ldr {{r[0-9]+}}, [[LABEL1:\.LCPI[0-9]+_[0-9]]]
+;CHECK:          [[LABEL1]]:
+;CHECK-MERGE:    .long _MergedGlobals_x
+;CHECK-NO-MERGE: .long {{_?x}}
+  store i32 %a1, i32* @x, align 4
+  store i32 %a2, i32* @y, align 4
+  ret void
+}
+
+define void @g1(i32 %a1, i32 %a2) {
+;CHECK:          g1:
+;CHECK:          ldr {{r[0-9]+}}, [[LABEL2:\.LCPI[0-9]+_[0-9]]]
+;CHECK:          [[LABEL2]]:
+;CHECK-MERGE:    .long _MergedGlobals_x
+;CHECK-NO-MERGE: .long {{_?y}}
+  store i32 %a1, i32* @y, align 4
+  store i32 %a2, i32* @z, align 4
+  ret void
+}
+
+;CHECK-NO-MERGE-NOT: .globl _MergedGlobals_x
+
+;CHECK-MERGE: .type _MergedGlobals_x,%object
+;CHECK-MERGE: .globl  _MergedGlobals_x
+;CHECK-MERGE: .align  2
+;CHECK-MERGE: _MergedGlobals_x:
+;CHECK-MERGE: .size _MergedGlobals_x, 12
+
+;CHECK-MERGE: .globl  x
+;CHECK-MERGE: x = _MergedGlobals_x
+;CHECK-MERGE: .size x, 4
+;CHECK-MERGE: .globl  y
+;CHECK-MERGE: y = _MergedGlobals_x+4
+;CHECK-MERGE: .size y, 4
+;CHECK-MERGE: .globl  z
+;CHECK-MERGE: z = _MergedGlobals_x+8
+;CHECK-MERGE: .size z, 4