This is an archive of the discontinued LLVM Phabricator instance.

[GlobalMerge] Allow merging globals with explicit section markings.
ClosedPublic

Authored by efriedma on Jul 25 2018, 2:25 PM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

efriedma created this revision.Jul 25 2018, 2:25 PM

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?

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 added inline comments.Jul 27 2018, 2:33 PM
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 updated this revision to Diff 157798.Jul 27 2018, 4:07 PM

Add a big comment explaining the alignment computation.

Not sure who else to add as a reviewer.

dmgreen accepted this revision.Jul 31 2018, 1:07 AM

This looks good to me.

This revision is now accepted and ready to land.Jul 31 2018, 1:07 AM

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.

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.

This revision was automatically updated to reflect the committed changes.