[PATCH] D41604: Add a pass to generate synthetic function entry counts.

[David Li via Phabricator via llvm-commits]

davidxl added inline comments.


================
Comment at: include/llvm/IR/Function.h:256
+  /// will execute.
+  Optional<uint64_t> getSyntheticEntryCount() const;
+
----------------
eraman wrote:
> davidxl wrote:
> > Is it better to let getEntryCount to take an optional flag to indicate 'is_synthetic'? The implementation of these two are mostly the same.
> I actually intended to remove this but this slipped in.  I have a follow up patch that makes EntryCount a class and make the  getEntryCount return it.  Using a boolean for synthetic feels brittle.  Shall I remove this get method?
The new interface that returns EntryCount seems a better approach. Yes, if you can unify the interfaces, this one should be removed. You can do it in two steps -- one takes the boolean flag, and then improved with a follow up patch.


================
Comment at: lib/Analysis/SyntheticCountsUtils.cpp:50
+
+  // Partition callsites so that those in the same SCC come first.
+  auto Mid = partition(CallSites, [&](CallSite &CS) {
----------------
Nit: callsites that call into the SCC come first.


================
Comment at: lib/Analysis/SyntheticCountsUtils.cpp:64
+  // traversal of functions within the SCC doesn't affect the computation.
+  DenseMap<Function *, uint64_t> PropagatedCounts;
+  for (auto It = CallSites.begin(); It != Mid; It++) {
----------------
PropagatedCounts --> ExtraSCCEntryCountFromCycles ? or just ExtraSCCEntryCounts?

Also modify the comments to say:
1) first update the global count of nodes with SCC using cyclic edges first.
2) the update is done in two steps ..


================
Comment at: lib/Analysis/SyntheticCountsUtils.cpp:67
+    auto &CS = *It;
+    auto Weight = GetCallSiteWeight(CS);
+    Function *Callee = CS.getCalledFunction();
----------------
Weight --> RelativeFreq

It is confusing to see Weight multiplied by Count


================
Comment at: lib/Transforms/IPO/SyntheticCountsPropagation.cpp:106
+  // and not integers since the relative block frequency could be less than 1.
+  auto GetCallSiteWeight = [&](CallSite CS) {
+    Function *Caller = CS.getCaller();
----------------
Call it CallSiteRelFreq may make the code more readable.


https://reviews.llvm.org/D41604