[LTO] Introduce helper functions to add GUIDs to ImportList (NFC) (llvm#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.

Author: kazutakahirata

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;

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;

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,22 +949,16 @@ 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++;
         if (IsCriticalCallsite)
           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);

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));