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

[Easwaran Raman via Phabricator via llvm-commits]

eraman added inline comments.


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


================
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 "
----------------
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.


================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:672
+
+void llvm::computeSyntheticCounts(ModuleSummaryIndex &Index) {
+  using Scaled64 = ScaledNumber<uint64_t>;
----------------
tejohnson wrote:
> Some comments about what the algorithm does would be nice.
I haven't  explained the algorithm here because it is done in SyntheticCountUtils that works on any callgraph. I have tried to add some comments. Let me know if you want any specific details.


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

This should have been set to all copies. I have modified this as well as initialization counts. 


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


Repository:
  rL LLVM

https://reviews.llvm.org/D43521