[PATCH] D29468: FunctionImport: Use IRMover directly. NFCI.

[Peter Collingbourne via Phabricator via llvm-commits]

pcc added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/FunctionImport.cpp:724
     for (GlobalAlias &GA : SrcModule->aliases()) {
+      if (FunctionImportGlobalProcessing::doImportAsDefinition(
+              &GA, &GlobalsToImport)) {
----------------
tejohnson wrote:
> I guess you have to do this to make it NFC, since it looks like we were doing this in the ModuleLinker.
> Interesting...I didn't realize or had forgotten that the ModuleLinker would do this - so essentially we always import an alias if its aliasee is linkonce_odr and marked for import? That seems like something we want to get rid of (can be in a follow-up change so this remains NFC). We've tried to get to the point where we only import values decided on by the thin link. This is presumably a holdover from the old scheme of always importing linkonce_odr references...
Yes, this was needed in order to be NFC. I've added a FIXME to remove.


================
Comment at: llvm/lib/Transforms/Utils/FunctionImportUtils.cpp:28
   if (auto *GA = dyn_cast<GlobalAlias>(SGV)) {
-    if (GA->hasWeakAnyLinkage())
+    if (GA->isInterposable())
       return false;
----------------
tejohnson wrote:
> This looks like a bug fix?
Yes, and also needed in order to be NFC. Without this we would import `@linkonceODRfuncLinkonceAlias` in `test/ThinLTO/X86/alias_import.ll` because the logic for whether to put globals in `GlobalsToImport` differed from that in `FunctionImportGlobalProcessing::doImportAsDefinition` and we would normally only import globals that satisfied both (this is because we were querying the latter on LinkModules.cpp:394 in the old code and querying the former on line 277).

But naturally that raises the question of why the change on line FunctionImport.cpp:724 is needed at all. The answer is that we only call `shouldLinkFromSource` if the destination defines a GV with the same name, so if the destination did not define the GV we would only test `FunctionImportGlobalProcessing::doImportAsDefinition`.

In short: ModuleLinker is complicated, and it's definitely a Good Thing that we're moving away from it.


https://reviews.llvm.org/D29468