[PATCH] D54125: [LTO] Drop non-prevailing definitions for non-local linkage types

[Pirama Arumuga Nainar via Phabricator via llvm-commits]

pirama added inline comments.


================
Comment at: lib/LTO/LTO.cpp:294
     GlobalValue::LinkageTypes OriginalLinkage = S->linkage();
-    if (!GlobalValue::isWeakForLinker(OriginalLinkage))
+    if (GlobalValue::isLocalLinkage(OriginalLinkage))
       continue;
----------------
tejohnson wrote:
> You mentioned in the PR that you get test failures if you guard against appending linkage symbols here. I'd like to understand why. With the old code we would not have resolved appending linkage symbols either.
Adding the guard here was fine.  But, per your suggstion, I replaced the guard with the change to ThinLTOCodeGenerator.cpp:computePrevailingCopies that skips adding prevailing symbols for appending-linkage symbols.

I'll leave a comment at the lines that tripped the tests.


================
Comment at: lib/LTO/LTO.cpp:353
     } else if (EnableLTOInternalization &&
                !GlobalValue::isLocalLinkage(S->linkage()))
       S->setLinkage(GlobalValue::InternalLinkage);
----------------
If I skip appending-linkage summaries here, so I can instead get rid of the AppendingLinkage check in FunctionImport.cpp:thinLTOResolveWeakForLinkerModule (line 914), I get some test failures.

My reasoning was that this line sets linkage to Internal for summaries of appending-linkage symbols, which then gets propagated to the actual symbol in FunctionImport.cpp:thinLTOResolveWeakForLinkerModule.  Not setting the linkage here will obviate the need for that check.

I just realized this line is slightly different from my prior test - I must not have synced recently.  I'll recreate this scenario and report on the tests that failed.


================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:915
+    if (GlobalValue::isLocalLinkage(GV.getLinkage()) ||
+        GlobalValue::isAppendingLinkage(GV.getLinkage()) ||
+        // In case it was dead and already converted to declaration.
----------------
This is the check I wanted to eliminate.


Repository:
  rL LLVM

https://reviews.llvm.org/D54125