[PATCH] D29367: LTO: Link non-prevailing weak_odr, linkonce_odr or available_externally globals into the combined module with available_externally linkage.

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson accepted this revision.
tejohnson added a comment.
This revision is now accepted and ready to land.

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



================
Comment at: llvm/lib/LTO/LTO.cpp:484
+        if (!CombinedGV || CombinedGV->isDeclaration()) {
+          Keep.push_back(GV);
+          GV->setLinkage(GlobalValue::AvailableExternallyLinkage);
----------------
pcc wrote:
> tejohnson wrote:
> > 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.
> 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.
> 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.


================
Comment at: llvm/lib/Linker/IRMover.cpp:873
 
-  if (SGV.hasAvailableExternallyLinkage())
+  if (SGV.hasAvailableExternallyLinkage() && (!DGV || DGV->isDeclaration()))
     return true;
----------------
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?


https://reviews.llvm.org/D29367