This is an archive of the discontinued LLVM Phabricator instance.

ThinLTO: Do not take into account whether a definition has multiple copies when promoting.
ClosedPublic

Authored by pcc on Jun 29 2016, 7:00 PM.

Details

Summary

We currently do not touch a symbol's linkage in the case where a definition
has a single copy. However, this code is effectively unnecessary: either
the definition is not exported, in which case the internalize phase sets
its linkage to internal, or it is exported, in which case we need to promote
linkage to weak. Those two cases are already handled by existing code.

I believe that the only real functional change here is in the case where we
have a single definition which does not prevail (e.g. because the definition
in a native object file prevails). In that case we now lower linkage to
available_externally following the existing code path for that case.

As a result we can remove the isExported function parameter from the
thinLTOResolveWeakForLinkerInIndex function.

Diff Detail

Event Timeline

pcc updated this revision to Diff 62327.Jun 29 2016, 7:00 PM
pcc retitled this revision from to ThinLTO: Do not take into account whether a definition has multiple copies when promoting..
pcc updated this object.
pcc added reviewers: tejohnson, mehdi_amini.
pcc added a subscriber: llvm-commits.
tejohnson accepted this revision.Jun 29 2016, 9:28 PM
tejohnson edited edge metadata.

This looks like a nice simplification. I'm guessing the special handling for the single copy case was because it predated ThinLTO internalization, but Mehdi can probably say for sure.

Do we have a test case for the single copy non-exported case to ensure that we end up with the same internalized linkage with this change? If not, please add one.

This revision is now accepted and ready to land.Jun 29 2016, 9:28 PM
mehdi_amini edited edge metadata.Jun 29 2016, 9:35 PM

"Either the definition is not exported, in which case the internalize phase sets its linkage to internal, or it is exported, in which case we need to promote linkage to weak"

We intentionally don't promote linkonce to weak when possible because linkonce -> weak promotion inhibit auto-hidden on MachO.
I should look at it again with the recent local_unnamed_addr attribute to understand exactly where we stand now and how this could affect our codegen.

pcc added a comment.Jun 30 2016, 12:12 PM

We intentionally don't promote linkonce to weak when possible because linkonce -> weak promotion inhibit auto-hidden on MachO.
I should look at it again with the recent local_unnamed_addr attribute to understand exactly where we stand now and how this could affect our codegen.

Sorry if I wasn't clear enough, but I believe this to be NFC in the case where the client applies both promotion and internalization to the index. I believe that in every case where I changed linkonce to weak in a test case, the internalize phase would change linkage to internal. We were already upgrading exported symbols from linkonce to weak, see the code I deleted in thinLTOResolveWeakForLinkerGUID. If we do want to apply auto-hidden to these decls, we should probably think about that separately.

(IMHO, the fact that we apply promotion and internalization to the index separately is very confusing, and we should fix that at some point.)

pcc updated this revision to Diff 62450.Jun 30 2016, 5:34 PM
pcc edited edge metadata.

Refresh

pcc added a comment.Jun 30 2016, 5:35 PM

Do we have a test case for the single copy non-exported case to ensure that we end up with the same internalized linkage with this change? If not, please add one.

Please see D21915.

In D21883#471613, @pcc wrote:

We intentionally don't promote linkonce to weak when possible because linkonce -> weak promotion inhibit auto-hidden on MachO.
I should look at it again with the recent local_unnamed_addr attribute to understand exactly where we stand now and how this could affect our codegen.

Sorry if I wasn't clear enough, but I believe this to be NFC in the case where the client applies both promotion and internalization to the index. I believe that in every case where I changed linkonce to weak in a test case, the internalize phase would change linkage to internal. We were already upgrading exported symbols from linkonce to weak, see the code I deleted in thinLTOResolveWeakForLinkerGUID. If we do want to apply auto-hidden to these decls, we should probably think about that separately.

(IMHO, the fact that we apply promotion and internalization to the index separately is very confusing, and we should fix that at some point.)

The test cases I added in D21915 should help show that this is NFC.

In D21883#471613, @pcc wrote:

(IMHO, the fact that we apply promotion and internalization to the index separately is very confusing, and we should fix that at some point.)

Yep, it is something we have talked about. See also the FIXME in llvm::thinLTOInternalizeModule:

// FIXME: Eventually we should control promotion instead of promoting
// and internalizing again.
mehdi_amini accepted this revision.Jul 7 2016, 10:54 AM
mehdi_amini edited edge metadata.

LGTM, thanks.
(sorry for the delay, I'm catching up with two weeks vacations)

This revision was automatically updated to reflect the committed changes.