[PATCH] D149298: [nfc][thinlto] Handle global constant importing separately

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/FunctionImport.cpp:259
 
 static bool shouldImportGlobal(
     const ValueInfo &VI, const GVSummaryMapTy &DefinedGVSummaries,
----------------
Can we make this a member of GlobalsImporter to avoid having to pass in DefinedGVSummaries and isPrevailing?


================
Comment at: llvm/lib/Transforms/IPO/FunctionImport.cpp:283
 
-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) {
-  for (const auto &VI : Summary.refs()) {
-    if (!shouldImportGlobal(VI, DefinedGVSummaries, isPrevailing)) {
-      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 {
-      return GlobalValue::isLocalLinkage(RefSummary->linkage()) &&
-             RefSummary->modulePath() != Summary.modulePath();
-    };
+class GlobalsImporter final {
+  const ModuleSummaryIndex &Index;
----------------
Can you add a brief description in comment for class?


================
Comment at: llvm/lib/Transforms/IPO/FunctionImport.cpp:305
+  ~GlobalsImporter() {
+    while (!Worklist.empty())
+      onImportingSummary(*Worklist.pop_back_val());
----------------
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.


================
Comment at: llvm/lib/Transforms/IPO/FunctionImport.cpp:331
+
+      for (const auto &RefSummary : VI.getSummaryList())
+        if (const auto *GVS = dyn_cast<GlobalVarSummary>(RefSummary.get()))
----------------
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.


================
Comment at: llvm/lib/Transforms/IPO/FunctionImport.cpp:332
+      for (const auto &RefSummary : VI.getSummaryList())
+        if (const auto *GVS = dyn_cast<GlobalVarSummary>(RefSummary.get()))
+          if (Index.canImportGlobalVar(GVS, /* AnalyzeRefs */ true) &&
----------------
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D149298/new/

https://reviews.llvm.org/D149298