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

[Easwaran Raman via Phabricator via llvm-commits]

eraman marked 4 inline comments as done.
eraman added inline comments.


================
Comment at: include/llvm/IR/Function.h:256
+  /// will execute.
+  Optional<uint64_t> getSyntheticEntryCount() const;
+
----------------
davidxl wrote:
> 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.
This patch doesn't read the synthetic entry count and hence I am just removing the getSyntheticEntryCount from this.  I will leave the boolean on the setEntryCount and in the next patch add an EntryCount class. 


================
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++) {
----------------
davidxl wrote:
> 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 ..
I have modified the comments along the lines of what you say. I have renamed the variable to AdditionalCounts  - I prefer this to your other suggestions. 


https://reviews.llvm.org/D41604