This is an archive of the discontinued LLVM Phabricator instance.

[ARM] Make GlobalMerge merge extern globals by default
ClosedPublic

Authored by john.brawn on Jul 6 2015, 9:41 AM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

john.brawn updated this revision to Diff 29100.Jul 6 2015, 9:41 AM
john.brawn retitled this revision from to [ARM] Make GlobalMerge merge extern globals by default.
john.brawn updated this object.
john.brawn added reviewers: ab, echristo.
john.brawn set the repository for this revision to rL LLVM.
john.brawn added a subscriber: llvm-commits.
ab requested changes to this revision.Jul 6 2015, 11:50 AM
ab edited edge metadata.

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
353

Even with the comment, can you add:

/*MergeExternalByDefault=*/true
This revision now requires changes to proceed.Jul 6 2015, 11:50 AM
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:

john.brawn edited edge metadata.
john.brawn removed rL LLVM as the repository for this revision.

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 marked an inline comment as done.Jul 30 2015, 10:27 AM

Ping.

ab added a comment.Jul 30 2015, 11:36 AM
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!

In D10966#215452, @ab 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 updated this revision to Diff 31115.Jul 31 2015, 4:28 AM
john.brawn edited edge metadata.
john.brawn set the repository for this revision to rL LLVM.

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

ab accepted this revision.Jul 31 2015, 11:22 AM
ab edited edge metadata.

LGTM, thanks!

This revision is now accepted and ready to land.Jul 31 2015, 11:22 AM
This revision was automatically updated to reflect the committed changes.