[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/.

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