This is an archive of the discontinued LLVM Phabricator instance.

LTO: Link non-prevailing weak_odr, linkonce_odr or available_externally globals into the combined module with available_externally linkage.
ClosedPublic

Authored by pcc on Jan 31 2017, 7:48 PM.

Details

Summary

These linkages mean that the ultimately prevailing symbol will have the same
semantics as any non-prevailing copy of the symbol, so we are free to ignore
the linker's resolution.

Event Timeline

pcc created this revision.Jan 31 2017, 7:48 PM
tejohnson added inline comments.
llvm/lib/LTO/LTO.cpp
478

Will the IRLinker not replace the existing available_externally definition with the prevailing copy since it is added above to Keep (ValuesToLink)?

484

CombinedGO should never have a Comdat at this point, since it was available_externally.

pcc updated this revision to Diff 86568.Jan 31 2017, 8:50 PM
  • Simplify
llvm/lib/LTO/LTO.cpp
478

Yes it will, removed this code.

484

Now moot

pcc updated this revision to Diff 86569.Jan 31 2017, 8:56 PM
  • Add one more test
tejohnson added inline comments.Feb 1 2017, 7:02 AM
llvm/lib/LTO/LTO.cpp
484

I don't think adding to the Keep is technically necessary because of http://llvm-cs.pcc.me.uk/lib/Linker/IRMover.cpp#873 (we always link in available_externally copies in the IRLinker). That's presumably why inglorion's D29366 worked which didn't do this, although I had to look it up because I was initially confused as to why it worked without adding to the Keep! IMO it is clearer to have the explicit add to Keep here and not rely on the IRLinker behavior - but in either case perhaps leave a comment (particularly if you end up removing it). If you end up removing it, then incoming available_externally should be removed.

llvm/test/LTO/Resolution/X86/link-availextern.ll
8

Minor quibble with the names SINGLE and BOTH which seem a little confusing (e.g. we have "both" inputs here). Maybe NONPREVAILING and HAVEPREVAILING or something like that would be more descriptive?

pcc updated this revision to Diff 86677.Feb 1 2017, 11:06 AM
  • Address review comments
llvm/lib/LTO/LTO.cpp
484

I was also surprised to see that code in IRMover that always links available_externally, but I suspect that it may be the right place for it (linker-independent "optimisations" based on linkage).

If we accept that to be the correct place for it, that raises the question of whether this code itself should live in IRMover. I'm not sure, so I left a FIXME.

I went ahead and changed this code to set linkage to available_externally unconditionally for non-prevailing. That revealed a bug of sorts in IRMover in that it would repeatedly link available_externally definitions from source modules if they were available, instead of being satisfied with the first one. I also fixed that in this patch. A separate test failure revealed that we need to inhibit this for globals referenced by aliases, which I've also done.

pcc updated this revision to Diff 86678.Feb 1 2017, 11:08 AM
pcc marked an inline comment as done.
  • Rename checks in test case
tejohnson accepted this revision.Feb 1 2017, 11:31 AM

LGTM, but one suggestion on the IRMover change and a question about your FIXME.

llvm/lib/LTO/LTO.cpp
484

A separate test failure revealed that we need to inhibit this for globals referenced by aliases, which I've also done.

Ah - I should have noticed that, we have to do the same thing for ThinLTO (thinLTOResolveWeakForLinkerGUID).

If we accept that to be the correct place for it, that raises the question of whether this code itself should live in IRMover. I'm not sure, so I left a FIXME.

Do we have the linker resolution available there? Presumably we'd have to wire it in, but it seems like this might be better suited to staying in LTO.cpp which is designed to handle linker resolution info.

llvm/lib/Linker/IRMover.cpp
873 ↗(On Diff #86678)

The new check is almost identical to the one a couple lines up. So we should never hit this when DGV && !DGV->isDeclaration() unless it is available_externally - maybe add a comment to that effect?

This revision is now accepted and ready to land.Feb 1 2017, 11:31 AM
mehdi_amini added inline comments.Feb 1 2017, 11:44 AM
llvm/lib/LTO/LTO.cpp
484

I was also surprised to see that code in IRMover that always links available_externally,

I personally *hate* this kind of behavior that exhibit magic hidden from the API. This makes the software component harder to user and reason about, and invariant unclear.

I mentioned it multiple times in the case of the IRMover during ThinLTO bring-up that I'd even consider this a layer violation in term of responsibility of the IRMover (which should be moving the IR we tell it to and nothing else, *deciding* what to move being the other layer.).

pcc marked an inline comment as done.Feb 1 2017, 5:36 PM
pcc added inline comments.
llvm/lib/LTO/LTO.cpp
484

Do we have the linker resolution available there? Presumably we'd have to wire it in, but it seems like this might be better suited to staying in LTO.cpp which is designed to handle linker resolution info.

Yes, we have the set ValuesToLink. I was imagining that IRLinker could link *_odr globals and change their linkage to available_externally if they were not members of ValuesToLink, similar to how it is already handling available_externally globals.

484

Fair point, it sounds like the consensus is in favour of doing everything explicitly in the LTO layer then.

I sent out D29435 which moves the available_externally special casing to clients and rebased this change on top of it.

tejohnson added inline comments.Feb 1 2017, 6:58 PM
llvm/lib/LTO/LTO.cpp
484

Yeah, making it explicit rather that relying on more IRLinker magic sounds better.

pcc updated this revision to Diff 86764.Feb 1 2017, 9:07 PM
pcc marked an inline comment as done.
  • Simplify test
This revision was automatically updated to reflect the committed changes.