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

[Peter Collingbourne via Phabricator via llvm-commits]

pcc 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());
----------------
davide wrote:
> 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 :)
You could also reduce the number of summary lookups by changing the code to look like this:
```if (Res.LinkerRedefined)
  if (auto S = ...)
    S->setLinkage(...);
```


https://reviews.llvm.org/D35064