LTO: Include dso-local bit in ThinLTO cache key.

[tejohnson]

llvm/lib/LTO/LTO.cpp

181 ↗

(On Diff #132061)

Is it necessary to do this for all refs, rather than just for each DefinedGlobals summary (like we do for the linkage below)?

llvm/test/LTO/Resolution/X86/cache-dso-local.ll

9 ↗

(On Diff #132061)

comment please

[pcc]

• Address review comment

[pcc]

llvm/lib/LTO/LTO.cpp

181 ↗

(On Diff #132061)

Yes because dso-local generally affects the code generated for references to globals.

[tejohnson]

llvm/lib/LTO/LTO.cpp

181 ↗

(On Diff #132061)

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.

[pcc]

llvm/lib/LTO/LTO.cpp

181 ↗

(On Diff #132061)

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.

[tejohnson]

LGTM

llvm/lib/LTO/LTO.cpp

181 ↗

(On Diff #132061)

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.

llvm/trunk/include/llvm/IR/ModuleSummaryIndex.h

return isFromAnalysis() ? getRef()->second.U.GV->getName()
                             : getRef()->second.U.Name;
   }
 
   bool isFromAnalysis() const { return RefAndFlag.getInt(); }
 
   const GlobalValueSummaryMapTy::value_type *getRef() const {
     return RefAndFlag.getPointer();
   }
+
+  bool isDSOLocal() const;
 };
 
 template <> struct DenseMapInfo<ValueInfo> {
   static inline ValueInfo getEmptyKey() {
     return ValueInfo(false, (GlobalValueSummaryMapTy::value_type *)-8);
   }
 
   static inline ValueInfo getTombstoneKey() {

llvm/trunk/lib/IR/ModuleSummaryIndex.cpp

 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/IR/ModuleSummaryIndex.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/Support/Path.h"
 using namespace llvm;
 
+bool ValueInfo::isDSOLocal() const {
+  // Need to check all summaries are local in case of hash collisions.
+  return getSummaryList().size() &&
+         llvm::all_of(getSummaryList(),
+                      [](const std::unique_ptr<GlobalValueSummary> &Summary) {
+                        return Summary->isDSOLocal();
+                      });
+}
+
 // Collect for the given module the list of function it defines
 // (GUID -> Summary).
 void ModuleSummaryIndex::collectDefinedFunctionsForModule(
     StringRef ModulePath, GVSummaryMapTy &GVSummaryMap) const {
   for (auto &GlobalList : *this) {
     auto GUID = GlobalList.first;
     for (auto &GlobSummary : GlobalList.second.SummaryList) {
       auto *Summary = dyn_cast_or_null<FunctionSummary>(GlobSummary.get());

llvm/trunk/lib/LTO/LTO.cpp

auto AddUsedCfiGlobal = [&](GlobalValue::GUID ValueGUID) {
     if (CfiFunctionDefs.count(ValueGUID))
       UsedCfiDefs.insert(ValueGUID);
     if (CfiFunctionDecls.count(ValueGUID))
       UsedCfiDecls.insert(ValueGUID);
   };
 
   auto AddUsedThings = [&](GlobalValueSummary *GS) {
     if (!GS) return;
-    for (const ValueInfo &VI : GS->refs())
+    for (const ValueInfo &VI : GS->refs()) {
+      AddUnsigned(VI.isDSOLocal());
       AddUsedCfiGlobal(VI.getGUID());
+    }
     if (auto *FS = dyn_cast<FunctionSummary>(GS)) {
       for (auto &TT : FS->type_tests())
         UsedTypeIds.insert(TT);
       for (auto &TT : FS->type_test_assume_vcalls())
         UsedTypeIds.insert(TT.GUID);
       for (auto &TT : FS->type_checked_load_vcalls())
         UsedTypeIds.insert(TT.GUID);
       for (auto &TT : FS->type_test_assume_const_vcalls())

llvm/trunk/lib/Transforms/Utils/FunctionImportUtils.cpp

 }
 
 void FunctionImportGlobalProcessing::processGlobalForThinLTO(GlobalValue &GV) {
 
   // Check the summaries to see if the symbol gets resolved to a known local
   // definition.
   if (GV.hasName()) {
     ValueInfo VI = ImportIndex.getValueInfo(GV.getGUID());
-    if (VI) {
-      // Need to check all summaries are local in case of hash collisions.
-      bool IsLocal = VI.getSummaryList().size() &&
-          llvm::all_of(VI.getSummaryList(),
-                       [](const std::unique_ptr<GlobalValueSummary> &Summary) {
-                         return Summary->isDSOLocal();
-                       });
-      if (IsLocal)
-        GV.setDSOLocal(true);
-    }
-  }
+    if (VI && VI.isDSOLocal())
 
   bool DoPromote = false;
   if (GV.hasLocalLinkage() &&
       ((DoPromote = shouldPromoteLocalToGlobal(&GV)) || isPerformingImport())) {
     // Once we change the name or linkage it is difficult to determine
     // again whether we should promote since shouldPromoteLocalToGlobal needs
     // to locate the summary (based on GUID from name and linkage). Therefore,
     // use DoPromote result saved above.

llvm/trunk/test/LTO/Resolution/X86/cache-dso-local.ll

+; Tests whether the cache is sensitive to the dso-local bit on referenced
+; globals.
+; RUN: rm -rf %t.cache
+; RUN: opt -module-hash -module-summary -o %t.bc %s
+; RUN: llvm-lto2 run -o %t.o %t.bc -cache-dir %t.cache \
+; RUN:   -r %t.bc,foo,px \
+; RUN:   -r %t.bc,bar,px
+; RUN: llvm-lto2 run -o %t.o %t.bc -cache-dir %t.cache \
+; RUN:   -r %t.bc,foo,plx \
+; RUN:   -r %t.bc,bar,px
+; RUN: ls %t.cache | count 2
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+define weak void @foo() {
+  ret void
+}
+
+define weak void()* @bar() {
+  ret void()* @foo
+}