Page MenuHomePhabricator

[ThinLTO] Ensure prevailing linkonce emitted as weak in ThinLTO backends
ClosedPublic

Authored by tejohnson on Jan 13 2016, 8:00 PM.

Details

Summary

Since IR files are all compiled into separate independent object files in ThinLTO mode,
the prevailing linkonce symbols must be emitted in its object file even if it is no longer
referenced there, e.g. if no references remain in the module after inlining, since it may
be referenced by another ThinLTO compiled object file. This is done by changing
LDPR_PREVAILING_DEF_IRONLY* symbols to LDPR_PREVAILING_DEF, which
converts the prevailing linkonce to weak. We also don't need the other prevailing
IRONLY handling for internalization, which is not currently performed for ThinLTO.

Test case included.

Diff Detail

Repository
rL LLVM

Event Timeline

tejohnson updated this revision to Diff 44828.Jan 13 2016, 8:00 PM
tejohnson retitled this revision from to [ThinLTO] Do not mark linkonce symbols as preempted in ThinLTO backends.
tejohnson updated this object.
tejohnson added reviewers: rafael, davidxl.
tejohnson added a subscriber: llvm-commits.

It seems to me that you are changing the ODR resolution compared to a regular FullLTO where the %t2 copy of f() would be selected. I don't know how much it is a problem.

In D16173#338468, @joker.eph wrote:

It seems to me that you are changing the ODR resolution compared to a regular FullLTO where the %t2 copy of f() would be selected. I don't know how much it is a problem.

Although it won't change it compared to a non-LTO link in the same order (where none would be preempted).

Rafael?

Mmmm, not sure how to compare to a non-LTO link: both definition of f would remain and won't disappear.

rafael edited edge metadata.Feb 16 2016, 12:31 PM

Is there any change the file that got the prevailing def could have the linkage changed to weak_odr? That way we would know it would output the symbol, using the same logic that is used in the regular lto case.

Is there any change the file that got the prevailing def could have the linkage changed to weak_odr? That way we would know it would output the symbol, using the same logic that is used in the regular lto case.

Yes, it seems like we could change the handling of LDPR_PREVAILING_DEF_IRONLY (and LDPR_PREVAILING_DEF_IRONLY_EXP I think) to be the same as LDPR_PREVAILING_DEF in the below switch statement. that would change linkonce_any/odr to weak_any/odr and keep the symbol. Let me try that and will update this patch.

tejohnson updated this revision to Diff 48757.Feb 22 2016, 4:55 PM
tejohnson edited edge metadata.

Implement Rafael's suggestion to convert a prevailing def that is
linkonce to weak instead of not marking linkonce as preempted.

Please ignore this latest revision - it pulled in all the differences from the revision on which it was dependent (the gold thinlto threads patch) as well. I'll see if I can fix it, otherwise will kill this one and create a new patch.

tejohnson updated this revision to Diff 48760.Feb 22 2016, 5:11 PM

Implement Rafael's suggestion to convert a prevailing def that is
linkonce to weak instead of not marking linkonce as preempted.

Ok, the diff is fixed now. PTAL. Thanks!

The description is now out of sync.

rafael accepted this revision.Feb 24 2016, 3:07 PM
rafael edited edge metadata.

Just before the code you changed there is an area were we patch gold's decision. Would it work to just add

if (options::thinlto && (Resolution == LDPR_PREVAILING_DEF_IRONLY_EXP || Resolution == LDPR_PREVAILING_DEF_IRONLY))

Resolution =  LDPR_PREVAILING_DEF;

If so, I think that is better.

LGTM either way.

test/tools/gold/X86/thinlto_linkonceresolution.ll
14 ↗(On Diff #48760)

if you pass save-temps you should be able to more directly test the linkage change, no?

This revision is now accepted and ready to land.Feb 24 2016, 3:07 PM

Just before the code you changed there is an area were we patch gold's decision. Would it work to just add

if (options::thinlto && (Resolution == LDPR_PREVAILING_DEF_IRONLY_EXP || Resolution == LDPR_PREVAILING_DEF_IRONLY))

Resolution =  LDPR_PREVAILING_DEF;

If so, I think that is better.

LGTM either way.

I like your suggestion, it is cleaner. Implemented and will upload new patch here. Will also update description in Phab and in git before I commit. Note that this is dependent on the gold threading patch D15390 (for the test case to work), so I will wait to commit this until after that goes in (hopefully very very soon! =) ).

test/tools/gold/X86/thinlto_linkonceresolution.ll
14 ↗(On Diff #48760)

Good idea, adding that.

tejohnson updated this revision to Diff 49080.Feb 25 2016, 9:21 AM
tejohnson retitled this revision from [ThinLTO] Do not mark linkonce symbols as preempted in ThinLTO backends to [ThinLTO] Ensure prevailing linkonce emitted as weak in ThinLTO backends.
tejohnson updated this object.
tejohnson edited edge metadata.

Implement Rafael's suggestion to change resolution for all ThinLTO
prevailing copies to LDPR_PREVAILING_DEF, and modify test to check
linkage type directly with save-temps.

Update description/title.

This revision was automatically updated to reflect the committed changes.