[PATCH] D29435: Linker: Move special casing for available_externally in IRMover to clients. NFCI.

[Peter Collingbourne via Phabricator via llvm-commits]

pcc added inline comments.


================
Comment at: llvm/lib/LTO/LTO.cpp:481
+          Keep.push_back(GV);
       }
     }
----------------
mehdi_amini wrote:
> Do we have a test that shows that:
> 
> 1) if we first import an available_externally, then a later linkonce_odr (and/or strong) would overwrite?
> 2) if we first import a linkonce_odr (and/or strong), a later available_externally wouldn't overwrite?
I've added tests for both of those things, as well as a standalone test for the available_externally functionality (previously only a gold plugin test was covering this).


================
Comment at: llvm/lib/Linker/IRMover.cpp:875
-    return true;
-
   if (SGV.isDeclaration() || DoneLinkingBodies)
----------------
mehdi_amini wrote:
> No test changed, that's annoying that we're not covering this case (independently of the new LTO API). Any idea how to test this? (along with the removal below as well)
The only direct users of IRMover are LinkModules and LTO. I guess we could write a unit test for IRMover perhaps? Doesn't seem worth it for this one change though.


================
Comment at: llvm/lib/Linker/LinkModules.cpp:398
-             (GV.hasLocalLinkage() || GV.hasLinkOnceLinkage() ||
-              GV.hasAvailableExternallyLinkage()))
     return false;
----------------
tejohnson wrote:
> I guess removing this is required to make it NFC. Is it required for correctness too? I don't think so but want to make sure my understanding is right. Is it better to continue not mapping these in here (I think they get mapped later if referenced, right?) and not bother making this NFC?
Actually this change was needed for us to link available_externally at all (with only the change to shouldLink we would hit the 'return false;' here and never link any available_externally globals). But taking a closer look I found that we were linking too much, even unreferenced available_externally globals (which was a functional change). I've uploaded a new change that causes available_externally to be handled like linkonce and only linked if referenced.


https://reviews.llvm.org/D29435