[LTO] Turn ImportMapTy into a proper class (NFC) (llvm#105748)

This patch turns type alias ImportMapTy into a proper class to provide
a more intuitive interface like:

  ImportList.addDefinition(...)

as opposed to:

  FunctionImporter::addDefinition(ImportList, ...)

Also, this patch requires all non-const accesses to go through
addDefinition, maybeAddDeclaration, and addGUID while providing const
accesses via:

  const ImportMapTyImpl &getImportMap() const { return ImportMap; }

I realize ImportMapTy may not be the best name as a class (maybe OK as
a type alias).  I am not renaming ImportMapTy in this patch at least
because there are 47 mentions of ImportMapTy under llvm/.

Author: kazutakahirata

llvm/include/llvm/Transforms/IPO/FunctionImport.h

@@ -96,13 +96,54 @@ class FunctionImporter {
                std::tuple<unsigned, const GlobalValueSummary *,
                           std::unique_ptr<ImportFailureInfo>>>;
 
-  /// The map contains an entry for every module to import from, the key being
-  /// the module identifier to pass to the ModuleLoader. The value is the set of
-  /// functions to import. The module identifier strings must be owned
-  /// elsewhere, typically by the in-memory ModuleSummaryIndex the importing
-  /// decisions are made from (the module path for each summary is owned by the
-  /// index's module path string table).
-  using ImportMapTy = DenseMap<StringRef, FunctionsToImportTy>;
+  /// The map maintains the list of imports.  Conceptually, it is a collection
+  /// of tuples of the form:
+  ///
+  ///   (The name of the source module, GUID, Definition/Declaration)
+  ///
+  /// The name of the source module is the module identifier to pass to the
+  /// ModuleLoader.  The module identifier strings must be owned elsewhere,
+  /// typically by the in-memory ModuleSummaryIndex the importing decisions are
+  /// made from (the module path for each summary is owned by the index's module
+  /// path string table).
+  class ImportMapTy {
+  public:
+    using ImportMapTyImpl = DenseMap<StringRef, FunctionsToImportTy>;
+
+    enum class AddDefinitionStatus {
+      // No change was made to the list of imports or whether each import should
+      // be imported as a declaration or definition.
+      NoChange,
+      // Successfully added the given GUID to be imported as a definition. There
+      // was no existing entry with the same GUID as a declaration.
+      Inserted,
+      // An existing with the given GUID was changed to a definition.
+      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.
+    AddDefinitionStatus addDefinition(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.
+    void maybeAddDeclaration(StringRef FromModule, GlobalValue::GUID GUID);
+
+    void addGUID(StringRef FromModule, GlobalValue::GUID GUID,
+                 GlobalValueSummary::ImportKind ImportKind) {
+      if (ImportKind == GlobalValueSummary::Definition)
+        addDefinition(FromModule, GUID);
+      else
+        maybeAddDeclaration(FromModule, GUID);
+    }
+
+    const ImportMapTyImpl &getImportMap() const { return ImportMap; }
+
+  private:
+    ImportMapTyImpl ImportMap;
+  };
 
   /// The set contains an entry for every global value that the module exports.
   /// Depending on the user context, this container is allowed to contain
@@ -122,33 +163,6 @@ 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/LTO.cpp

@@ -177,7 +177,8 @@ std::string llvm::computeLTOCacheKey(
   // Include the hash for every module we import functions from. The set of
   // imported symbols for each module may affect code generation and is
   // sensitive to link order, so include that as well.
-  using ImportMapIteratorTy = FunctionImporter::ImportMapTy::const_iterator;
+  using ImportMapIteratorTy =
+      FunctionImporter::ImportMapTy::ImportMapTyImpl::const_iterator;
   struct ImportModule {
     ImportMapIteratorTy ModIt;
     const ModuleSummaryIndex::ModuleInfo *ModInfo;
@@ -191,10 +192,10 @@ std::string llvm::computeLTOCacheKey(
   };
 
   std::vector<ImportModule> ImportModulesVector;
-  ImportModulesVector.reserve(ImportList.size());
+  ImportModulesVector.reserve(ImportList.getImportMap().size());
 
-  for (ImportMapIteratorTy It = ImportList.begin(); It != ImportList.end();
-       ++It) {
+  for (ImportMapIteratorTy It = ImportList.getImportMap().begin();
+       It != ImportList.getImportMap().end(); ++It) {
     ImportModulesVector.push_back({It, Index.getModule(It->getFirst())});
   }
   // Order using module hash, to be both independent of module name and

llvm/lib/LTO/LTOBackend.cpp

@@ -726,8 +726,7 @@ bool lto::initImportList(const Module &M,
       if (Summary->modulePath() == M.getModuleIdentifier())
         continue;
       // Add an entry to provoke importing by thinBackend.
-      FunctionImporter::addGUID(ImportList, Summary->modulePath(), GUID,
-                                Summary->importType());
+      ImportList.addGUID(Summary->modulePath(), GUID, Summary->importType());
     }
   }
   return true;

llvm/lib/Transforms/IPO/FunctionImport.cpp

@@ -334,11 +334,11 @@ using EdgeInfo = std::tuple<const FunctionSummary *, unsigned /* Threshold */>;
 
 } // anonymous namespace
 
-FunctionImporter::AddDefinitionStatus
-FunctionImporter::addDefinition(ImportMapTy &ImportList, StringRef FromModule,
-                                GlobalValue::GUID GUID) {
+FunctionImporter::ImportMapTy::AddDefinitionStatus
+FunctionImporter::ImportMapTy::addDefinition(StringRef FromModule,
+                                             GlobalValue::GUID GUID) {
   auto [It, Inserted] =
-      ImportList[FromModule].try_emplace(GUID, GlobalValueSummary::Definition);
+      ImportMap[FromModule].try_emplace(GUID, GlobalValueSummary::Definition);
   if (Inserted)
     return AddDefinitionStatus::Inserted;
   if (It->second == GlobalValueSummary::Definition)
@@ -347,10 +347,9 @@ FunctionImporter::addDefinition(ImportMapTy &ImportList, StringRef FromModule,
   return AddDefinitionStatus::ChangedToDefinition;
 }
 
-void FunctionImporter::maybeAddDeclaration(ImportMapTy &ImportList,
-                                           StringRef FromModule,
-                                           GlobalValue::GUID GUID) {
-  ImportList[FromModule].try_emplace(GUID, GlobalValueSummary::Declaration);
+void FunctionImporter::ImportMapTy::maybeAddDeclaration(
+    StringRef FromModule, GlobalValue::GUID GUID) {
+  ImportMap[FromModule].try_emplace(GUID, GlobalValueSummary::Declaration);
 }
 
 /// Import globals referenced by a function or other globals that are being
@@ -411,9 +410,8 @@ class GlobalsImporter final {
 
         // If there isn't an entry for GUID, insert <GUID, Definition> pair.
         // Otherwise, definition should take precedence over declaration.
-        if (FunctionImporter::addDefinition(
-                ImportList, RefSummary->modulePath(), VI.getGUID()) !=
-            FunctionImporter::AddDefinitionStatus::Inserted)
+        if (ImportList.addDefinition(RefSummary->modulePath(), VI.getGUID()) !=
+            FunctionImporter::ImportMapTy::AddDefinitionStatus::Inserted)
           break;
 
         // Only update stat and exports if we haven't already imported this
@@ -600,8 +598,7 @@ class WorkloadImportsManager : public ModuleImportsManager {
       LLVM_DEBUG(dbgs() << "[Workload][Including]" << VI.name() << " from "
                         << ExportingModule << " : "
                         << Function::getGUID(VI.name()) << "\n");
-      FunctionImporter::addDefinition(ImportList, ExportingModule,
-                                      VI.getGUID());
+      ImportList.addDefinition(ExportingModule, VI.getGUID());
       GVI.onImportingSummary(*GVS);
       if (ExportLists)
         (*ExportLists)[ExportingModule].insert(VI);
@@ -899,8 +896,7 @@ static void computeImportForFunction(
 
           // Note `ExportLists` only keeps track of exports due to imported
           // definitions.
-          FunctionImporter::maybeAddDeclaration(ImportList, DeclSourceModule,
-                                                VI.getGUID());
+          ImportList.maybeAddDeclaration(DeclSourceModule, VI.getGUID());
         }
         // Update with new larger threshold if this was a retry (otherwise
         // we would have already inserted with NewThreshold above). Also
@@ -949,9 +945,8 @@ static void computeImportForFunction(
 
       // Try emplace the definition entry, and update stats based on insertion
       // status.
-      if (FunctionImporter::addDefinition(ImportList, ExportModulePath,
-                                          VI.getGUID()) !=
-          FunctionImporter::AddDefinitionStatus::NoChange) {
+      if (ImportList.addDefinition(ExportModulePath, VI.getGUID()) !=
+          FunctionImporter::ImportMapTy::AddDefinitionStatus::NoChange) {
         NumImportedFunctionsThinLink++;
         if (IsHotCallsite)
           NumImportedHotFunctionsThinLink++;
@@ -1084,7 +1079,7 @@ numGlobalVarSummaries(const ModuleSummaryIndex &Index,
 // the number of defined function summaries as output parameter.
 static unsigned
 numGlobalVarSummaries(const ModuleSummaryIndex &Index,
-                      FunctionImporter::FunctionsToImportTy &ImportMap,
+                      const FunctionImporter::FunctionsToImportTy &ImportMap,
                       unsigned &DefinedFS) {
   unsigned NumGVS = 0;
   DefinedFS = 0;
@@ -1105,9 +1100,9 @@ static bool checkVariableImport(
     DenseMap<StringRef, FunctionImporter::ExportSetTy> &ExportLists) {
   DenseSet<GlobalValue::GUID> FlattenedImports;
 
-  for (auto &ImportPerModule : ImportLists)
-    for (auto &ExportPerModule : ImportPerModule.second)
-      for (auto &[GUID, Type] : ExportPerModule.second)
+  for (const auto &ImportPerModule : ImportLists)
+    for (const auto &ExportPerModule : ImportPerModule.second.getImportMap())
+      for (const auto &[GUID, Type] : ExportPerModule.second)
         FlattenedImports.insert(GUID);
 
   // Checks that all GUIDs of read/writeonly vars we see in export lists
@@ -1217,9 +1212,10 @@ void llvm::ComputeCrossModuleImport(
     unsigned NumGVS = numGlobalVarSummaries(Index, Exports);
     LLVM_DEBUG(dbgs() << "* Module " << ModName << " exports "
                       << Exports.size() - NumGVS << " functions and " << NumGVS
-                      << " vars. Imports from " << ModuleImports.second.size()
+                      << " vars. Imports from "
+                      << ModuleImports.second.getImportMap().size()
                       << " modules.\n");
-    for (auto &Src : ModuleImports.second) {
+    for (const auto &Src : ModuleImports.second.getImportMap()) {
       auto SrcModName = Src.first;
       unsigned DefinedFS = 0;
       unsigned NumGVSPerMod =
@@ -1240,8 +1236,8 @@ static void dumpImportListForModule(const ModuleSummaryIndex &Index,
                                     StringRef ModulePath,
                                     FunctionImporter::ImportMapTy &ImportList) {
   LLVM_DEBUG(dbgs() << "* Module " << ModulePath << " imports from "
-                    << ImportList.size() << " modules.\n");
-  for (auto &Src : ImportList) {
+                    << ImportList.getImportMap().size() << " modules.\n");
+  for (const auto &Src : ImportList.getImportMap()) {
     auto SrcModName = Src.first;
     unsigned DefinedFS = 0;
     unsigned NumGVSPerMod = numGlobalVarSummaries(Index, Src.second, DefinedFS);
@@ -1306,8 +1302,7 @@ static void ComputeCrossModuleImportForModuleFromIndexForTest(
     if (Summary->modulePath() == ModulePath)
       continue;
     // Add an entry to provoke importing by thinBackend.
-    FunctionImporter::addGUID(ImportList, Summary->modulePath(), GUID,
-                              Summary->importType());
+    ImportList.addGUID(Summary->modulePath(), GUID, Summary->importType());
   }
 #ifndef NDEBUG
   dumpImportListForModule(Index, ModulePath, ImportList);
@@ -1496,7 +1491,7 @@ void llvm::gatherImportedSummariesForModule(
   ModuleToSummariesForIndex[std::string(ModulePath)] =
       ModuleToDefinedGVSummaries.lookup(ModulePath);
   // Include summaries for imports.
-  for (const auto &ILI : ImportList) {
+  for (const auto &ILI : ImportList.getImportMap()) {
     auto &SummariesForIndex = ModuleToSummariesForIndex[std::string(ILI.first)];
 
     const auto &DefinedGVSummaries =
@@ -1777,7 +1772,7 @@ Expected<bool> FunctionImporter::importFunctions(
   IRMover Mover(DestModule);
   // Do the actual import of functions now, one Module at a time
   std::set<StringRef> ModuleNameOrderedList;
-  for (const auto &FunctionsToImportPerModule : ImportList) {
+  for (const auto &FunctionsToImportPerModule : ImportList.getImportMap()) {
     ModuleNameOrderedList.insert(FunctionsToImportPerModule.first);
   }
 
@@ -1792,8 +1787,9 @@ Expected<bool> FunctionImporter::importFunctions(
 
   for (const auto &Name : ModuleNameOrderedList) {
     // Get the module for the import
-    const auto &FunctionsToImportPerModule = ImportList.find(Name);
-    assert(FunctionsToImportPerModule != ImportList.end());
+    const auto &FunctionsToImportPerModule =
+        ImportList.getImportMap().find(Name);
+    assert(FunctionsToImportPerModule != ImportList.getImportMap().end());
     Expected<std::unique_ptr<Module>> SrcModuleOrErr = ModuleLoader(Name);
     if (!SrcModuleOrErr)
       return SrcModuleOrErr.takeError();

llvm/tools/llvm-link/llvm-link.cpp

@@ -381,9 +381,8 @@ 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).
-    FunctionImporter::addDefinition(
-        ImportList, FileNameStringCache.insert(FileName).first->getKey(),
-        F->getGUID());
+    ImportList.addDefinition(
+        FileNameStringCache.insert(FileName).first->getKey(), F->getGUID());
   }
   auto CachedModuleLoader = [&](StringRef Identifier) {
     return ModuleLoaderCache.takeModule(std::string(Identifier));