From 14deb75f8d82647260165ab791794d29778befa5 Mon Sep 17 00:00:00 2001 From: mingmingl Date: Mon, 1 Jul 2024 14:14:18 -0700 Subject: [PATCH 1/4] [ThinLTO] Track definitions only in export-set --- .../llvm/Transforms/IPO/FunctionImport.h | 11 ++-- llvm/lib/LTO/LTO.cpp | 14 +++-- llvm/lib/Transforms/IPO/FunctionImport.cpp | 52 +++++++++++-------- 3 files changed, 41 insertions(+), 36 deletions(-) diff --git a/llvm/include/llvm/Transforms/IPO/FunctionImport.h b/llvm/include/llvm/Transforms/IPO/FunctionImport.h index d8c142ec89d82..3b03ba82b9272 100644 --- a/llvm/include/llvm/Transforms/IPO/FunctionImport.h +++ b/llvm/include/llvm/Transforms/IPO/FunctionImport.h @@ -104,13 +104,10 @@ class FunctionImporter { /// index's module path string table). using ImportMapTy = DenseMap; - /// The map contains an entry for every global value the module exports. - /// The key is ValueInfo, and the value indicates whether the definition - /// or declaration is visible to another module. If a function's definition is - /// visible to other modules, the global values this function referenced are - /// visible and shouldn't be internalized. - /// TODO: Rename to `ExportMapTy`. - using ExportSetTy = DenseMap; + /// The set contains an entry for every global value that the module exports. + /// Depending on the user context, this container is allowed to contain + /// definitions, declarations or a mix of both. + using ExportSetTy = DenseSet; /// A function of this type is used to load modules referenced by the index. using ModuleLoaderTy = diff --git a/llvm/lib/LTO/LTO.cpp b/llvm/lib/LTO/LTO.cpp index 6bbec535d8e98..5382b1158cb04 100644 --- a/llvm/lib/LTO/LTO.cpp +++ b/llvm/lib/LTO/LTO.cpp @@ -161,19 +161,17 @@ void llvm::computeLTOCacheKey( auto ModHash = Index.getModuleHash(ModuleID); Hasher.update(ArrayRef((uint8_t *)&ModHash[0], sizeof(ModHash))); - std::vector> ExportsGUID; + // TODO: `ExportList` is determined by `ImportList`. Since `ImportList` is + // used to compute cache key, we could omit hashing `ExportList` here. + std::vector ExportsGUID; ExportsGUID.reserve(ExportList.size()); - for (const auto &[VI, ExportType] : ExportList) - ExportsGUID.push_back( - std::make_pair(VI.getGUID(), static_cast(ExportType))); + for (const auto &VI : ExportList) + ExportsGUID.push_back(VI.getGUID()); // Sort the export list elements GUIDs. llvm::sort(ExportsGUID); - for (auto [GUID, ExportType] : ExportsGUID) { - // The export list can impact the internalization, be conservative here + for (auto GUID : ExportsGUID) Hasher.update(ArrayRef((uint8_t *)&GUID, sizeof(GUID))); - AddUint8(ExportType); - } // Include the hash for every module we import functions from. The set of // imported symbols for each module may affect code generation and is diff --git a/llvm/lib/Transforms/IPO/FunctionImport.cpp b/llvm/lib/Transforms/IPO/FunctionImport.cpp index ec5294b9512cf..f2e67aa998606 100644 --- a/llvm/lib/Transforms/IPO/FunctionImport.cpp +++ b/llvm/lib/Transforms/IPO/FunctionImport.cpp @@ -400,8 +400,7 @@ class GlobalsImporter final { // later, in ComputeCrossModuleImport, after import decisions are // complete, which is more efficient than adding them here. if (ExportLists) - (*ExportLists)[RefSummary->modulePath()][VI] = - GlobalValueSummary::Definition; + (*ExportLists)[RefSummary->modulePath()].insert(VI); // If variable is not writeonly we attempt to recursively analyze // its references in order to import referenced constants. @@ -582,7 +581,7 @@ class WorkloadImportsManager : public ModuleImportsManager { GlobalValueSummary::Definition; GVI.onImportingSummary(*GVS); if (ExportLists) - (*ExportLists)[ExportingModule][VI] = GlobalValueSummary::Definition; + (*ExportLists)[ExportingModule].insert(VI); } LLVM_DEBUG(dbgs() << "[Workload] Done\n"); } @@ -818,10 +817,8 @@ static void computeImportForFunction( // Since definition takes precedence over declaration for the same VI, // try emplace pair without checking insert result. // If insert doesn't happen, there must be an existing entry keyed by - // VI. - if (ExportLists) - (*ExportLists)[DeclSourceModule].try_emplace( - VI, GlobalValueSummary::Declaration); + // VI. Note `ExportLists` only keeps track of definitions so VI won't + // be inserted. ImportList[DeclSourceModule].try_emplace( VI.getGUID(), GlobalValueSummary::Declaration); } @@ -892,7 +889,7 @@ static void computeImportForFunction( // later, in ComputeCrossModuleImport, after import decisions are // complete, which is more efficient than adding them here. if (ExportLists) - (*ExportLists)[ExportModulePath][VI] = GlobalValueSummary::Definition; + (*ExportLists)[ExportModulePath].insert(VI); } auto GetAdjustedThreshold = [](unsigned Threshold, bool IsHotCallsite) { @@ -998,14 +995,29 @@ static bool isGlobalVarSummary(const ModuleSummaryIndex &Index, return false; } -template -static unsigned numGlobalVarSummaries(const ModuleSummaryIndex &Index, T &Cont, +static unsigned numGlobalVarSummaries(const ModuleSummaryIndex &Index, + FunctionImporter::ExportSetTy &ExportSet, unsigned &DefinedGVS, unsigned &DefinedFS) { + DefinedGVS = 0; + DefinedFS = 0; + for (auto &VI : ExportSet) { + if (isGlobalVarSummary(Index, VI.getGUID())) { + ++DefinedGVS; + } else + ++DefinedFS; + } + return DefinedGVS; +} + +static unsigned +numGlobalVarSummaries(const ModuleSummaryIndex &Index, + FunctionImporter::FunctionsToImportTy &ImportMap, + unsigned &DefinedGVS, unsigned &DefinedFS) { unsigned NumGVS = 0; DefinedGVS = 0; DefinedFS = 0; - for (auto &[GUID, Type] : Cont) { + for (auto &[GUID, Type] : ImportMap) { if (isGlobalVarSummary(Index, GUID)) { if (Type == GlobalValueSummary::Definition) ++DefinedGVS; @@ -1046,7 +1058,7 @@ static bool checkVariableImport( }; for (auto &ExportPerModule : ExportLists) - for (auto &[VI, Unused] : ExportPerModule.second) + for (auto &VI : ExportPerModule.second) if (!FlattenedImports.count(VI.getGUID()) && IsReadOrWriteOnlyVarNeedingImporting(ExportPerModule.first, VI)) return false; @@ -1079,14 +1091,12 @@ void llvm::ComputeCrossModuleImport( // since we may import the same values multiple times into different modules // during the import computation. for (auto &ELI : ExportLists) { + // `NewExports` tracks the VI that gets exported because the full definition + // of its user/referencer gets exported. FunctionImporter::ExportSetTy NewExports; const auto &DefinedGVSummaries = ModuleToDefinedGVSummaries.lookup(ELI.first); - for (auto &[EI, Type] : ELI.second) { - // If a variable is exported as a declaration, its 'refs' and 'calls' are - // not further exported. - if (Type == GlobalValueSummary::Declaration) - continue; + for (auto &EI : ELI.second) { // Find the copy defined in the exporting module so that we can mark the // values it references in that specific definition as exported. // Below we will add all references and called values, without regard to @@ -1108,19 +1118,19 @@ void llvm::ComputeCrossModuleImport( for (const auto &VI : GVS->refs()) { // Try to emplace the declaration entry. If a definition entry // already exists for key `VI`, this is a no-op. - NewExports.try_emplace(VI, GlobalValueSummary::Declaration); + NewExports.insert(VI); } } else { auto *FS = cast(S); for (const auto &Edge : FS->calls()) { // Try to emplace the declaration entry. If a definition entry // already exists for key `VI`, this is a no-op. - NewExports.try_emplace(Edge.first, GlobalValueSummary::Declaration); + NewExports.insert(Edge.first); } for (const auto &Ref : FS->refs()) { // Try to emplace the declaration entry. If a definition entry // already exists for key `VI`, this is a no-op. - NewExports.try_emplace(Ref, GlobalValueSummary::Declaration); + NewExports.insert(Ref); } } } @@ -1129,7 +1139,7 @@ void llvm::ComputeCrossModuleImport( // the same ref/call target multiple times in above loop, and it is more // efficient to avoid a set lookup each time. for (auto EI = NewExports.begin(); EI != NewExports.end();) { - if (!DefinedGVSummaries.count(EI->first.getGUID())) + if (!DefinedGVSummaries.count(EI->getGUID())) NewExports.erase(EI++); else ++EI; From db822c99d0ed5da097e6df4e403f8b9652b1e206 Mon Sep 17 00:00:00 2001 From: mingmingl Date: Tue, 2 Jul 2024 22:16:47 -0700 Subject: [PATCH 2/4] resolve feedback --- llvm/lib/Transforms/IPO/FunctionImport.cpp | 78 ++++++++----------- llvm/test/ThinLTO/X86/funcimport-stats.ll | 2 +- .../Transforms/FunctionImport/funcimport.ll | 2 +- 3 files changed, 34 insertions(+), 48 deletions(-) diff --git a/llvm/lib/Transforms/IPO/FunctionImport.cpp b/llvm/lib/Transforms/IPO/FunctionImport.cpp index f2e67aa998606..b36fd210b28cd 100644 --- a/llvm/lib/Transforms/IPO/FunctionImport.cpp +++ b/llvm/lib/Transforms/IPO/FunctionImport.cpp @@ -817,8 +817,8 @@ static void computeImportForFunction( // Since definition takes precedence over declaration for the same VI, // try emplace pair without checking insert result. // If insert doesn't happen, there must be an existing entry keyed by - // VI. Note `ExportLists` only keeps track of definitions so VI won't - // be inserted. + // VI. Note `ExportLists` only keeps track of exports due to imported + // definitions. ImportList[DeclSourceModule].try_emplace( VI.getGUID(), GlobalValueSummary::Declaration); } @@ -995,34 +995,36 @@ static bool isGlobalVarSummary(const ModuleSummaryIndex &Index, return false; } +// Return the number of global summaries and record the number of function +// summaries as output parameter. static unsigned numGlobalVarSummaries(const ModuleSummaryIndex &Index, FunctionImporter::ExportSetTy &ExportSet, - unsigned &DefinedGVS, unsigned &DefinedFS) { - DefinedGVS = 0; + unsigned NumGVS = 0; DefinedFS = 0; for (auto &VI : ExportSet) { - if (isGlobalVarSummary(Index, VI.getGUID())) { - ++DefinedGVS; - } else + if (isGlobalVarSummary(Index, VI.getGUID())) + ++NumGVS; + else ++DefinedFS; } - return DefinedGVS; + return NumGVS; } +// Return the number of global summaries and record the number of function +// summaries as output parameter. This is the same as `numGlobalVarSummaries` +// except that it takes `FunctionImporter::FunctionsToImportTy` as input +// parameter. static unsigned numGlobalVarSummaries(const ModuleSummaryIndex &Index, FunctionImporter::FunctionsToImportTy &ImportMap, - unsigned &DefinedGVS, unsigned &DefinedFS) { + unsigned &DefinedFS) { unsigned NumGVS = 0; - DefinedGVS = 0; DefinedFS = 0; for (auto &[GUID, Type] : ImportMap) { - if (isGlobalVarSummary(Index, GUID)) { - if (Type == GlobalValueSummary::Definition) - ++DefinedGVS; + if (isGlobalVarSummary(Index, GUID)) ++NumGVS; - } else if (Type == GlobalValueSummary::Definition) + else if (Type == GlobalValueSummary::Definition) ++DefinedFS; } return NumGVS; @@ -1115,23 +1117,14 @@ void llvm::ComputeCrossModuleImport( // we convert such variables initializers to "zeroinitializer". // See processGlobalForThinLTO. if (!Index.isWriteOnly(GVS)) - for (const auto &VI : GVS->refs()) { - // Try to emplace the declaration entry. If a definition entry - // already exists for key `VI`, this is a no-op. + for (const auto &VI : GVS->refs()) NewExports.insert(VI); - } } else { auto *FS = cast(S); - for (const auto &Edge : FS->calls()) { - // Try to emplace the declaration entry. If a definition entry - // already exists for key `VI`, this is a no-op. + for (const auto &Edge : FS->calls()) NewExports.insert(Edge.first); - } - for (const auto &Ref : FS->refs()) { - // Try to emplace the declaration entry. If a definition entry - // already exists for key `VI`, this is a no-op. + for (const auto &Ref : FS->refs()) NewExports.insert(Ref); - } } } // Prune list computed above to only include values defined in the @@ -1154,29 +1147,25 @@ void llvm::ComputeCrossModuleImport( for (auto &ModuleImports : ImportLists) { auto ModName = ModuleImports.first; auto &Exports = ExportLists[ModName]; - unsigned DefinedGVS = 0, DefinedFS = 0; - unsigned NumGVS = - numGlobalVarSummaries(Index, Exports, DefinedGVS, DefinedFS); + unsigned DefinedFS = 0; + unsigned NumGVS = numGlobalVarSummaries(Index, Exports, DefinedFS); LLVM_DEBUG(dbgs() << "* Module " << ModName << " exports " << DefinedFS << " function as definitions, " << Exports.size() - NumGVS - DefinedFS - << " functions as declarations, " << DefinedGVS - << " var definitions and " << NumGVS - DefinedGVS - << " var declarations. Imports from " - << ModuleImports.second.size() << " modules.\n"); + << " functions as declarations and " << NumGVS + << " vars. Imports from " << ModuleImports.second.size() + << " modules.\n"); for (auto &Src : ModuleImports.second) { auto SrcModName = Src.first; - unsigned DefinedGVS = 0, DefinedFS = 0; + unsigned DefinedFS = 0; unsigned NumGVSPerMod = - numGlobalVarSummaries(Index, Src.second, DefinedGVS, DefinedFS); + numGlobalVarSummaries(Index, Src.second, DefinedFS); LLVM_DEBUG(dbgs() << " - " << DefinedFS << " function definitions and " << Src.second.size() - NumGVSPerMod - DefinedFS << " function declarations imported from " << SrcModName << "\n"); - LLVM_DEBUG(dbgs() << " - " << DefinedGVS << " global vars definition and " - << NumGVSPerMod - DefinedGVS - << " global vars declaration imported from " - << SrcModName << "\n"); + LLVM_DEBUG(dbgs() << " - " << NumGVSPerMod + << " global vars imported from " << SrcModName << "\n"); } } #endif @@ -1190,17 +1179,14 @@ static void dumpImportListForModule(const ModuleSummaryIndex &Index, << ImportList.size() << " modules.\n"); for (auto &Src : ImportList) { auto SrcModName = Src.first; - unsigned DefinedGVS = 0, DefinedFS = 0; - unsigned NumGVSPerMod = - numGlobalVarSummaries(Index, Src.second, DefinedGVS, DefinedFS); + unsigned DefinedFS = 0; + unsigned NumGVSPerMod = numGlobalVarSummaries(Index, Src.second, DefinedFS); LLVM_DEBUG(dbgs() << " - " << DefinedFS << " function definitions and " << Src.second.size() - DefinedFS - NumGVSPerMod << " function declarations imported from " << SrcModName << "\n"); - LLVM_DEBUG(dbgs() << " - " << DefinedGVS << " var definitions and " - << NumGVSPerMod - DefinedGVS - << " var declarations imported from " << SrcModName - << "\n"); + LLVM_DEBUG(dbgs() << " - " << NumGVSPerMod << " vars imported from " + << SrcModName << "\n"); } } #endif diff --git a/llvm/test/ThinLTO/X86/funcimport-stats.ll b/llvm/test/ThinLTO/X86/funcimport-stats.ll index 7fcd33855fe1a..1c2fd092ccb49 100644 --- a/llvm/test/ThinLTO/X86/funcimport-stats.ll +++ b/llvm/test/ThinLTO/X86/funcimport-stats.ll @@ -10,7 +10,7 @@ ; RUN: cat %t4 | FileCheck %s ; CHECK: - [[NUM_FUNCS:[0-9]+]] function definitions and 0 function declarations imported from -; CHECK-NEXT: - [[NUM_VARS:[0-9]+]] global vars definition and 0 global vars declaration imported from +; CHECK-NEXT: - [[NUM_VARS:[0-9]+]] global vars imported from ; CHECK: [[NUM_FUNCS]] function-import - Number of functions imported in backend ; CHECK-NEXT: [[NUM_FUNCS]] function-import - Number of functions thin link decided to import diff --git a/llvm/test/Transforms/FunctionImport/funcimport.ll b/llvm/test/Transforms/FunctionImport/funcimport.ll index 635750b33fff0..8f7e8340d4909 100644 --- a/llvm/test/Transforms/FunctionImport/funcimport.ll +++ b/llvm/test/Transforms/FunctionImport/funcimport.ll @@ -167,7 +167,7 @@ declare void @variadic_va_start(...) ; DUMP: Module [[M1:.*]] imports from 1 module ; DUMP-NEXT: 15 function definitions and 0 function declarations imported from [[M2:.*]] -; DUMP-NEXT: 4 var definitions and 0 var declarations imported from [[M2]] +; DUMP-NEXT: 4 vars imported from [[M2]] ; DUMP: Imported 15 functions for Module [[M1]] ; DUMP-NEXT: Imported 4 global variables for Module [[M1]] From 805923adc45e086c8bbac87d07c77ec8f6b83c7e Mon Sep 17 00:00:00 2001 From: mingmingl Date: Wed, 3 Jul 2024 09:42:20 -0700 Subject: [PATCH 3/4] simplify numGlobalVarSummaries --- llvm/lib/Transforms/IPO/FunctionImport.cpp | 30 ++++++++-------------- 1 file changed, 10 insertions(+), 20 deletions(-) diff --git a/llvm/lib/Transforms/IPO/FunctionImport.cpp b/llvm/lib/Transforms/IPO/FunctionImport.cpp index b36fd210b28cd..288ee313bf10c 100644 --- a/llvm/lib/Transforms/IPO/FunctionImport.cpp +++ b/llvm/lib/Transforms/IPO/FunctionImport.cpp @@ -995,26 +995,19 @@ static bool isGlobalVarSummary(const ModuleSummaryIndex &Index, return false; } -// Return the number of global summaries and record the number of function -// summaries as output parameter. -static unsigned numGlobalVarSummaries(const ModuleSummaryIndex &Index, - FunctionImporter::ExportSetTy &ExportSet, - unsigned &DefinedFS) { +// Return the number of global summaries in ExportSet. +static unsigned +numGlobalVarSummaries(const ModuleSummaryIndex &Index, + FunctionImporter::ExportSetTy &ExportSet) { unsigned NumGVS = 0; - DefinedFS = 0; - for (auto &VI : ExportSet) { + for (auto &VI : ExportSet) if (isGlobalVarSummary(Index, VI.getGUID())) ++NumGVS; - else - ++DefinedFS; - } return NumGVS; } -// Return the number of global summaries and record the number of function -// summaries as output parameter. This is the same as `numGlobalVarSummaries` -// except that it takes `FunctionImporter::FunctionsToImportTy` as input -// parameter. +// Given ImportMap, return the number of global summaries and record the number +// of defined function summaries as output parameter. static unsigned numGlobalVarSummaries(const ModuleSummaryIndex &Index, FunctionImporter::FunctionsToImportTy &ImportMap, @@ -1147,12 +1140,9 @@ void llvm::ComputeCrossModuleImport( for (auto &ModuleImports : ImportLists) { auto ModName = ModuleImports.first; auto &Exports = ExportLists[ModName]; - unsigned DefinedFS = 0; - unsigned NumGVS = numGlobalVarSummaries(Index, Exports, DefinedFS); - LLVM_DEBUG(dbgs() << "* Module " << ModName << " exports " << DefinedFS - << " function as definitions, " - << Exports.size() - NumGVS - DefinedFS - << " functions as declarations and " << NumGVS + unsigned NumGVS = numGlobalVarSummaries(Index, Exports); + LLVM_DEBUG(dbgs() << "* Module " << ModName << " exports " + << Exports.size() - NumGVS << " functions and " << NumGVS << " vars. Imports from " << ModuleImports.second.size() << " modules.\n"); for (auto &Src : ModuleImports.second) { From 07d061b97958d0a257331f876a354d243d5d1712 Mon Sep 17 00:00:00 2001 From: mingmingl Date: Wed, 3 Jul 2024 11:50:58 -0700 Subject: [PATCH 4/4] resolve comments --- llvm/lib/Transforms/IPO/FunctionImport.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/llvm/lib/Transforms/IPO/FunctionImport.cpp b/llvm/lib/Transforms/IPO/FunctionImport.cpp index 288ee313bf10c..2b0ee46f489ca 100644 --- a/llvm/lib/Transforms/IPO/FunctionImport.cpp +++ b/llvm/lib/Transforms/IPO/FunctionImport.cpp @@ -995,7 +995,7 @@ static bool isGlobalVarSummary(const ModuleSummaryIndex &Index, return false; } -// Return the number of global summaries in ExportSet. +// Return the number of global variable summaries in ExportSet. static unsigned numGlobalVarSummaries(const ModuleSummaryIndex &Index, FunctionImporter::ExportSetTy &ExportSet) { @@ -1006,8 +1006,8 @@ numGlobalVarSummaries(const ModuleSummaryIndex &Index, return NumGVS; } -// Given ImportMap, return the number of global summaries and record the number -// of defined function summaries as output parameter. +// Given ImportMap, return the number of global variable summaries and record +// the number of defined function summaries as output parameter. static unsigned numGlobalVarSummaries(const ModuleSummaryIndex &Index, FunctionImporter::FunctionsToImportTy &ImportMap,