Skip to content

[LTO] Make getImportType a proper function (NFC) #106450

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

kazutakahirata
Copy link
Contributor

I'm planning to reduce the memory footprint of ThinLTO indexing by
changing ImportMapTy. A look-up of the import type will involve data
private to ImportMapTy, so it must be done by a member function of
ImportMapTy. This patch turns getImportType into a member function so
that a subsequent "real" change will just have to update the
implementation of the function in place.

I'm planning to reduce the memory footprint of ThinLTO indexing by
changing ImportMapTy.  A look-up of the import type will involve data
private to ImportMapTy, so it must be done by a member function of
ImportMapTy.  This patch turns getImportType into a member function so
that a subsequent "real" change will just have to update the
implementation of the function in place.
@llvmbot llvmbot added LTO Link time optimization (regular/full LTO or ThinLTO) llvm:transforms labels Aug 28, 2024
@kazutakahirata kazutakahirata requested a review from jvoung August 28, 2024 20:31
@llvmbot
Copy link
Member

llvmbot commented Aug 28, 2024

@llvm/pr-subscribers-lto

@llvm/pr-subscribers-llvm-transforms

Author: Kazu Hirata (kazutakahirata)

Changes

I'm planning to reduce the memory footprint of ThinLTO indexing by
changing ImportMapTy. A look-up of the import type will involve data
private to ImportMapTy, so it must be done by a member function of
ImportMapTy. This patch turns getImportType into a member function so
that a subsequent "real" change will just have to update the
implementation of the function in place.


Full diff: https://github.com/llvm/llvm-project/pull/106450.diff

2 Files Affected:

  • (modified) llvm/include/llvm/Transforms/IPO/FunctionImport.h (+4)
  • (modified) llvm/lib/Transforms/IPO/FunctionImport.cpp (+12-12)
diff --git a/llvm/include/llvm/Transforms/IPO/FunctionImport.h b/llvm/include/llvm/Transforms/IPO/FunctionImport.h
index 78932c12e76ff8..b7280c56be9cc8 100644
--- a/llvm/include/llvm/Transforms/IPO/FunctionImport.h
+++ b/llvm/include/llvm/Transforms/IPO/FunctionImport.h
@@ -143,6 +143,10 @@ class FunctionImporter {
     // order.
     SmallVector<StringRef, 0> getSourceModules() const;
 
+    std::optional<GlobalValueSummary::ImportKind>
+    getImportType(const FunctionsToImportTy &GUIDToImportType,
+                  GlobalValue::GUID GUID) const;
+
     const ImportMapTyImpl &getImportMap() const { return ImportMap; }
 
   private:
diff --git a/llvm/lib/Transforms/IPO/FunctionImport.cpp b/llvm/lib/Transforms/IPO/FunctionImport.cpp
index 6227b085f13a60..7a60ae51f02cb4 100644
--- a/llvm/lib/Transforms/IPO/FunctionImport.cpp
+++ b/llvm/lib/Transforms/IPO/FunctionImport.cpp
@@ -359,6 +359,15 @@ FunctionImporter::ImportMapTy::getSourceModules() const {
   return Modules;
 }
 
+std::optional<GlobalValueSummary::ImportKind>
+FunctionImporter::ImportMapTy::getImportType(
+    const FunctionsToImportTy &GUIDToImportType, GlobalValue::GUID GUID) const {
+  auto Iter = GUIDToImportType.find(GUID);
+  if (Iter == GUIDToImportType.end())
+    return std::nullopt;
+  return Iter->second;
+}
+
 /// Import globals referenced by a function or other globals that are being
 /// imported, if importing such global is possible.
 class GlobalsImporter final {
@@ -1800,15 +1809,6 @@ Expected<bool> FunctionImporter::importFunctions(
 
   IRMover Mover(DestModule);
 
-  auto getImportType = [&](const FunctionsToImportTy &GUIDToImportType,
-                           GlobalValue::GUID GUID)
-      -> std::optional<GlobalValueSummary::ImportKind> {
-    auto Iter = GUIDToImportType.find(GUID);
-    if (Iter == GUIDToImportType.end())
-      return std::nullopt;
-    return Iter->second;
-  };
-
   // Do the actual import of functions now, one Module at a time
   for (const auto &Name : ImportList.getSourceModules()) {
     // Get the module for the import
@@ -1835,7 +1835,7 @@ Expected<bool> FunctionImporter::importFunctions(
       if (!F.hasName())
         continue;
       auto GUID = F.getGUID();
-      auto MaybeImportType = getImportType(ImportGUIDs, GUID);
+      auto MaybeImportType = ImportList.getImportType(ImportGUIDs, GUID);
       bool ImportDefinition = MaybeImportType == GlobalValueSummary::Definition;
 
       LLVM_DEBUG(dbgs() << (MaybeImportType ? "Is" : "Not")
@@ -1871,7 +1871,7 @@ Expected<bool> FunctionImporter::importFunctions(
       if (!GV.hasName())
         continue;
       auto GUID = GV.getGUID();
-      auto MaybeImportType = getImportType(ImportGUIDs, GUID);
+      auto MaybeImportType = ImportList.getImportType(ImportGUIDs, GUID);
       bool ImportDefinition = MaybeImportType == GlobalValueSummary::Definition;
 
       LLVM_DEBUG(dbgs() << (MaybeImportType ? "Is" : "Not")
@@ -1891,7 +1891,7 @@ Expected<bool> FunctionImporter::importFunctions(
       if (!GA.hasName() || isa<GlobalIFunc>(GA.getAliaseeObject()))
         continue;
       auto GUID = GA.getGUID();
-      auto MaybeImportType = getImportType(ImportGUIDs, GUID);
+      auto MaybeImportType = ImportList.getImportType(ImportGUIDs, GUID);
       bool ImportDefinition = MaybeImportType == GlobalValueSummary::Definition;
 
       LLVM_DEBUG(dbgs() << (MaybeImportType ? "Is" : "Not")

@kazutakahirata kazutakahirata merged commit eb9c49c into llvm:main Aug 28, 2024
8 of 10 checks passed
@kazutakahirata kazutakahirata deleted the cleanup_thinlto_ImportMapTy_getImportType branch August 28, 2024 20:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
llvm:transforms LTO Link time optimization (regular/full LTO or ThinLTO)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants