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

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added inline comments.


================
Comment at: lib/Analysis/ModuleSummaryAnalysis.cpp:479
                 llvm::make_unique<FunctionSummary>(
-                    GVFlags, 0,
+                    GVFlags, /*InsCount=*/0,
                     FunctionSummary::FFlags{
----------------
nit: s/InsCount/InstCount/
Thanks for adding the documentation for these existing parameters too!


================
Comment at: lib/LTO/LTO.cpp:1160
 
+  computeSyntheticCounts(ThinLTO.CombinedIndex);
+
----------------
Add brief comment about what this does.


================
Comment at: lib/LTO/ThinLTOCodeGenerator.cpp:203
+                      const FunctionImporter::ImportMapTy &ImportList,
+                      const GVSummaryMapTy &DefinedGlobals) {
   auto Loader = [&](StringRef Identifier) {
----------------
I don't see this new parameter getting used anywhere. I think it might be leftover from your earlier approach (along with the other changes to this file)?


================
Comment at: lib/LTO/ThinLTOCodeGenerator.cpp:941
   StringMap<FunctionImporter::ExportSetTy> ExportLists(ModuleCount);
   ComputeCrossModuleImport(*Index, ModuleToDefinedGVSummaries, ImportLists,
                            ExportLists);
----------------
Add a call to your new analysis above here so it is supported with the old LTO API as well. Then you can augment your new test with an llvm-lto invocation as well.


================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:847
+
+void llvm::computeSyntheticCounts(ModuleSummaryIndex &Index) {
+  using Scaled64 = ScaledNumber<uint64_t>;
----------------
I assume that eventually we would want to gate this based on whether we have FDO or not - how will that work?


================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:851
+  auto GetCallSiteRelFreq = [](FunctionSummary::EdgeTy &Edge) {
+    // FIXME: Handle fake/invalid edges?
+    return Scaled64(Edge.second.RelBlockFreq, -CalleeInfo::ScaleShift);
----------------
What is a fake/invalid edge in the index?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D43521