[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