[ThinLTO] Only import for non-prevailing interposable global variables

smeenai · smeenai · commit 377e1311d50c · 2023-03-25T21:37:42.000-07:00
This logic was added in https://reviews.llvm.org/D95943 specifically to handle an issue for non-prevailing global variables. It turns out that it adds a new issue for prevailing glboal variables, since those could be replaced by an available_externally definition and hence incorrectly omitted from the output object file. Limit the import to non-prevailing global variables to fix this, as suggested by @tejohnson. The bulk of the diff is mechanical changes to thread isPrevailing through to where it's needed and ensure it's available before the relevant calls; the actual logic change itself is straightforward. Fixes #61677 Reviewed By: tejohnson Differential Revision: https://reviews.llvm.org/D146876
diff --git a/llvm/include/llvm/Transforms/IPO/FunctionImport.h b/llvm/include/llvm/Transforms/IPO/FunctionImport.h
@@ -146,6 +146,8 @@ class FunctionImportPass : public PassInfoMixin<FunctionImportPass> {
 void ComputeCrossModuleImport(
     const ModuleSummaryIndex &Index,
     const StringMap<GVSummaryMapTy> &ModuleToDefinedGVSummaries,
+    function_ref<bool(GlobalValue::GUID, const GlobalValueSummary *)>
+        isPrevailing,
     StringMap<FunctionImporter::ImportMapTy> &ImportLists,
     StringMap<FunctionImporter::ExportSetTy> &ExportLists);
 
@@ -154,8 +156,10 @@ void ComputeCrossModuleImport(
 /// \p ImportList will be populated with a map that can be passed to
 /// FunctionImporter::importFunctions() above (see description there).
 void ComputeCrossModuleImportForModule(
-    StringRef ModulePath, const ModuleSummaryIndex &Index,
-    FunctionImporter::ImportMapTy &ImportList);
+    StringRef ModulePath,
+    function_ref<bool(GlobalValue::GUID, const GlobalValueSummary *)>
+        isPrevailing,
+    const ModuleSummaryIndex &Index, FunctionImporter::ImportMapTy &ImportList);
 
 /// Mark all external summaries in \p Index for import into the given module.
 /// Used for distributed builds using a distributed index.
diff --git a/llvm/lib/LTO/LTO.cpp b/llvm/lib/LTO/LTO.cpp
@@ -1553,7 +1553,7 @@ Error LTO::runThinLTO(AddStreamFn AddStream, FileCache Cache,
 
   if (Conf.OptLevel > 0)
     ComputeCrossModuleImport(ThinLTO.CombinedIndex, ModuleToDefinedGVSummaries,
-                             ImportLists, ExportLists);
+                             isPrevailing, ImportLists, ExportLists);
 
   // Figure out which symbols need to be internalized. This also needs to happen
   // at -O0 because summary-based DCE is implemented using internalization, and
diff --git a/llvm/lib/LTO/ThinLTOCodeGenerator.cpp b/llvm/lib/LTO/ThinLTOCodeGenerator.cpp
@@ -714,15 +714,17 @@ void ThinLTOCodeGenerator::promote(Module &TheModule, ModuleSummaryIndex &Index,
   // Compute "dead" symbols, we don't want to import/export these!
   computeDeadSymbolsInIndex(Index, GUIDPreservedSymbols);
 
+  // Compute prevailing symbols
+  DenseMap<GlobalValue::GUID, const GlobalValueSummary *> PrevailingCopy;
+  computePrevailingCopies(Index, PrevailingCopy);
+
   // Generate import/export list
   StringMap<FunctionImporter::ImportMapTy> ImportLists(ModuleCount);
   StringMap<FunctionImporter::ExportSetTy> ExportLists(ModuleCount);
-  ComputeCrossModuleImport(Index, ModuleToDefinedGVSummaries, ImportLists,
+  ComputeCrossModuleImport(Index, ModuleToDefinedGVSummaries,
+                           IsPrevailing(PrevailingCopy), ImportLists,
                            ExportLists);
 
-  DenseMap<GlobalValue::GUID, const GlobalValueSummary *> PrevailingCopy;
-  computePrevailingCopies(Index, PrevailingCopy);
-
   // Resolve prevailing symbols
   StringMap<std::map<GlobalValue::GUID, GlobalValue::LinkageTypes>> ResolvedODR;
   resolvePrevailingInIndex(Index, ResolvedODR, GUIDPreservedSymbols,
@@ -764,10 +766,15 @@ void ThinLTOCodeGenerator::crossModuleImport(Module &TheModule,
   // Compute "dead" symbols, we don't want to import/export these!
   computeDeadSymbolsInIndex(Index, GUIDPreservedSymbols);
 
+  // Compute prevailing symbols
+  DenseMap<GlobalValue::GUID, const GlobalValueSummary *> PrevailingCopy;
+  computePrevailingCopies(Index, PrevailingCopy);
+
   // Generate import/export list
   StringMap<FunctionImporter::ImportMapTy> ImportLists(ModuleCount);
   StringMap<FunctionImporter::ExportSetTy> ExportLists(ModuleCount);
-  ComputeCrossModuleImport(Index, ModuleToDefinedGVSummaries, ImportLists,
+  ComputeCrossModuleImport(Index, ModuleToDefinedGVSummaries,
+                           IsPrevailing(PrevailingCopy), ImportLists,
                            ExportLists);
   auto &ImportList = ImportLists[TheModule.getModuleIdentifier()];
 
@@ -799,10 +806,15 @@ void ThinLTOCodeGenerator::gatherImportedSummariesForModule(
   // Compute "dead" symbols, we don't want to import/export these!
   computeDeadSymbolsInIndex(Index, GUIDPreservedSymbols);
 
+  // Compute prevailing symbols
+  DenseMap<GlobalValue::GUID, const GlobalValueSummary *> PrevailingCopy;
+  computePrevailingCopies(Index, PrevailingCopy);
+
   // Generate import/export list
   StringMap<FunctionImporter::ImportMapTy> ImportLists(ModuleCount);
   StringMap<FunctionImporter::ExportSetTy> ExportLists(ModuleCount);
-  ComputeCrossModuleImport(Index, ModuleToDefinedGVSummaries, ImportLists,
+  ComputeCrossModuleImport(Index, ModuleToDefinedGVSummaries,
+                           IsPrevailing(PrevailingCopy), ImportLists,
                            ExportLists);
 
   llvm::gatherImportedSummariesForModule(
@@ -832,10 +844,15 @@ void ThinLTOCodeGenerator::emitImports(Module &TheModule, StringRef OutputName,
   // Compute "dead" symbols, we don't want to import/export these!
   computeDeadSymbolsInIndex(Index, GUIDPreservedSymbols);
 
+  // Compute prevailing symbols
+  DenseMap<GlobalValue::GUID, const GlobalValueSummary *> PrevailingCopy;
+  computePrevailingCopies(Index, PrevailingCopy);
+
   // Generate import/export list
   StringMap<FunctionImporter::ImportMapTy> ImportLists(ModuleCount);
   StringMap<FunctionImporter::ExportSetTy> ExportLists(ModuleCount);
-  ComputeCrossModuleImport(Index, ModuleToDefinedGVSummaries, ImportLists,
+  ComputeCrossModuleImport(Index, ModuleToDefinedGVSummaries,
+                           IsPrevailing(PrevailingCopy), ImportLists,
                            ExportLists);
 
   std::map<std::string, GVSummaryMapTy> ModuleToSummariesForIndex;
@@ -874,10 +891,15 @@ void ThinLTOCodeGenerator::internalize(Module &TheModule,
   // Compute "dead" symbols, we don't want to import/export these!
   computeDeadSymbolsInIndex(Index, GUIDPreservedSymbols);
 
+  // Compute prevailing symbols
+  DenseMap<GlobalValue::GUID, const GlobalValueSummary *> PrevailingCopy;
+  computePrevailingCopies(Index, PrevailingCopy);
+
   // Generate import/export list
   StringMap<FunctionImporter::ImportMapTy> ImportLists(ModuleCount);
   StringMap<FunctionImporter::ExportSetTy> ExportLists(ModuleCount);
-  ComputeCrossModuleImport(Index, ModuleToDefinedGVSummaries, ImportLists,
+  ComputeCrossModuleImport(Index, ModuleToDefinedGVSummaries,
+                           IsPrevailing(PrevailingCopy), ImportLists,
                            ExportLists);
   auto &ExportList = ExportLists[ModuleIdentifier];
 
@@ -886,9 +908,6 @@ void ThinLTOCodeGenerator::internalize(Module &TheModule,
   if (ExportList.empty() && GUIDPreservedSymbols.empty())
     return;
 
-  DenseMap<GlobalValue::GUID, const GlobalValueSummary *> PrevailingCopy;
-  computePrevailingCopies(Index, PrevailingCopy);
-
   // Resolve prevailing symbols
   StringMap<std::map<GlobalValue::GUID, GlobalValue::LinkageTypes>> ResolvedODR;
   resolvePrevailingInIndex(Index, ResolvedODR, GUIDPreservedSymbols,
@@ -1068,11 +1087,16 @@ void ThinLTOCodeGenerator::run() {
   for (auto GUID : ExportedGUIDs)
     GUIDPreservedSymbols.insert(GUID);
 
+  // Compute prevailing symbols
+  DenseMap<GlobalValue::GUID, const GlobalValueSummary *> PrevailingCopy;
+  computePrevailingCopies(*Index, PrevailingCopy);
+
   // Collect the import/export lists for all modules from the call-graph in the
   // combined index.
   StringMap<FunctionImporter::ImportMapTy> ImportLists(ModuleCount);
   StringMap<FunctionImporter::ExportSetTy> ExportLists(ModuleCount);
-  ComputeCrossModuleImport(*Index, ModuleToDefinedGVSummaries, ImportLists,
+  ComputeCrossModuleImport(*Index, ModuleToDefinedGVSummaries,
+                           IsPrevailing(PrevailingCopy), ImportLists,
                            ExportLists);
 
   // We use a std::map here to be able to have a defined ordering when
@@ -1081,9 +1105,6 @@ void ThinLTOCodeGenerator::run() {
   // on the index, and nuke this map.
   StringMap<std::map<GlobalValue::GUID, GlobalValue::LinkageTypes>> ResolvedODR;
 
-  DenseMap<GlobalValue::GUID, const GlobalValueSummary *> PrevailingCopy;
-  computePrevailingCopies(*Index, PrevailingCopy);
-
   // Resolve prevailing symbols, this has to be computed early because it
   // impacts the caching.
   resolvePrevailingInIndex(*Index, ResolvedODR, GUIDPreservedSymbols,
diff --git a/llvm/lib/Transforms/IPO/FunctionImport.cpp b/llvm/lib/Transforms/IPO/FunctionImport.cpp
@@ -245,24 +245,25 @@ using EdgeInfo =
 
 } // anonymous namespace
 
-static bool shouldImportGlobal(const ValueInfo &VI,
-                               const GVSummaryMapTy &DefinedGVSummaries) {
+static bool shouldImportGlobal(
+    const ValueInfo &VI, const GVSummaryMapTy &DefinedGVSummaries,
+    function_ref<bool(GlobalValue::GUID, const GlobalValueSummary *)>
+        isPrevailing) {
   const auto &GVS = DefinedGVSummaries.find(VI.getGUID());
   if (GVS == DefinedGVSummaries.end())
     return true;
-  // We should not skip import if the module contains a definition with
-  // interposable linkage type. This is required for correctness in
-  // the situation with two following conditions:
-  // * the def with interposable linkage is non-prevailing,
-  // * there is a prevailing def available for import and marked read-only.
-  // In this case, the non-prevailing def will be converted to a declaration,
-  // while the prevailing one becomes internal, thus no definitions will be
-  // available for linking. In order to prevent undefined symbol link error,
-  // the prevailing definition must be imported.
+  // We should not skip import if the module contains a non-prevailing
+  // definition with interposable linkage type. This is required for correctness
+  // in the situation where there is a prevailing def available for import and
+  // marked read-only. In this case, the non-prevailing def will be converted to
+  // a declaration, while the prevailing one becomes internal, thus no
+  // definitions will be available for linking. In order to prevent undefined
+  // symbol link error, the prevailing definition must be imported.
   // FIXME: Consider adding a check that the suitable prevailing definition
   // exists and marked read-only.
   if (VI.getSummaryList().size() > 1 &&
-      GlobalValue::isInterposableLinkage(GVS->second->linkage()))
+      GlobalValue::isInterposableLinkage(GVS->second->linkage()) &&
+      !isPrevailing(VI.getGUID(), GVS->second))
     return true;
 
   return false;
@@ -271,11 +272,13 @@ static bool shouldImportGlobal(const ValueInfo &VI,
 static void computeImportForReferencedGlobals(
     const GlobalValueSummary &Summary, const ModuleSummaryIndex &Index,
     const GVSummaryMapTy &DefinedGVSummaries,
+    function_ref<bool(GlobalValue::GUID, const GlobalValueSummary *)>
+        isPrevailing,
     SmallVectorImpl<EdgeInfo> &Worklist,
     FunctionImporter::ImportMapTy &ImportList,
     StringMap<FunctionImporter::ExportSetTy> *ExportLists) {
   for (const auto &VI : Summary.refs()) {
-    if (!shouldImportGlobal(VI, DefinedGVSummaries)) {
+    if (!shouldImportGlobal(VI, DefinedGVSummaries, isPrevailing)) {
       LLVM_DEBUG(
           dbgs() << "Ref ignored! Target already in destination module.\n");
       continue;
@@ -348,12 +351,15 @@ getFailureName(FunctionImporter::ImportFailureReason Reason) {
 static void computeImportForFunction(
     const FunctionSummary &Summary, const ModuleSummaryIndex &Index,
     const unsigned Threshold, const GVSummaryMapTy &DefinedGVSummaries,
+    function_ref<bool(GlobalValue::GUID, const GlobalValueSummary *)>
+        isPrevailing,
     SmallVectorImpl<EdgeInfo> &Worklist,
     FunctionImporter::ImportMapTy &ImportList,
     StringMap<FunctionImporter::ExportSetTy> *ExportLists,
     FunctionImporter::ImportThresholdsTy &ImportThresholds) {
   computeImportForReferencedGlobals(Summary, Index, DefinedGVSummaries,
-                                    Worklist, ImportList, ExportLists);
+                                    isPrevailing, Worklist, ImportList,
+                                    ExportLists);
   static int ImportCount = 0;
   for (const auto &Edge : Summary.calls()) {
     ValueInfo VI = Edge.first;
@@ -519,8 +525,11 @@ static void computeImportForFunction(
 /// as well as the list of "exports", i.e. the list of symbols referenced from
 /// another module (that may require promotion).
 static void ComputeImportForModule(
-    const GVSummaryMapTy &DefinedGVSummaries, const ModuleSummaryIndex &Index,
-    StringRef ModName, FunctionImporter::ImportMapTy &ImportList,
+    const GVSummaryMapTy &DefinedGVSummaries,
+    function_ref<bool(GlobalValue::GUID, const GlobalValueSummary *)>
+        isPrevailing,
+    const ModuleSummaryIndex &Index, StringRef ModName,
+    FunctionImporter::ImportMapTy &ImportList,
     StringMap<FunctionImporter::ExportSetTy> *ExportLists = nullptr) {
   // Worklist contains the list of function imported in this module, for which
   // we will analyse the callees and may import further down the callgraph.
@@ -546,8 +555,8 @@ static void ComputeImportForModule(
       continue;
     LLVM_DEBUG(dbgs() << "Initialize import for " << VI << "\n");
     computeImportForFunction(*FuncSummary, Index, ImportInstrLimit,
-                             DefinedGVSummaries, Worklist, ImportList,
-                             ExportLists, ImportThresholds);
+                             DefinedGVSummaries, isPrevailing, Worklist,
+                             ImportList, ExportLists, ImportThresholds);
   }
 
   // Process the newly imported functions and add callees to the worklist.
@@ -558,11 +567,12 @@ static void ComputeImportForModule(
 
     if (auto *FS = dyn_cast<FunctionSummary>(Summary))
       computeImportForFunction(*FS, Index, Threshold, DefinedGVSummaries,
-                               Worklist, ImportList, ExportLists,
+                               isPrevailing, Worklist, ImportList, ExportLists,
                                ImportThresholds);
     else
       computeImportForReferencedGlobals(*Summary, Index, DefinedGVSummaries,
-                                        Worklist, ImportList, ExportLists);
+                                        isPrevailing, Worklist, ImportList,
+                                        ExportLists);
   }
 
   // Print stats about functions considered but rejected for importing
@@ -653,14 +663,16 @@ checkVariableImport(const ModuleSummaryIndex &Index,
 void llvm::ComputeCrossModuleImport(
     const ModuleSummaryIndex &Index,
     const StringMap<GVSummaryMapTy> &ModuleToDefinedGVSummaries,
+    function_ref<bool(GlobalValue::GUID, const GlobalValueSummary *)>
+        isPrevailing,
     StringMap<FunctionImporter::ImportMapTy> &ImportLists,
     StringMap<FunctionImporter::ExportSetTy> &ExportLists) {
   // For each module that has function defined, compute the import/export lists.
   for (const auto &DefinedGVSummaries : ModuleToDefinedGVSummaries) {
     auto &ImportList = ImportLists[DefinedGVSummaries.first()];
     LLVM_DEBUG(dbgs() << "Computing import for Module '"
                       << DefinedGVSummaries.first() << "'\n");
-    ComputeImportForModule(DefinedGVSummaries.second, Index,
+    ComputeImportForModule(DefinedGVSummaries.second, isPrevailing, Index,
                            DefinedGVSummaries.first(), ImportList,
                            &ExportLists);
   }
@@ -759,7 +771,10 @@ static void dumpImportListForModule(const ModuleSummaryIndex &Index,
 
 /// Compute all the imports for the given module in the Index.
 void llvm::ComputeCrossModuleImportForModule(
-    StringRef ModulePath, const ModuleSummaryIndex &Index,
+    StringRef ModulePath,
+    function_ref<bool(GlobalValue::GUID, const GlobalValueSummary *)>
+        isPrevailing,
+    const ModuleSummaryIndex &Index,
     FunctionImporter::ImportMapTy &ImportList) {
   // Collect the list of functions this module defines.
   // GUID -> Summary
@@ -768,7 +783,8 @@ void llvm::ComputeCrossModuleImportForModule(
 
   // Compute the import list for this module.
   LLVM_DEBUG(dbgs() << "Computing import for Module '" << ModulePath << "'\n");
-  ComputeImportForModule(FunctionSummaryMap, Index, ModulePath, ImportList);
+  ComputeImportForModule(FunctionSummaryMap, isPrevailing, Index, ModulePath,
+                         ImportList);
 
 #ifndef NDEBUG
   dumpImportListForModule(Index, ModulePath, ImportList);
@@ -1395,7 +1411,9 @@ Expected<bool> FunctionImporter::importFunctions(
   return ImportedCount;
 }
 
-static bool doImportingForModule(Module &M) {
+static bool doImportingForModule(
+    Module &M, function_ref<bool(GlobalValue::GUID, const GlobalValueSummary *)>
+                   isPrevailing) {
   if (SummaryFile.empty())
     report_fatal_error("error: -function-import requires -summary-file\n");
   Expected<std::unique_ptr<ModuleSummaryIndex>> IndexPtrOrErr =
@@ -1416,8 +1434,8 @@ static bool doImportingForModule(Module &M) {
     ComputeCrossModuleImportForModuleFromIndex(M.getModuleIdentifier(), *Index,
                                                ImportList);
   else
-    ComputeCrossModuleImportForModule(M.getModuleIdentifier(), *Index,
-                                      ImportList);
+    ComputeCrossModuleImportForModule(M.getModuleIdentifier(), isPrevailing,
+                                      *Index, ImportList);
 
   // Conservatively mark all internal values as promoted. This interface is
   // only used when doing importing via the function importing pass. The pass
@@ -1458,7 +1476,14 @@ static bool doImportingForModule(Module &M) {
 
 PreservedAnalyses FunctionImportPass::run(Module &M,
                                           ModuleAnalysisManager &AM) {
-  if (!doImportingForModule(M))
+  // This is only used for testing the function import pass via opt, where we
+  // don't have prevailing information from the LTO context available, so just
+  // conservatively assume everything is prevailing (which is fine for the very
+  // limited use of prevailing checking in this pass).
+  auto isPrevailing = [](GlobalValue::GUID, const GlobalValueSummary *) {
+    return true;
+  };
+  if (!doImportingForModule(M, isPrevailing))
     return PreservedAnalyses::all();
 
   return PreservedAnalyses::none();
diff --git a/llvm/test/ThinLTO/X86/nonprevailing_weak_globals_import.ll b/llvm/test/ThinLTO/X86/nonprevailing_weak_globals_import.ll
diff --git a/llvm/test/ThinLTO/X86/prevailing_weak_globals_import.ll b/llvm/test/ThinLTO/X86/prevailing_weak_globals_import.ll
@@ -0,0 +1,31 @@
+; RUN: split-file %s %t
+
+; RUN: opt -module-summary %t/av_ext_def.ll -o %t/av_ext_def.bc
+; RUN: opt -module-summary %t/weak_def.ll -o %t/weak_def.bc
+; RUN: llvm-lto2 run -o %t/prevailing_import -save-temps %t/av_ext_def.bc %t/weak_def.bc \
+; RUN:   -r=%t/av_ext_def.bc,ret_av_ext_def,px -r=%t/av_ext_def.bc,def,x \
+; RUN:   -r=%t/weak_def.bc,ret_weak_def,px -r=%t/weak_def.bc,def,px
+; RUN: llvm-dis %t/prevailing_import.2.3.import.bc -o - | FileCheck --match-full-lines --check-prefix=WEAK_DEF %s
+; RUN: llvm-nm -jU %t/prevailing_import.2 | FileCheck --match-full-lines --check-prefix=NM %s
+
+;; def should remain weak after function importing in the weak_def module
+; WEAK_DEF: @def = weak constant i32 0
+
+;; It should also be defined in the corresponding object file
+; NM: def
+
+;--- av_ext_def.ll
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+@def = available_externally constant i32 0
+define ptr @ret_av_ext_def() {
+  ret ptr @def
+}
+
+;--- weak_def.ll
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+@def = weak constant i32 0
+define ptr @ret_weak_def() {
+  ret ptr @def
+}
