[LTO] Turn ImportMapTy into a proper class (NFC) #105748
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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/.
llvmbot
added
LTO
|
@llvm/pr-subscribers-llvm-transforms @llvm/pr-subscribers-lto Author: Kazu Hirata (kazutakahirata) ChangesThis patch turns type alias ImportMapTy into a proper class to provide ImportList.addDefinition(...) as opposed to: FunctionImporter::addDefinition(ImportList, ...) Also, this patch requires all non-const accesses to go through const ImportMapTyImpl &getImportMap() const { return ImportMap; } I realize ImportMapTy may not be the best name as a class (maybe OK as Full diff: https://github.com/llvm/llvm-project/pull/105748.diff 5 Files Affected:
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));
|
mingmingl-llvm
approved these changes
Aug 23, 2024
| // 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); |
There was a problem hiding this comment.
nit: make these two private methods of class ImportMapTy
There was a problem hiding this comment.
I'd like to keep addDefinition and maybeAddDeclaration public methods so that the intent is clear at the call sites.
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/159/builds/4487 Here is the relevant piece of the build log for the reference: |
cjdb
pushed a commit
to cjdb/llvm-project
that referenced
this pull request
Aug 23, 2024
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/.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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/.