[llvm] [LTO] Turn ImportMapTy into a proper class (NFC) (PR #105748)
via llvm-commits
llvm-commits at lists.llvm.org
Thu Aug 22 16:20:59 PDT 2024
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-lto
Author: Kazu Hirata (kazutakahirata)
<details>
<summary>Changes</summary>
This patch turns type alias ImportMapTy into a proper class to provide
a more intuitive interface like:
ImportList.addDefinition(...)
as opposed to:
FunctionImporter::addDefinition(ImportList, ...)
Also, this patch requires all non-const accesses to go through
addDefinition, maybeAddDeclaration, and addGUID while providing const
accesses via:
const ImportMapTyImpl &getImportMap() const { return ImportMap; }
I realize ImportMapTy may not be the best name as a class (maybe OK as
a type alias). I am not renaming ImportMapTy in this patch at least
because there are 47 mentions of ImportMapTy under llvm/.
---
Full diff: https://github.com/llvm/llvm-project/pull/105748.diff
5 Files Affected:
- (modified) llvm/include/llvm/Transforms/IPO/FunctionImport.h (+33-28)
- (modified) llvm/lib/LTO/LTO.cpp (+5-4)
- (modified) llvm/lib/LTO/LTOBackend.cpp (+1-2)
- (modified) llvm/lib/Transforms/IPO/FunctionImport.cpp (+28-32)
- (modified) llvm/tools/llvm-link/llvm-link.cpp (+2-3)
``````````diff
diff --git a/llvm/include/llvm/Transforms/IPO/FunctionImport.h b/llvm/include/llvm/Transforms/IPO/FunctionImport.h
index 5ab8c6d130b60a..d777d0310d4123 100644
--- a/llvm/include/llvm/Transforms/IPO/FunctionImport.h
+++ b/llvm/include/llvm/Transforms/IPO/FunctionImport.h
@@ -102,7 +102,39 @@ class FunctionImporter {
/// elsewhere, typically by the in-memory ModuleSummaryIndex the importing
/// decisions are made from (the module path for each summary is owned by the
/// index's module path string table).
- using ImportMapTy = DenseMap<StringRef, FunctionsToImportTy>;
+ class ImportMapTy {
+ public:
+ using ImportMapTyImpl = DenseMap<StringRef, FunctionsToImportTy>;
+
+ enum class AddDefinitionStatus {
+ NoChange,
+ Inserted,
+ ChangedToDefinition,
+ };
+
+ // Add the given GUID to ImportList as a definition. If the same GUID has
+ // been added as a declaration previously, that entry is overridden.
+ AddDefinitionStatus addDefinition(StringRef FromModule,
+ GlobalValue::GUID GUID);
+
+ // Add the given GUID to ImportList as a declaration. If the same GUID has
+ // been added as a definition previously, that entry takes precedence, and
+ // no change is made.
+ void maybeAddDeclaration(StringRef FromModule, GlobalValue::GUID GUID);
+
+ void addGUID(StringRef FromModule, GlobalValue::GUID GUID,
+ GlobalValueSummary::ImportKind ImportKind) {
+ if (ImportKind == GlobalValueSummary::Definition)
+ addDefinition(FromModule, GUID);
+ else
+ maybeAddDeclaration(FromModule, GUID);
+ }
+
+ const ImportMapTyImpl &getImportMap() const { return ImportMap; }
+
+ private:
+ ImportMapTyImpl ImportMap;
+ };
/// The set contains an entry for every global value that the module exports.
/// Depending on the user context, this container is allowed to contain
@@ -122,33 +154,6 @@ class FunctionImporter {
/// Import functions in Module \p M based on the supplied import list.
Expected<bool> importFunctions(Module &M, const ImportMapTy &ImportList);
- enum class AddDefinitionStatus {
- NoChange,
- Inserted,
- ChangedToDefinition,
- };
-
- // Add the given GUID to ImportList as a definition. If the same GUID has
- // been added as a declaration previously, that entry is overridden.
- static AddDefinitionStatus addDefinition(ImportMapTy &ImportList,
- StringRef FromModule,
- GlobalValue::GUID GUID);
-
- // Add the given GUID to ImportList as a declaration. If the same GUID has
- // been added as a definition previously, that entry takes precedence, and no
- // change is made.
- static void maybeAddDeclaration(ImportMapTy &ImportList, StringRef FromModule,
- GlobalValue::GUID GUID);
-
- static void addGUID(ImportMapTy &ImportList, StringRef FromModule,
- GlobalValue::GUID GUID,
- GlobalValueSummary::ImportKind ImportKind) {
- if (ImportKind == GlobalValueSummary::Definition)
- addDefinition(ImportList, FromModule, GUID);
- else
- maybeAddDeclaration(ImportList, FromModule, GUID);
- }
-
private:
/// The summaries index used to trigger importing.
const ModuleSummaryIndex &Index;
diff --git a/llvm/lib/LTO/LTO.cpp b/llvm/lib/LTO/LTO.cpp
index e5545860c329d4..ee0193344d5ac9 100644
--- a/llvm/lib/LTO/LTO.cpp
+++ b/llvm/lib/LTO/LTO.cpp
@@ -177,7 +177,8 @@ std::string llvm::computeLTOCacheKey(
// Include the hash for every module we import functions from. The set of
// imported symbols for each module may affect code generation and is
// sensitive to link order, so include that as well.
- using ImportMapIteratorTy = FunctionImporter::ImportMapTy::const_iterator;
+ using ImportMapIteratorTy =
+ FunctionImporter::ImportMapTy::ImportMapTyImpl::const_iterator;
struct ImportModule {
ImportMapIteratorTy ModIt;
const ModuleSummaryIndex::ModuleInfo *ModInfo;
@@ -191,10 +192,10 @@ std::string llvm::computeLTOCacheKey(
};
std::vector<ImportModule> ImportModulesVector;
- ImportModulesVector.reserve(ImportList.size());
+ ImportModulesVector.reserve(ImportList.getImportMap().size());
- for (ImportMapIteratorTy It = ImportList.begin(); It != ImportList.end();
- ++It) {
+ for (ImportMapIteratorTy It = ImportList.getImportMap().begin();
+ It != ImportList.getImportMap().end(); ++It) {
ImportModulesVector.push_back({It, Index.getModule(It->getFirst())});
}
// Order using module hash, to be both independent of module name and
diff --git a/llvm/lib/LTO/LTOBackend.cpp b/llvm/lib/LTO/LTOBackend.cpp
index ae46d946ae06a6..4e58cd369c3ac9 100644
--- a/llvm/lib/LTO/LTOBackend.cpp
+++ b/llvm/lib/LTO/LTOBackend.cpp
@@ -726,8 +726,7 @@ bool lto::initImportList(const Module &M,
if (Summary->modulePath() == M.getModuleIdentifier())
continue;
// Add an entry to provoke importing by thinBackend.
- FunctionImporter::addGUID(ImportList, Summary->modulePath(), GUID,
- Summary->importType());
+ ImportList.addGUID(Summary->modulePath(), GUID, Summary->importType());
}
}
return true;
diff --git a/llvm/lib/Transforms/IPO/FunctionImport.cpp b/llvm/lib/Transforms/IPO/FunctionImport.cpp
index 55803670071d16..b26c15b665b590 100644
--- a/llvm/lib/Transforms/IPO/FunctionImport.cpp
+++ b/llvm/lib/Transforms/IPO/FunctionImport.cpp
@@ -334,11 +334,11 @@ using EdgeInfo = std::tuple<const FunctionSummary *, unsigned /* Threshold */>;
} // anonymous namespace
-FunctionImporter::AddDefinitionStatus
-FunctionImporter::addDefinition(ImportMapTy &ImportList, StringRef FromModule,
- GlobalValue::GUID GUID) {
+FunctionImporter::ImportMapTy::AddDefinitionStatus
+FunctionImporter::ImportMapTy::addDefinition(StringRef FromModule,
+ GlobalValue::GUID GUID) {
auto [It, Inserted] =
- ImportList[FromModule].try_emplace(GUID, GlobalValueSummary::Definition);
+ ImportMap[FromModule].try_emplace(GUID, GlobalValueSummary::Definition);
if (Inserted)
return AddDefinitionStatus::Inserted;
if (It->second == GlobalValueSummary::Definition)
@@ -347,10 +347,9 @@ FunctionImporter::addDefinition(ImportMapTy &ImportList, StringRef FromModule,
return AddDefinitionStatus::ChangedToDefinition;
}
-void FunctionImporter::maybeAddDeclaration(ImportMapTy &ImportList,
- StringRef FromModule,
- GlobalValue::GUID GUID) {
- ImportList[FromModule].try_emplace(GUID, GlobalValueSummary::Declaration);
+void FunctionImporter::ImportMapTy::maybeAddDeclaration(
+ StringRef FromModule, GlobalValue::GUID GUID) {
+ ImportMap[FromModule].try_emplace(GUID, GlobalValueSummary::Declaration);
}
/// Import globals referenced by a function or other globals that are being
@@ -411,9 +410,8 @@ class GlobalsImporter final {
// If there isn't an entry for GUID, insert <GUID, Definition> pair.
// Otherwise, definition should take precedence over declaration.
- if (FunctionImporter::addDefinition(
- ImportList, RefSummary->modulePath(), VI.getGUID()) !=
- FunctionImporter::AddDefinitionStatus::Inserted)
+ if (ImportList.addDefinition(RefSummary->modulePath(), VI.getGUID()) !=
+ FunctionImporter::ImportMapTy::AddDefinitionStatus::Inserted)
break;
// Only update stat and exports if we haven't already imported this
@@ -600,8 +598,7 @@ class WorkloadImportsManager : public ModuleImportsManager {
LLVM_DEBUG(dbgs() << "[Workload][Including]" << VI.name() << " from "
<< ExportingModule << " : "
<< Function::getGUID(VI.name()) << "\n");
- FunctionImporter::addDefinition(ImportList, ExportingModule,
- VI.getGUID());
+ ImportList.addDefinition(ExportingModule, VI.getGUID());
GVI.onImportingSummary(*GVS);
if (ExportLists)
(*ExportLists)[ExportingModule].insert(VI);
@@ -899,8 +896,7 @@ static void computeImportForFunction(
// Note `ExportLists` only keeps track of exports due to imported
// definitions.
- FunctionImporter::maybeAddDeclaration(ImportList, DeclSourceModule,
- VI.getGUID());
+ ImportList.maybeAddDeclaration(DeclSourceModule, VI.getGUID());
}
// Update with new larger threshold if this was a retry (otherwise
// we would have already inserted with NewThreshold above). Also
@@ -949,9 +945,8 @@ static void computeImportForFunction(
// Try emplace the definition entry, and update stats based on insertion
// status.
- if (FunctionImporter::addDefinition(ImportList, ExportModulePath,
- VI.getGUID()) !=
- FunctionImporter::AddDefinitionStatus::NoChange) {
+ if (ImportList.addDefinition(ExportModulePath, VI.getGUID()) !=
+ FunctionImporter::ImportMapTy::AddDefinitionStatus::NoChange) {
NumImportedFunctionsThinLink++;
if (IsHotCallsite)
NumImportedHotFunctionsThinLink++;
@@ -1084,7 +1079,7 @@ numGlobalVarSummaries(const ModuleSummaryIndex &Index,
// the number of defined function summaries as output parameter.
static unsigned
numGlobalVarSummaries(const ModuleSummaryIndex &Index,
- FunctionImporter::FunctionsToImportTy &ImportMap,
+ const FunctionImporter::FunctionsToImportTy &ImportMap,
unsigned &DefinedFS) {
unsigned NumGVS = 0;
DefinedFS = 0;
@@ -1105,9 +1100,9 @@ static bool checkVariableImport(
DenseMap<StringRef, FunctionImporter::ExportSetTy> &ExportLists) {
DenseSet<GlobalValue::GUID> FlattenedImports;
- for (auto &ImportPerModule : ImportLists)
- for (auto &ExportPerModule : ImportPerModule.second)
- for (auto &[GUID, Type] : ExportPerModule.second)
+ for (const auto &ImportPerModule : ImportLists)
+ for (const auto &ExportPerModule : ImportPerModule.second.getImportMap())
+ for (const auto &[GUID, Type] : ExportPerModule.second)
FlattenedImports.insert(GUID);
// Checks that all GUIDs of read/writeonly vars we see in export lists
@@ -1217,9 +1212,10 @@ void llvm::ComputeCrossModuleImport(
unsigned NumGVS = numGlobalVarSummaries(Index, Exports);
LLVM_DEBUG(dbgs() << "* Module " << ModName << " exports "
<< Exports.size() - NumGVS << " functions and " << NumGVS
- << " vars. Imports from " << ModuleImports.second.size()
+ << " vars. Imports from "
+ << ModuleImports.second.getImportMap().size()
<< " modules.\n");
- for (auto &Src : ModuleImports.second) {
+ for (const auto &Src : ModuleImports.second.getImportMap()) {
auto SrcModName = Src.first;
unsigned DefinedFS = 0;
unsigned NumGVSPerMod =
@@ -1240,8 +1236,8 @@ static void dumpImportListForModule(const ModuleSummaryIndex &Index,
StringRef ModulePath,
FunctionImporter::ImportMapTy &ImportList) {
LLVM_DEBUG(dbgs() << "* Module " << ModulePath << " imports from "
- << ImportList.size() << " modules.\n");
- for (auto &Src : ImportList) {
+ << ImportList.getImportMap().size() << " modules.\n");
+ for (const auto &Src : ImportList.getImportMap()) {
auto SrcModName = Src.first;
unsigned DefinedFS = 0;
unsigned NumGVSPerMod = numGlobalVarSummaries(Index, Src.second, DefinedFS);
@@ -1306,8 +1302,7 @@ static void ComputeCrossModuleImportForModuleFromIndexForTest(
if (Summary->modulePath() == ModulePath)
continue;
// Add an entry to provoke importing by thinBackend.
- FunctionImporter::addGUID(ImportList, Summary->modulePath(), GUID,
- Summary->importType());
+ ImportList.addGUID(Summary->modulePath(), GUID, Summary->importType());
}
#ifndef NDEBUG
dumpImportListForModule(Index, ModulePath, ImportList);
@@ -1496,7 +1491,7 @@ void llvm::gatherImportedSummariesForModule(
ModuleToSummariesForIndex[std::string(ModulePath)] =
ModuleToDefinedGVSummaries.lookup(ModulePath);
// Include summaries for imports.
- for (const auto &ILI : ImportList) {
+ for (const auto &ILI : ImportList.getImportMap()) {
auto &SummariesForIndex = ModuleToSummariesForIndex[std::string(ILI.first)];
const auto &DefinedGVSummaries =
@@ -1777,7 +1772,7 @@ Expected<bool> FunctionImporter::importFunctions(
IRMover Mover(DestModule);
// Do the actual import of functions now, one Module at a time
std::set<StringRef> ModuleNameOrderedList;
- for (const auto &FunctionsToImportPerModule : ImportList) {
+ for (const auto &FunctionsToImportPerModule : ImportList.getImportMap()) {
ModuleNameOrderedList.insert(FunctionsToImportPerModule.first);
}
@@ -1792,8 +1787,9 @@ Expected<bool> FunctionImporter::importFunctions(
for (const auto &Name : ModuleNameOrderedList) {
// Get the module for the import
- const auto &FunctionsToImportPerModule = ImportList.find(Name);
- assert(FunctionsToImportPerModule != ImportList.end());
+ const auto &FunctionsToImportPerModule =
+ ImportList.getImportMap().find(Name);
+ assert(FunctionsToImportPerModule != ImportList.getImportMap().end());
Expected<std::unique_ptr<Module>> SrcModuleOrErr = ModuleLoader(Name);
if (!SrcModuleOrErr)
return SrcModuleOrErr.takeError();
diff --git a/llvm/tools/llvm-link/llvm-link.cpp b/llvm/tools/llvm-link/llvm-link.cpp
index ef6f85d38fede6..54d38a6ddd6f10 100644
--- a/llvm/tools/llvm-link/llvm-link.cpp
+++ b/llvm/tools/llvm-link/llvm-link.cpp
@@ -381,9 +381,8 @@ static bool importFunctions(const char *argv0, Module &DestModule) {
// definition, so make the import type definition directly.
// FIXME: A follow-up patch should add test coverage for import declaration
// in `llvm-link` CLI (e.g., by introducing a new command line option).
- FunctionImporter::addDefinition(
- ImportList, FileNameStringCache.insert(FileName).first->getKey(),
- F->getGUID());
+ ImportList.addDefinition(
+ FileNameStringCache.insert(FileName).first->getKey(), F->getGUID());
}
auto CachedModuleLoader = [&](StringRef Identifier) {
return ModuleLoaderCache.takeModule(std::string(Identifier));
``````````
</details>
https://github.com/llvm/llvm-project/pull/105748
More information about the llvm-commits
mailing list