[llvm] [LTO] Introduce helper functions to add GUIDs to ImportList (NFC) (PR #105555)
Kazu Hirata via llvm-commits
llvm-commits at lists.llvm.org
Thu Aug 22 10:37:50 PDT 2024
https://github.com/kazutakahirata updated https://github.com/llvm/llvm-project/pull/105555
>From 8cd227cea9a7464feb71038de7059d25b9878b9f Mon Sep 17 00:00:00 2001
From: Kazu Hirata <kazu at google.com>
Date: Tue, 20 Aug 2024 23:01:48 -0700
Subject: [PATCH 1/3] [LTO] Introduce helper functions to add GUIDs to
ImportList (NFC)
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.
---
.../llvm/Transforms/IPO/FunctionImport.h | 27 +++++++++
llvm/lib/LTO/LTOBackend.cpp | 10 +---
llvm/lib/Transforms/IPO/FunctionImport.cpp | 58 ++++++++++---------
llvm/tools/llvm-link/llvm-link.cpp | 6 +-
4 files changed, 63 insertions(+), 38 deletions(-)
diff --git a/llvm/include/llvm/Transforms/IPO/FunctionImport.h b/llvm/include/llvm/Transforms/IPO/FunctionImport.h
index 6df597c300c180..7a26267320886e 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 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 6ae89a49b6b9a3..8fa85a1f66098a 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, Ins] =
+ ImportList[FromModule].try_emplace(GUID, GlobalValueSummary::Definition);
+ if (Ins)
+ return FunctionImporter::Inserted;
+ if (It->second == GlobalValueSummary::Definition)
+ return FunctionImporter::NoChange;
+ It->second = GlobalValueSummary::Definition;
+ return FunctionImporter::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::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
@@ -887,8 +902,8 @@ static void computeImportForFunction(
// If insert doesn't happen, there must be an existing entry keyed by
// VI. 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 +952,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::NoChange) {
NumImportedFunctionsThinLink++;
if (IsHotCallsite)
NumImportedHotFunctionsThinLink++;
@@ -950,9 +962,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 +1309,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));
>From 2eafb7bbe1b81c286b8e82762e1406b754b91fc1 Mon Sep 17 00:00:00 2001
From: Kazu Hirata <kazu at google.com>
Date: Wed, 21 Aug 2024 11:05:01 -0700
Subject: [PATCH 2/3] Use enum class for AddDefinitionStatus.
---
llvm/include/llvm/Transforms/IPO/FunctionImport.h | 2 +-
llvm/lib/Transforms/IPO/FunctionImport.cpp | 14 +++++++-------
2 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/llvm/include/llvm/Transforms/IPO/FunctionImport.h b/llvm/include/llvm/Transforms/IPO/FunctionImport.h
index 7a26267320886e..b5d46b8ec5e948 100644
--- a/llvm/include/llvm/Transforms/IPO/FunctionImport.h
+++ b/llvm/include/llvm/Transforms/IPO/FunctionImport.h
@@ -122,7 +122,7 @@ class FunctionImporter {
/// Import functions in Module \p M based on the supplied import list.
Expected<bool> importFunctions(Module &M, const ImportMapTy &ImportList);
- enum AddDefinitionStatus {
+ enum class AddDefinitionStatus {
NoChange,
Inserted,
ChangedToDefinition,
diff --git a/llvm/lib/Transforms/IPO/FunctionImport.cpp b/llvm/lib/Transforms/IPO/FunctionImport.cpp
index 8fa85a1f66098a..1bf4503d105bb0 100644
--- a/llvm/lib/Transforms/IPO/FunctionImport.cpp
+++ b/llvm/lib/Transforms/IPO/FunctionImport.cpp
@@ -337,14 +337,14 @@ using EdgeInfo = std::tuple<const FunctionSummary *, unsigned /* Threshold */>;
FunctionImporter::AddDefinitionStatus
FunctionImporter::addDefinition(ImportMapTy &ImportList, StringRef FromModule,
GlobalValue::GUID GUID) {
- auto [It, Ins] =
+ auto [It, Inserted] =
ImportList[FromModule].try_emplace(GUID, GlobalValueSummary::Definition);
- if (Ins)
- return FunctionImporter::Inserted;
+ if (Inserted)
+ return AddDefinitionStatus::Inserted;
if (It->second == GlobalValueSummary::Definition)
- return FunctionImporter::NoChange;
+ return AddDefinitionStatus::NoChange;
It->second = GlobalValueSummary::Definition;
- return FunctionImporter::ChangedToDefinition;
+ return AddDefinitionStatus::ChangedToDefinition;
}
void FunctionImporter::maybeAddDeclaration(ImportMapTy &ImportList,
@@ -413,7 +413,7 @@ class GlobalsImporter final {
// Otherwise, definition should take precedence over declaration.
if (FunctionImporter::addDefinition(
ImportList, RefSummary->modulePath(), VI.getGUID()) !=
- FunctionImporter::Inserted)
+ FunctionImporter::AddDefinitionStatus::Inserted)
break;
// Only update stat and exports if we haven't already imported this
@@ -954,7 +954,7 @@ static void computeImportForFunction(
// status.
if (FunctionImporter::addDefinition(ImportList, ExportModulePath,
VI.getGUID()) !=
- FunctionImporter::NoChange) {
+ FunctionImporter::AddDefinitionStatus::NoChange) {
NumImportedFunctionsThinLink++;
if (IsHotCallsite)
NumImportedHotFunctionsThinLink++;
>From 6453f601cd98d19faeb34409dbd287495564c7ea Mon Sep 17 00:00:00 2001
From: Kazu Hirata <kazu at google.com>
Date: Thu, 22 Aug 2024 10:36:38 -0700
Subject: [PATCH 3/3] Adjust a comment.
---
llvm/lib/Transforms/IPO/FunctionImport.cpp | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/llvm/lib/Transforms/IPO/FunctionImport.cpp b/llvm/lib/Transforms/IPO/FunctionImport.cpp
index 1bf4503d105bb0..82ceb0fa82c0d9 100644
--- a/llvm/lib/Transforms/IPO/FunctionImport.cpp
+++ b/llvm/lib/Transforms/IPO/FunctionImport.cpp
@@ -897,10 +897,7 @@ 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.
FunctionImporter::maybeAddDeclaration(ImportList, DeclSourceModule,
VI.getGUID());
More information about the llvm-commits
mailing list