diff --git a/llvm/include/llvm/Transforms/IPO/FunctionImport.h b/llvm/include/llvm/Transforms/IPO/FunctionImport.h --- a/llvm/include/llvm/Transforms/IPO/FunctionImport.h +++ b/llvm/include/llvm/Transforms/IPO/FunctionImport.h @@ -146,6 +146,8 @@ void ComputeCrossModuleImport( const ModuleSummaryIndex &Index, const StringMap &ModuleToDefinedGVSummaries, + function_ref + isPrevailing, StringMap &ImportLists, StringMap &ExportLists); @@ -154,8 +156,10 @@ /// \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 + 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 --- a/llvm/lib/LTO/LTO.cpp +++ b/llvm/lib/LTO/LTO.cpp @@ -1553,7 +1553,7 @@ 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 --- a/llvm/lib/LTO/ThinLTOCodeGenerator.cpp +++ b/llvm/lib/LTO/ThinLTOCodeGenerator.cpp @@ -714,15 +714,17 @@ // Compute "dead" symbols, we don't want to import/export these! computeDeadSymbolsInIndex(Index, GUIDPreservedSymbols); + // Compute prevailing symbols + DenseMap PrevailingCopy; + computePrevailingCopies(Index, PrevailingCopy); + // Generate import/export list StringMap ImportLists(ModuleCount); StringMap ExportLists(ModuleCount); - ComputeCrossModuleImport(Index, ModuleToDefinedGVSummaries, ImportLists, + ComputeCrossModuleImport(Index, ModuleToDefinedGVSummaries, + IsPrevailing(PrevailingCopy), ImportLists, ExportLists); - DenseMap PrevailingCopy; - computePrevailingCopies(Index, PrevailingCopy); - // Resolve prevailing symbols StringMap> ResolvedODR; resolvePrevailingInIndex(Index, ResolvedODR, GUIDPreservedSymbols, @@ -764,10 +766,15 @@ // Compute "dead" symbols, we don't want to import/export these! computeDeadSymbolsInIndex(Index, GUIDPreservedSymbols); + // Compute prevailing symbols + DenseMap PrevailingCopy; + computePrevailingCopies(Index, PrevailingCopy); + // Generate import/export list StringMap ImportLists(ModuleCount); StringMap ExportLists(ModuleCount); - ComputeCrossModuleImport(Index, ModuleToDefinedGVSummaries, ImportLists, + ComputeCrossModuleImport(Index, ModuleToDefinedGVSummaries, + IsPrevailing(PrevailingCopy), ImportLists, ExportLists); auto &ImportList = ImportLists[TheModule.getModuleIdentifier()]; @@ -799,10 +806,15 @@ // Compute "dead" symbols, we don't want to import/export these! computeDeadSymbolsInIndex(Index, GUIDPreservedSymbols); + // Compute prevailing symbols + DenseMap PrevailingCopy; + computePrevailingCopies(Index, PrevailingCopy); + // Generate import/export list StringMap ImportLists(ModuleCount); StringMap ExportLists(ModuleCount); - ComputeCrossModuleImport(Index, ModuleToDefinedGVSummaries, ImportLists, + ComputeCrossModuleImport(Index, ModuleToDefinedGVSummaries, + IsPrevailing(PrevailingCopy), ImportLists, ExportLists); llvm::gatherImportedSummariesForModule( @@ -832,10 +844,15 @@ // Compute "dead" symbols, we don't want to import/export these! computeDeadSymbolsInIndex(Index, GUIDPreservedSymbols); + // Compute prevailing symbols + DenseMap PrevailingCopy; + computePrevailingCopies(Index, PrevailingCopy); + // Generate import/export list StringMap ImportLists(ModuleCount); StringMap ExportLists(ModuleCount); - ComputeCrossModuleImport(Index, ModuleToDefinedGVSummaries, ImportLists, + ComputeCrossModuleImport(Index, ModuleToDefinedGVSummaries, + IsPrevailing(PrevailingCopy), ImportLists, ExportLists); std::map ModuleToSummariesForIndex; @@ -874,10 +891,15 @@ // Compute "dead" symbols, we don't want to import/export these! computeDeadSymbolsInIndex(Index, GUIDPreservedSymbols); + // Compute prevailing symbols + DenseMap PrevailingCopy; + computePrevailingCopies(Index, PrevailingCopy); + // Generate import/export list StringMap ImportLists(ModuleCount); StringMap ExportLists(ModuleCount); - ComputeCrossModuleImport(Index, ModuleToDefinedGVSummaries, ImportLists, + ComputeCrossModuleImport(Index, ModuleToDefinedGVSummaries, + IsPrevailing(PrevailingCopy), ImportLists, ExportLists); auto &ExportList = ExportLists[ModuleIdentifier]; @@ -886,9 +908,6 @@ if (ExportList.empty() && GUIDPreservedSymbols.empty()) return; - DenseMap PrevailingCopy; - computePrevailingCopies(Index, PrevailingCopy); - // Resolve prevailing symbols StringMap> ResolvedODR; resolvePrevailingInIndex(Index, ResolvedODR, GUIDPreservedSymbols, @@ -1068,11 +1087,16 @@ for (auto GUID : ExportedGUIDs) GUIDPreservedSymbols.insert(GUID); + // Compute prevailing symbols + DenseMap PrevailingCopy; + computePrevailingCopies(*Index, PrevailingCopy); + // Collect the import/export lists for all modules from the call-graph in the // combined index. StringMap ImportLists(ModuleCount); StringMap 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 @@ // on the index, and nuke this map. StringMap> ResolvedODR; - DenseMap 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 --- a/llvm/lib/Transforms/IPO/FunctionImport.cpp +++ b/llvm/lib/Transforms/IPO/FunctionImport.cpp @@ -245,24 +245,25 @@ } // anonymous namespace -static bool shouldImportGlobal(const ValueInfo &VI, - const GVSummaryMapTy &DefinedGVSummaries) { +static bool shouldImportGlobal( + const ValueInfo &VI, const GVSummaryMapTy &DefinedGVSummaries, + function_ref + 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 void computeImportForReferencedGlobals( const GlobalValueSummary &Summary, const ModuleSummaryIndex &Index, const GVSummaryMapTy &DefinedGVSummaries, + function_ref + isPrevailing, SmallVectorImpl &Worklist, FunctionImporter::ImportMapTy &ImportList, StringMap *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 @@ static void computeImportForFunction( const FunctionSummary &Summary, const ModuleSummaryIndex &Index, const unsigned Threshold, const GVSummaryMapTy &DefinedGVSummaries, + function_ref + isPrevailing, SmallVectorImpl &Worklist, FunctionImporter::ImportMapTy &ImportList, StringMap *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 @@ /// 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 + isPrevailing, + const ModuleSummaryIndex &Index, StringRef ModName, + FunctionImporter::ImportMapTy &ImportList, StringMap *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 @@ 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 @@ if (auto *FS = dyn_cast(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,6 +663,8 @@ void llvm::ComputeCrossModuleImport( const ModuleSummaryIndex &Index, const StringMap &ModuleToDefinedGVSummaries, + function_ref + isPrevailing, StringMap &ImportLists, StringMap &ExportLists) { // For each module that has function defined, compute the import/export lists. @@ -660,7 +672,7 @@ 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 @@ /// Compute all the imports for the given module in the Index. void llvm::ComputeCrossModuleImportForModule( - StringRef ModulePath, const ModuleSummaryIndex &Index, + StringRef ModulePath, + function_ref + isPrevailing, + const ModuleSummaryIndex &Index, FunctionImporter::ImportMapTy &ImportList) { // Collect the list of functions this module defines. // GUID -> Summary @@ -768,7 +783,8 @@ // 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 @@ return ImportedCount; } -static bool doImportingForModule(Module &M) { +static bool doImportingForModule( + Module &M, function_ref + isPrevailing) { if (SummaryFile.empty()) report_fatal_error("error: -function-import requires -summary-file\n"); Expected> IndexPtrOrErr = @@ -1416,8 +1434,8 @@ 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 @@ 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/weak_globals_import.ll b/llvm/test/ThinLTO/X86/nonprevailing_weak_globals_import.ll rename from llvm/test/ThinLTO/X86/weak_globals_import.ll rename to 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 new file mode 100644 --- /dev/null +++ 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 +}