[nfc][thinlto] Handle global constant importing separately

[mtrofin]

Summary

This makes the logic for referenced globals reusable for import criteria
that don't use thresholds - in fact, we currently didn't consider any
thresholds when importing.

[tejohnson]

llvm/lib/Transforms/IPO/FunctionImport.cpp

258–259

Can we make this a member of GlobalsImporter to avoid having to pass in DefinedGVSummaries and isPrevailing?

291–299

Can you add a brief description in comment for class?

313

Some comments as to why we are doing this in the destructor would be helpful. But would it be clearer to just do this greedily from onImportingSummary? In fact, in that case we wouldn't even need a Worklist - onImportingSummary can just call itself recursively.

358

Nit: I think per LLVM coding standards the outer for and if should also have braces since the innermost if statement is nontrivial and has braces. It would improve readability imo.

359

Nit: prefer early continue if !GVS to avoid extra level of nesting. It might be nice to add a comment there about how we don't currently perform importing of any functions referenced by global variables (e.g. virtual functions in vtables). I suppose if we wanted to handle those in the future, we could arrange for this to return a set of those functions.

[mtrofin]

feedback

[mtrofin]

llvm/lib/Transforms/IPO/FunctionImport.cpp

313

Done, w/o recursion

[tejohnson]

lgtm

llvm/include/llvm/IR/ModuleSummaryIndex.h

public:
 
   /// Print out strongly connected components for debugging.
   void dumpSCCs(raw_ostream &OS);
 
   /// Do the access attribute and DSOLocal propagation in combined index.
   void propagateAttributes(const DenseSet<GlobalValue::GUID> &PreservedSymbols);
 
   /// Checks if we can import global variable from another module.
-  bool canImportGlobalVar(GlobalValueSummary *S, bool AnalyzeRefs) const;
+  bool canImportGlobalVar(const GlobalValueSummary *S, bool AnalyzeRefs) const;
 };
 
 /// GraphTraits definition to build SCC for the index
 template <> struct GraphTraits<ValueInfo> {
   typedef ValueInfo NodeRef;
   using EdgeRef = FunctionSummary::EdgeTy &;
 
   static NodeRef valueInfoFromEdge(FunctionSummary::EdgeTy &P) {

llvm/lib/IR/ModuleSummaryIndex.cpp

for (auto &P : *this)
           if (isGlobalValueLive(GVS)) {
             if (GVS->maybeReadOnly())
               ReadOnlyLiveGVars++;
             if (GVS->maybeWriteOnly())
               WriteOnlyLiveGVars++;
           }
 }
 
-bool ModuleSummaryIndex::canImportGlobalVar(GlobalValueSummary *S,
+bool ModuleSummaryIndex::canImportGlobalVar(const GlobalValueSummary *S,
                                             bool AnalyzeRefs) const {
   auto HasRefsPreventingImport = [this](const GlobalVarSummary *GVS) {
     // We don't analyze GV references during attribute propagation, so
     // GV with non-trivial initializer can be marked either read or
     // write-only.
     // Importing definiton of readonly GV with non-trivial initializer
     // allows us doing some extra optimizations (like converting indirect
     // calls to direct).

llvm/lib/Transforms/IPO/FunctionImport.cpp

for (auto QualifiedValue : QualifiedCandidates) {
 
     return Summary;
   }
   return nullptr;
 }
 
 namespace {
 
-using EdgeInfo =
-    std::tuple<const GlobalValueSummary *, unsigned /* Threshold */>;
+using EdgeInfo = std::tuple<const FunctionSummary *, unsigned /* Threshold */>;
 
 } // anonymous namespace
 
-static bool shouldImportGlobal(
-    const ValueInfo &VI, const GVSummaryMapTy &DefinedGVSummaries,
-    function_ref<bool(GlobalValue::GUID, const GlobalValueSummary *)>
-        isPrevailing) {
+/// Import globals referenced by a function or other globals that are being
+/// imported, if importing such global is possible.
+class GlobalsImporter final {
+  const ModuleSummaryIndex &Index;
+  const GVSummaryMapTy &DefinedGVSummaries;
+      IsPrevailing;
+  FunctionImporter::ImportMapTy &ImportList;
+  StringMap<FunctionImporter::ExportSetTy> *const ExportLists;
+
+  bool shouldImportGlobal(const ValueInfo &VI) {
     const auto &GVS = DefinedGVSummaries.find(VI.getGUID());
     if (GVS == DefinedGVSummaries.end())
       return true;
     // We should not skip import if the module contains a non-prevailing
-  // definition with interposable linkage type. This is required for correctness
-  // in the situation where there is a prevailing def available for import and
-  // marked read-only. In this case, the non-prevailing def will be converted to
-  // a declaration, while the prevailing one becomes internal, thus no
-  // definitions will be available for linking. In order to prevent undefined
-  // symbol link error, the prevailing definition must be imported.
+    // definition with interposable linkage type. This is required for
+    // correctness in the situation where there is a prevailing def available
+    // for import and marked read-only. In this case, the non-prevailing def
+    // will be converted to a declaration, while the prevailing one becomes
+    // internal, thus no definitions will be available for linking. In order to
+    // prevent undefined symbol link error, the prevailing definition must be
+    // imported.
     // FIXME: Consider adding a check that the suitable prevailing definition
     // exists and marked read-only.
     if (VI.getSummaryList().size() > 1 &&
         GlobalValue::isInterposableLinkage(GVS->second->linkage()) &&
-      !isPrevailing(VI.getGUID(), GVS->second))
+        !IsPrevailing(VI.getGUID(), GVS->second))
       return true;
 
     return false;
   }
 
-static void computeImportForReferencedGlobals(
-    const GlobalValueSummary &Summary, const ModuleSummaryIndex &Index,
-    const GVSummaryMapTy &DefinedGVSummaries,
-    function_ref<bool(GlobalValue::GUID, const GlobalValueSummary *)>
-        isPrevailing,
-    SmallVectorImpl<EdgeInfo> &Worklist,
-    FunctionImporter::ImportMapTy &ImportList,
-    StringMap<FunctionImporter::ExportSetTy> *ExportLists) {
+  void
+  onImportingSummaryImpl(const GlobalValueSummary &Summary,
+                         SmallVectorImpl<const GlobalVarSummary *> &Worklist) {
     for (const auto &VI : Summary.refs()) {
-    if (!shouldImportGlobal(VI, DefinedGVSummaries, isPrevailing)) {
+      if (!shouldImportGlobal(VI)) {
         LLVM_DEBUG(
             dbgs() << "Ref ignored! Target already in destination module.\n");
         continue;
       }
 
       LLVM_DEBUG(dbgs() << " ref -> " << VI << "\n");
 
       // If this is a local variable, make sure we import the copy
       // in the caller's module. The only time a local variable can
       // share an entry in the index is if there is a local with the same name
       // in another module that had the same source file name (in a different
       // directory), where each was compiled in their own directory so there
       // was not distinguishing path.
-    auto LocalNotInModule = [&](const GlobalValueSummary *RefSummary) -> bool {
+      auto LocalNotInModule =
+          [&](const GlobalValueSummary *RefSummary) -> bool {
         return GlobalValue::isLocalLinkage(RefSummary->linkage()) &&
                RefSummary->modulePath() != Summary.modulePath();
       };
 
-    for (const auto &RefSummary : VI.getSummaryList())
-      if (isa<GlobalVarSummary>(RefSummary.get()) &&
-          Index.canImportGlobalVar(RefSummary.get(), /* AnalyzeRefs */ true) &&
-          !LocalNotInModule(RefSummary.get())) {
+      for (const auto &RefSummary : VI.getSummaryList()) {
+        const auto *GVS = dyn_cast<GlobalVarSummary>(RefSummary.get());
+        // Functions could be referenced by global vars - e.g. a vtable; but we
+        // don't currently imagine a reason those would be imported here, rather
+        // than as part of the logic deciding which functions to import (i.e.
+        // based on profile information). Should we decide to handle them here,
+        // we can refactor accordingly at that time.
+        if (!GVS || !Index.canImportGlobalVar(GVS, /* AnalyzeRefs */ true) ||
+            LocalNotInModule(GVS))
+          continue;
         auto ILI = ImportList[RefSummary->modulePath()].insert(VI.getGUID());
         // Only update stat and exports if we haven't already imported this
         // variable.
         if (!ILI.second)
           break;
         NumImportedGlobalVarsThinLink++;
-        // Any references made by this variable will be marked exported later,
-        // in ComputeCrossModuleImport, after import decisions are complete,
-        // which is more efficient than adding them here.
+        // Any references made by this variable will be marked exported
+        // later, in ComputeCrossModuleImport, after import decisions are
+        // complete, which is more efficient than adding them here.
         if (ExportLists)
           (*ExportLists)[RefSummary->modulePath()].insert(VI);
 
         // If variable is not writeonly we attempt to recursively analyze
         // its references in order to import referenced constants.
-        if (!Index.isWriteOnly(cast<GlobalVarSummary>(RefSummary.get())))
-          Worklist.emplace_back(RefSummary.get(), 0);
+        if (!Index.isWriteOnly(GVS))
+          Worklist.emplace_back(GVS);
         break;
       }
     }
   }
 
+public:
+  GlobalsImporter(
+      const ModuleSummaryIndex &Index, const GVSummaryMapTy &DefinedGVSummaries,
+      function_ref<bool(GlobalValue::GUID, const GlobalValueSummary *)>
+          IsPrevailing,
+      FunctionImporter::ImportMapTy &ImportList,
+      StringMap<FunctionImporter::ExportSetTy> *ExportLists)
+      : Index(Index), DefinedGVSummaries(DefinedGVSummaries),
+        IsPrevailing(IsPrevailing), ImportList(ImportList),
+        ExportLists(ExportLists) {}
+
+  void onImportingSummary(const GlobalValueSummary &Summary) {
+    SmallVector<const GlobalVarSummary *, 128> Worklist;
+    onImportingSummaryImpl(Summary, Worklist);
+    while (!Worklist.empty())
+      onImportingSummaryImpl(*Worklist.pop_back_val(), Worklist);
+  }
+};
+
 static const char *
 getFailureName(FunctionImporter::ImportFailureReason Reason) {
   switch (Reason) {
   case FunctionImporter::ImportFailureReason::None:
     return "None";
   case FunctionImporter::ImportFailureReason::GlobalVar:
     return "GlobalVar";
   case FunctionImporter::ImportFailureReason::NotLive:

 /// Compute the list of functions to import for a given caller. Mark these
 /// imported functions and the symbols they reference in their source module as
 /// exported from their source module.
 static void computeImportForFunction(
     const FunctionSummary &Summary, const ModuleSummaryIndex &Index,
     const unsigned Threshold, const GVSummaryMapTy &DefinedGVSummaries,
     function_ref<bool(GlobalValue::GUID, const GlobalValueSummary *)>
         isPrevailing,
-    SmallVectorImpl<EdgeInfo> &Worklist,
+    SmallVectorImpl<EdgeInfo> &Worklist, GlobalsImporter &GVImporter,
     FunctionImporter::ImportMapTy &ImportList,
     StringMap<FunctionImporter::ExportSetTy> *ExportLists,
     FunctionImporter::ImportThresholdsTy &ImportThresholds) {
-  computeImportForReferencedGlobals(Summary, Index, DefinedGVSummaries,
-                                    isPrevailing, Worklist, ImportList,
-                                    ExportLists);
+  GVImporter.onImportingSummary(Summary);
   static int ImportCount = 0;
   for (const auto &Edge : Summary.calls()) {
     ValueInfo VI = Edge.first;
     LLVM_DEBUG(dbgs() << " edge -> " << VI << " Threshold:" << Threshold
                       << "\n");
 
     if (ImportCutoff >= 0 && ImportCount >= ImportCutoff) {
       LLVM_DEBUG(dbgs() << "ignored! import-cutoff value of " << ImportCutoff
static void ComputeImportForModule(
     function_ref<bool(GlobalValue::GUID, const GlobalValueSummary *)>
         isPrevailing,
     const ModuleSummaryIndex &Index, StringRef ModName,
     FunctionImporter::ImportMapTy &ImportList,
     StringMap<FunctionImporter::ExportSetTy> *ExportLists = nullptr) {
   // Worklist contains the list of function imported in this module, for which
   // we will analyse the callees and may import further down the callgraph.
   SmallVector<EdgeInfo, 128> Worklist;
+  GlobalsImporter GVI(Index, DefinedGVSummaries, isPrevailing, ImportList,
+                      ExportLists);
   FunctionImporter::ImportThresholdsTy ImportThresholds;
 
   // Populate the worklist with the import for the functions in the current
   // module
   for (const auto &GVSummary : DefinedGVSummaries) {
 #ifndef NDEBUG
     // FIXME: Change the GVSummaryMapTy to hold ValueInfo instead of GUID
     // so this map look up (and possibly others) can be avoided.
     auto VI = Index.getValueInfo(GVSummary.first);
 #endif
     if (!Index.isGlobalValueLive(GVSummary.second)) {
       LLVM_DEBUG(dbgs() << "Ignores Dead GUID: " << VI << "\n");
       continue;
     }
     auto *FuncSummary =
         dyn_cast<FunctionSummary>(GVSummary.second->getBaseObject());
     if (!FuncSummary)
       // Skip import for global variables
       continue;
     LLVM_DEBUG(dbgs() << "Initialize import for " << VI << "\n");
     computeImportForFunction(*FuncSummary, Index, ImportInstrLimit,
-                             DefinedGVSummaries, isPrevailing, Worklist,
+                             DefinedGVSummaries, isPrevailing, Worklist, GVI,
                              ImportList, ExportLists, ImportThresholds);
   }
 
   // Process the newly imported functions and add callees to the worklist.
   while (!Worklist.empty()) {
     auto GVInfo = Worklist.pop_back_val();
     auto *Summary = std::get<0>(GVInfo);
     auto Threshold = std::get<1>(GVInfo);
 
     if (auto *FS = dyn_cast<FunctionSummary>(Summary))
       computeImportForFunction(*FS, Index, Threshold, DefinedGVSummaries,
-                               isPrevailing, Worklist, ImportList, ExportLists,
-                               ImportThresholds);
-    else
-      computeImportForReferencedGlobals(*Summary, Index, DefinedGVSummaries,
-                                        isPrevailing, Worklist, ImportList,
-                                        ExportLists);
+                               isPrevailing, Worklist, GVI, ImportList,
+                               ExportLists, ImportThresholds);
   }
 
   // Print stats about functions considered but rejected for importing
   // when requested.
   if (PrintImportFailures) {
     dbgs() << "Missed imports into module " << ModName << "\n";
     for (auto &I : ImportThresholds) {
       auto &ProcessedThreshold = std::get<0>(I.second);