[PATCH] D43521: [ThinLTO] Compute synthetic function entry count

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

Sorry for the delay.



================
Comment at: include/llvm/IR/ModuleSummaryIndex.h:448
             /*NotEligibleToImport=*/true, /*Live=*/true, /*IsLocal=*/false),
-        0, FunctionSummary::FFlags{}, std::vector<ValueInfo>(),
+        0, FunctionSummary::FFlags{}, 0, std::vector<ValueInfo>(),
         std::move(Edges), std::vector<GlobalValue::GUID>(),
----------------
Document '0' parameter like you did elsewhere.


================
Comment at: lib/Analysis/ModuleSummaryAnalysis.cpp:469
                         F->returnDoesNotAlias()},
-                    ArrayRef<ValueInfo>{}, ArrayRef<FunctionSummary::EdgeTy>{},
+                    0, ArrayRef<ValueInfo>{},
+                    ArrayRef<FunctionSummary::EdgeTy>{},
----------------
Document '0' parameter like you did elsewhere.


================
Comment at: lib/Bitcode/Writer/BitcodeWriter.cpp:3747
+      CountIndex = 6;
+      NameVals.push_back(FS->entryCount());
+    }
----------------
eraman wrote:
> tejohnson wrote:
> > We read this value out of the 5th index unconditionally (or at least when the version is high enough) in BitcodeReader.cpp. Won't it cause an issue on the reader side if we only emit this (and adjust where the ref count is emitted) conditionally?
> > 
> > Regardless, suggest asserting in the writer here and in writePerModuleFunctionSummaryRecord when you expect the FS->entryCount() to be 0.
> Indeed this is a bug. I have changed it to write unconditionally. I don't quite follow the second part of your comment regarding the assert.
Nevermind the comment about the assert, looking again, I don't think it makes much sense.


================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:127
+    cl::desc("Attach 'synthetic_entry_count' metadata after "
+             "importing"));
+
----------------
Nit - does this need to be split into 2 lines? It looks like it would fit on one 80 col line.


================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:686
+  auto Root = Index.calculateCallGraphRoot();
+  // Root is a fake node. All successors of are the actual roots of the
+  // callgraph.
----------------
Nit: reads better without the "of" (i.e. just "All successors are the ...").


================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:729
+  // be propagated across the combined callgraph.
+  // SyntheticCountsUtils::propagate takes of this propagation on any callgraph
+  // that specialized GraphTraits.
----------------
Nit: comment needs a fix, i.e. should "takes of" be "takes on"? 


================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:975
           return std::move(Err);
+        auto *Summary = Index.findSummaryInModule(GUID, Name);
+        FunctionSummary *FS =
----------------
Similarly, this handling can be moved into renameModuleForThinLTO, called further below here, which already does some index-based updates for each imported global.  See the start of processGlobalForThinLTO, which does:
ValueInfo VI = ImportIndex.getValueInfo(GV.getGUID());

and the ValueInfo has a pointer to the summary entry. You can have it get the ValueInfo once and share it with your update, which can be done in that method.

You may want to add an interface to the ValueInfo to get the summary entry given a ModulePath (Name), since it only has an interface to return the list of summary entries currently.


================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:121
+static cl::opt<bool> EnableImportEntryCount(
+    "enable-import-entry-count", cl::init(false), cl::Hidden,
+    cl::desc("Attach 'synthetic_entry_count' metadata after "
----------------
eraman wrote:
> tejohnson wrote:
> > why off by default?
> I would like to enable this in the inliner, do some tuning and then turn this on by default. If you strongly prefer this to  be default now, I will do so.
Ok


================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:901
+    FunctionSummary *FS =
+        dyn_cast_or_null<FunctionSummary>(Index.findSummaryInModule(
+            F.getGUID(), DestModule.getModuleIdentifier()));
----------------
eraman wrote:
> tejohnson wrote:
> > Probably shouldn't do this during importing - setting on the functions already in the dest module isn't really related to importing anyway. Also, if done from the caller (i.e. see thinBackend(), we have a ready map of GUID->Summary, DefinedGlobals). Using that is much more efficient that looking up in the full Index hash map.
> thinBackend has the DefinedGlobalsMap, but not the other callers to this function. Could you suggest what is the best way to do this on all callers?
You only need to invoke this functionality from two places:
1) The new LTO API (thinBackend) - tested via llvm-lto2
2) The old C LTO API (ThinLTOCodeGenerator.cpp) - tested via llvm-lto
Both of these already have the DefinedGlobals map, because they invoke other Thin Link optimizations that require it. I.e. search for callers of thinLTOInternalizeModule, which is passed this map. And just add a call to this functionality, split out into a similar type of function, that takes the map. You'll note that for testing purposes, the old C API has the ability to call the thin link optimizations independent of the other optimizations, i.e. see ThinLTOCodeGenerator::internalize, and you may want to do something similar too (for testing via llvm-lto).


Repository:
  rL LLVM

https://reviews.llvm.org/D43521