[llvm] [LTO] Introduce helper functions to add GUIDs to ImportList (NFC) (PR #105555)
via llvm-commits
llvm-commits at lists.llvm.org
Wed Aug 21 10:16:03 PDT 2024
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-llvm-transforms
@llvm/pr-subscribers-lto
Author: Kazu Hirata (kazutakahirata)
<details>
<summary>Changes</summary>
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.
---
Full diff: https://github.com/llvm/llvm-project/pull/105555.diff
4 Files Affected:
- (modified) llvm/include/llvm/Transforms/IPO/FunctionImport.h (+27)
- (modified) llvm/lib/LTO/LTOBackend.cpp (+2-8)
- (modified) llvm/lib/Transforms/IPO/FunctionImport.cpp (+31-27)
- (modified) llvm/tools/llvm-link/llvm-link.cpp (+3-3)
``````````diff
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));
``````````
</details>
https://github.com/llvm/llvm-project/pull/105555
More information about the llvm-commits
mailing list