This is an archive of the discontinued LLVM Phabricator instance.

[ThinLTO/gold] Perform index-based weak/linkonce resolution

Authored by tejohnson on Jul 13 2016, 6:29 AM.



Invoke the weak/linkonce symbol resolution support (already used by
libLTO) that operates via the summary index.

This ensures prevailing linkonce are kept, by making them weak, and
marks preempted copies as available_externally when possible.

With this change, the older support for keeping the prevailing linkonce
(by changing their symbol resolution) is removed.

Diff Detail


Event Timeline

tejohnson updated this revision to Diff 63800.Jul 13 2016, 6:29 AM
tejohnson retitled this revision from to [ThinLTO/gold] Perform index-based weak/linkonce resolution.
tejohnson updated this object.
tejohnson added a reviewer: mehdi_amini.
tejohnson added a subscriber: llvm-commits.
tejohnson updated this revision to Diff 63813.Jul 13 2016, 9:10 AM

Add back a missing piece that I accidentally removed during patch
cleanup: add save-temps dumping of module just after index-based
optimizations and importing, but before the rest of the optimizations.

mehdi_amini accepted this revision.Jul 14 2016, 12:28 PM
mehdi_amini edited edge metadata.

LGTM, with some comments.

1023 ↗(On Diff #63813)

Is this related to the rest of this patch? How was it handled before?

1364 ↗(On Diff #63813)

I'm not sure what is this doing, the comment said "identify symbols referenced by more than a single IR module".

After checking ; it is Preserving any symbols that is not *exclusively referenced from an IR file* (i.e. the comment above the loop is incomplete: it will "identify symbols that can needs to be preserved outside of LTO, i.e. symbols that LTO can't eliminate under any condition".

1370 ↗(On Diff #63813)

GlobalValue::getGUID isn't free, you may want to CSE on these two lines.

This revision is now accepted and ready to land.Jul 14 2016, 12:28 PM
Prazek added a subscriber: Prazek.Jul 14 2016, 12:58 PM
Prazek added inline comments.
1018–1019 ↗(On Diff #63813)

I think it would be good to put this comment in an assert.

Thanks for the comments, will address as noted below before submitting.

1018–1019 ↗(On Diff #63813)

Ok, will do when I add this later in a separate patch.

1023 ↗(On Diff #63813)

It is related in the sense that the new test uses the .import.bc file to do some checking (before the optimization pipeline kicks in and does inlining, e.g.). But actually, I just changed the test to check the .opt.bc saved temps files and it still works - looks like the linkonce_odr removal doesn't happen until code gen. I will remove this new block (which is also useful for debugging, but not then related to this patch, I can submit separately) and change the test.

1364 ↗(On Diff #63813)

Right, I was trying to include both scenarios where we need to preserve this in one comment. I.e. if is is not simply referenced by a single IR modue (referenced from other IR modules or from a non-IR file) then it will not have LDPR_PREVAILING_DEF_IRONLY somewhere and will end up in the Preserve set. I will clarify this in the comment.

1370 ↗(On Diff #63813)

Will do.

mehdi_amini added inline comments.Jul 14 2016, 1:36 PM
1023 ↗(On Diff #63813)

I'm not sure what you mean with linkonce_odr removal does not happen until codegen?
If you turn it into an available_externally, we should always remove them after the inliner is complete.
If you don't turn them into available_externally but they are inlined, the inline is supposed to remove them:

/// Remove now-dead linkonce functions at the end of
/// processing to avoid breaking the SCC traversal.
bool Inliner::doFinalization(CallGraph &CG) {
  return removeDeadFunctions(CG);
tejohnson updated this revision to Diff 64039.Jul 14 2016, 1:47 PM
tejohnson edited edge metadata.
  • Address review comments
This revision was automatically updated to reflect the committed changes.