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

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

Sorry for the lateness of the review! Comments/questions below.



================
Comment at: include/llvm/IR/ModuleSummaryIndex.h:457
+  /// The synthesized entry count of the function.
+  uint64_t EntryCount = 0;
+
----------------
Need to document that this is only populated by the Thin Link (and unused by per-module summaries).


================
Comment at: include/llvm/IR/ModuleSummaryIndex.h:1094
+    auto *S = N.getSummaryList().front().get()->getBaseObject();
+    FunctionSummary *F = cast<FunctionSummary>(S);
+    return F->CallGraphEdgeList.begin();
----------------
For consistency it would be nice to have the above two lines match the same code in child_begin (same with child*_end). I.e. 

    FunctionSummary *F =
        cast<FunctionSummary>(N.getSummaryList().front()->getBaseObject());


================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:5145
     return error("Invalid summary version " + Twine(Version) +
                  ", 1, 2, 3 or 4 expected");
   Record.clear();
----------------
Update message (maybe change to a range rather than a list, since we now have quite a few valid versions).


================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:5248
       auto FS = llvm::make_unique<FunctionSummary>(
-          Flags, InstCount, getDecodedFFlags(RawFunFlags), std::move(Refs),
+          Flags, InstCount, getDecodedFFlags(RawFunFlags), 0, std::move(Refs),
           std::move(Calls), std::move(PendingTypeTests),
----------------
document constant 0 param (here and in other constructor calls). I.e. "/*Entry Count=*/0"


================
Comment at: lib/Bitcode/Writer/BitcodeWriter.cpp:3747
+      CountIndex = 6;
+      NameVals.push_back(FS->entryCount());
+    }
----------------
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.


================
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 "
----------------
why off by default?


================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:672
+
+void llvm::computeSyntheticCounts(ModuleSummaryIndex &Index) {
+  using Scaled64 = ScaledNumber<uint64_t>;
----------------
Some comments about what the algorithm does would be nice.


================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:680
+  auto GetEntryCount = [](ValueInfo V) {
+    FunctionSummary *F = nullptr;
+    if (V.getSummaryList().size()) {
----------------
Can this be moved down into the if statement body? It isn't used outside of it.


================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:690
+  auto AddToEntryCount = [](ValueInfo V, uint64_t New) {
+    FunctionSummary *F = nullptr;
+    if (V.getSummaryList().size()) {
----------------
ditto.


================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:694
+      F = cast<FunctionSummary>(S);
+      F->setEntryCount(F->entryCount() + New);
+    }
----------------
can this overflow?
Also, why only set the entry count on the first copy? Shouldn't we propagate into all copies?


================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:901
+    FunctionSummary *FS =
+        dyn_cast_or_null<FunctionSummary>(Index.findSummaryInModule(
+            F.getGUID(), DestModule.getModuleIdentifier()));
----------------
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.


================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:994
+        /*
+        auto *Summary = Index.findSummaryInModule(GUID, Name);
+        FunctionSummary *FS =
----------------
Remove?


Repository:
  rL LLVM

https://reviews.llvm.org/D43521