[llvm] 3082a38 - [LTO] Introduce helper functions to add GUIDs to ImportList (NFC) (#105555)
via llvm-commits
llvm-commits at lists.llvm.org
Thu Aug 22 12:06:51 PDT 2024
Author: Kazu Hirata
Date: 2024-08-22T12:06:47-07:00
New Revision: 3082a381f57ef2885c270f41f2955e08c79634c5
URL: https://github.com/llvm/llvm-project/commit/3082a381f57ef2885c270f41f2955e08c79634c5
DIFF: https://github.com/llvm/llvm-project/commit/3082a381f57ef2885c270f41f2955e08c79634c5.diff
LOG: [LTO] Introduce helper functions to add GUIDs to ImportList (NFC) (#105555)
The new helper functions make the intent clearer while hiding
implementation details, including how we handle previously added
entries. Note that:
- If we are adding a GUID as a GlobalValueSummary::Definition, then we
override a previously added GlobalValueSummary::Declaration entry
for the same GUID.
- If we are adding a GUID as a GlobalValueSummary::Declaration, then a
previously added GlobalValueSummary::Definition entry for the same
GUID takes precedence, and no change is made.
Added:
Modified:
llvm/include/llvm/Transforms/IPO/FunctionImport.h
llvm/lib/LTO/LTOBackend.cpp
llvm/lib/Transforms/IPO/FunctionImport.cpp
llvm/tools/llvm-link/llvm-link.cpp
Removed:
################################################################################
diff --git a/llvm/include/llvm/Transforms/IPO/FunctionImport.h b/llvm/include/llvm/Transforms/IPO/FunctionImport.h
index 5dad572532c8ae..5ab8c6d130b60a 100644
--- a/llvm/include/llvm/Transforms/IPO/FunctionImport.h
+++ b/llvm/include/llvm/Transforms/IPO/FunctionImport.h
@@ -122,6 +122,33 @@ 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/LTOBackend.cpp b/llvm/lib/LTO/LTOBackend.cpp
index effaed2d9bfb60..ae46d946ae06a6 100644
--- a/llvm/lib/LTO/LTOBackend.cpp
+++ b/llvm/lib/LTO/LTOBackend.cpp
@@ -726,14 +726,8 @@ bool lto::initImportList(const Module &M,
if (Summary->modulePath() == M.getModuleIdentifier())
continue;
// Add an entry to provoke importing by thinBackend.
- // Try emplace the entry first. If an entry with the same key already
- // exists, set the value to 'std::min(existing-value, new-value)' to make
- // sure a definition takes precedence over a declaration.
- auto [Iter, Inserted] = ImportList[Summary->modulePath()].try_emplace(
- GUID, Summary->importType());
-
- if (!Inserted)
- Iter->second = std::min(Iter->second, Summary->importType());
+ FunctionImporter::addGUID(ImportList, Summary->modulePath(), GUID,
+ Summary->importType());
}
}
return true;
diff --git a/llvm/lib/Transforms/IPO/FunctionImport.cpp b/llvm/lib/Transforms/IPO/FunctionImport.cpp
index 92371720e0eceb..354ad0fde092a7 100644
--- a/llvm/lib/Transforms/IPO/FunctionImport.cpp
+++ b/llvm/lib/Transforms/IPO/FunctionImport.cpp
@@ -334,6 +334,25 @@ using EdgeInfo = std::tuple<const FunctionSummary *, unsigned /* Threshold */>;
} // anonymous namespace
+FunctionImporter::AddDefinitionStatus
+FunctionImporter::addDefinition(ImportMapTy &ImportList, StringRef FromModule,
+ GlobalValue::GUID GUID) {
+ auto [It, Inserted] =
+ ImportList[FromModule].try_emplace(GUID, GlobalValueSummary::Definition);
+ if (Inserted)
+ return AddDefinitionStatus::Inserted;
+ if (It->second == GlobalValueSummary::Definition)
+ return AddDefinitionStatus::NoChange;
+ It->second = GlobalValueSummary::Definition;
+ return AddDefinitionStatus::ChangedToDefinition;
+}
+
+void FunctionImporter::maybeAddDeclaration(ImportMapTy &ImportList,
+ StringRef FromModule,
+ GlobalValue::GUID GUID) {
+ ImportList[FromModule].try_emplace(GUID, GlobalValueSummary::Declaration);
+}
+
/// Import globals referenced by a function or other globals that are being
/// imported, if importing such global is possible.
class GlobalsImporter final {
@@ -392,17 +411,13 @@ class GlobalsImporter final {
// If there isn't an entry for GUID, insert <GUID, Definition> pair.
// Otherwise, definition should take precedence over declaration.
- auto [Iter, Inserted] =
- ImportList[RefSummary->modulePath()].try_emplace(
- VI.getGUID(), GlobalValueSummary::Definition);
+ if (FunctionImporter::addDefinition(
+ ImportList, RefSummary->modulePath(), VI.getGUID()) !=
+ FunctionImporter::AddDefinitionStatus::Inserted)
+ break;
+
// Only update stat and exports if we haven't already imported this
// variable.
- if (!Inserted) {
- // Set the value to 'std::min(existing-value, new-value)' to make
- // sure a definition takes precedence over a declaration.
- Iter->second = std::min(GlobalValueSummary::Definition, Iter->second);
- break;
- }
NumImportedGlobalVarsThinLink++;
// Any references made by this variable will be marked exported
// later, in ComputeCrossModuleImport, after import decisions are
@@ -882,13 +897,10 @@ static void computeImportForFunction(
if (ImportDeclaration && SummaryForDeclImport) {
StringRef DeclSourceModule = SummaryForDeclImport->modulePath();
- // Since definition takes precedence over declaration for the same VI,
- // try emplace <VI, declaration> 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 exports due to imported
+ // Note `ExportLists` only keeps track of exports due to imported
// definitions.
- ImportList[DeclSourceModule].try_emplace(
- VI.getGUID(), GlobalValueSummary::Declaration);
+ FunctionImporter::maybeAddDeclaration(ImportList, DeclSourceModule,
+ VI.getGUID());
}
// Update with new larger threshold if this was a retry (otherwise
// we would have already inserted with NewThreshold above). Also
@@ -937,12 +949,9 @@ static void computeImportForFunction(
// Try emplace the definition entry, and update stats based on insertion
// status.
- auto [Iter, Inserted] = ImportList[ExportModulePath].try_emplace(
- VI.getGUID(), GlobalValueSummary::Definition);
-
- // We previously decided to import this GUID definition if it was already
- // inserted in the set of imports from the exporting module.
- if (Inserted || Iter->second == GlobalValueSummary::Declaration) {
+ if (FunctionImporter::addDefinition(ImportList, ExportModulePath,
+ VI.getGUID()) !=
+ FunctionImporter::AddDefinitionStatus::NoChange) {
NumImportedFunctionsThinLink++;
if (IsHotCallsite)
NumImportedHotFunctionsThinLink++;
@@ -950,9 +959,6 @@ static void computeImportForFunction(
NumImportedCriticalFunctionsThinLink++;
}
- if (Iter->second == GlobalValueSummary::Declaration)
- Iter->second = GlobalValueSummary::Definition;
-
// Any calls/references made by this function will be marked exported
// later, in ComputeCrossModuleImport, after import decisions are
// complete, which is more efficient than adding them here.
@@ -1300,13 +1306,8 @@ static void ComputeCrossModuleImportForModuleFromIndexForTest(
if (Summary->modulePath() == ModulePath)
continue;
// Add an entry to provoke importing by thinBackend.
- auto [Iter, Inserted] = ImportList[Summary->modulePath()].try_emplace(
- GUID, Summary->importType());
- if (!Inserted) {
- // Use 'std::min' to make sure definition (with enum value 0) takes
- // precedence over declaration (with enum value 1).
- Iter->second = std::min(Iter->second, Summary->importType());
- }
+ FunctionImporter::addGUID(ImportList, Summary->modulePath(), GUID,
+ Summary->importType());
}
#ifndef NDEBUG
dumpImportListForModule(Index, ModulePath, ImportList);
diff --git a/llvm/tools/llvm-link/llvm-link.cpp b/llvm/tools/llvm-link/llvm-link.cpp
index b311820ce58709..ef6f85d38fede6 100644
--- a/llvm/tools/llvm-link/llvm-link.cpp
+++ b/llvm/tools/llvm-link/llvm-link.cpp
@@ -381,9 +381,9 @@ 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).
- auto &Entry =
- ImportList[FileNameStringCache.insert(FileName).first->getKey()];
- Entry[F->getGUID()] = GlobalValueSummary::Definition;
+ FunctionImporter::addDefinition(
+ ImportList, FileNameStringCache.insert(FileName).first->getKey(),
+ F->getGUID());
}
auto CachedModuleLoader = [&](StringRef Identifier) {
return ModuleLoaderCache.takeModule(std::string(Identifier));
More information about the llvm-commits
mailing list