[PATCH] D42713: LTO: Include dso-local bit in ThinLTO cache key.

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson accepted this revision.
tejohnson added a comment.
This revision is now accepted and ready to land.

LGTM



================
Comment at: llvm/lib/LTO/LTO.cpp:181
+    for (const ValueInfo &VI : GS->refs()) {
+      AddUnsigned(VI.isDSOLocal());
       AddUsedCfiGlobal(VI.getGUID());
----------------
pcc wrote:
> tejohnson wrote:
> > pcc wrote:
> > > tejohnson wrote:
> > > > Is it necessary to do this for all refs, rather than just for each DefinedGlobals summary (like we do for the linkage below)?
> > > Yes because dso-local generally affects the code generated for references to globals.
> > Perhaps, but in FunctionImportGlobalProcessing::processGlobalForThinLTO we apply this flag to every GV defined in the module. Even if that generally only affects references to those GVs, it seems incomplete not to hash it for all GVs that might have the flag set in the IR. By updating the hash only for references, we will update the hash multiple times for some GVs (those that are in multiple ref lists) and not at all for others (those that don't have any references from within this module). It seems like it would be sufficient and more complete to update the hash for all GVs in the DefinedGlobals set.
> But if I understand correctly, globals that are only referenced and not defined by a module will not appear in DefinedGlobals, so the hash would not take into account the dso-local bit for such globals.
> 
> If a GV declaration is not referenced from a module (say it is an unreferenced declaration that for some reason wasn't gc'ed) then it wouldn't seem to matter what its dso-local bit is because no code would be generated based on it.
> But if I understand correctly, globals that are only referenced and not defined by a module will not appear in DefinedGlobals, so the hash would not take into account the dso-local bit for such globals.

Ok, I was thinking about the distributed backend case (where we're not even invoking this caching code), where we only have summaries for things defined in this module or being imported into it, so we wouldn't be able to update the DSOLocal bit in the backend on GVs for refs we don't have the definition for. (Hmm, does that mean we need to enhance the distributed build case to get the full benefit of the DSOLocal optimization? I.e. include summaries for things we reference but aren't importing, and somehow mark them to not be imported, so that we can update their DSO local flag.)

> If a GV declaration is not referenced from a module (say it is an unreferenced declaration that for some reason wasn't gc'ed) then it wouldn't seem to matter what its dso-local bit is because no code would be generated based on it.

Again confused myself thinking about the distributed case, here we have the whole index. Nevermind...unless the DSO local flag will in the future ever affect something other than the code gen for references.


https://reviews.llvm.org/D42713