[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());
----------------
pcc wrote:
> 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(...);
> ```
You're right, forgot we don't have that flag handy later. Maybe ok for now then, but we may want to reevaluate at some point - I've seen the index lookups get hot before for really large apps. I guess we could stash in the PrevailingModuleForGUID map (make the result a tuple), and then change isPrevailing to return this flag. But perhaps not critical for now.

If it is left as is here, then in thinLTOResolveWeakForLinkerModule it should probably be documented where the new linkage is set up in the summary (since not in the corresponding "InIndex" version of that routine). 


https://reviews.llvm.org/D35064