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

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson 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());
----------------
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...


================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:555
+
+    // Handle switch to available_externally.
+    if (!GlobalValue::isWeakForLinker(GV.getLinkage()))
----------------
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.


https://reviews.llvm.org/D35064