[llvm] 377e131 - [ThinLTO] Only import for non-prevailing interposable global variables
Shoaib Meenai via llvm-commits
llvm-commits at lists.llvm.org
Sat Mar 25 21:37:57 PDT 2023
Author: Shoaib Meenai
Date: 2023-03-25T21:37:42-07:00
New Revision: 377e1311d50c7e5b5aab3db081938e0d0ceebdfc
URL: https://github.com/llvm/llvm-project/commit/377e1311d50c7e5b5aab3db081938e0d0ceebdfc
DIFF: https://github.com/llvm/llvm-project/commit/377e1311d50c7e5b5aab3db081938e0d0ceebdfc.diff
LOG: [ThinLTO] Only import for non-prevailing interposable global variables
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 https://github.com/llvm/llvm-project/issues/61677
Reviewed By: tejohnson
Differential Revision: https://reviews.llvm.org/D146876
Added:
llvm/test/ThinLTO/X86/nonprevailing_weak_globals_import.ll
llvm/test/ThinLTO/X86/prevailing_weak_globals_import.ll
Modified:
llvm/include/llvm/Transforms/IPO/FunctionImport.h
llvm/lib/LTO/LTO.cpp
llvm/lib/LTO/ThinLTOCodeGenerator.cpp
llvm/lib/Transforms/IPO/FunctionImport.cpp
Removed:
llvm/test/ThinLTO/X86/weak_globals_import.ll
################################################################################
diff --git a/llvm/include/llvm/Transforms/IPO/FunctionImport.h b/llvm/include/llvm/Transforms/IPO/FunctionImport.h
index c5bafb89fcb5e..92d171b20c60d 100644
--- 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
index ee6b8c3aa234d..4d6a62041c568 100644
--- 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
index 144cc5a1e8971..0549098f10209 100644
--- 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
index 4875c18e1facf..f9eaec589e103 100644
--- 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,6 +663,8 @@ 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.
@@ -660,7 +672,7 @@ void llvm::ComputeCrossModuleImport(
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/weak_globals_import.ll b/llvm/test/ThinLTO/X86/nonprevailing_weak_globals_import.ll
similarity index 100%
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
index 0000000000000..fa9159328fade
--- /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"
+ at 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"
+ at def = weak constant i32 0
+define ptr @ret_weak_def() {
+ ret ptr @def
+}
More information about the llvm-commits
mailing list