[PATCH] D35064: [lib/LTO] Fix the interaction between linker redefined symbols and ThinLTO

[Davide Italiano via Phabricator via llvm-commits]

davide added inline comments.


================
Comment at: lib/LTO/LTO.cpp:673
+        // linkage so that we can switch it when we import the GV.
+        auto S = ThinLTO.CombinedIndex.findSummaryInModule(
+            GUID, BM.getModuleIdentifier());
----------------
tejohnson wrote:
> Suggest doing this in thinLTOResolveWeakForLinkerGUID/thinLTOResolveWeakForLinkerInIndex for 2 reasons:
> 1) avoids extra combined index hash lookup to find the summary
> 2) since we are doing linkage update in the corresponding LTO backend routine hinLTOResolveWeakForLinkerModule
> 
> The names of all 3 of these routines should probably be changed to reflect the additional scope (not just handling linker resolution of weak symbols anymore). Not sure of a good name offhand though...
In order to find which symbols are `LinkerRedefined` we need to access the resolution vector, so if we move this logic to `thinLTOResolveWeakForLinkerGUID` we need to add an extra symbol walk for all the input files, I guess, which I'm not entirely sure it will be faster than having extra summary lookups.

I'll leave the call to you, but this is something I wanted to point out anyway :)


================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:555
+
+    // Handle switch to available_externally.
+    if (!GlobalValue::isWeakForLinker(GV.getLinkage()))
----------------
tejohnson wrote:
> Actually we may be switching to either available_externally (for the non-prevailing copies) or converting linkonce->weak for the prevailing copy (sorry, my earlier comment was misleading). So probably change comment to reflect that we are doing weak for linker resolution below.
I'll remove this.


https://reviews.llvm.org/D35064