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

[Peter Collingbourne via Phabricator via llvm-commits]

pcc added inline comments.


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


https://reviews.llvm.org/D29367