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

Easwaran Raman via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Jan 4 16:58:55 PST 2018

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. 


More information about the llvm-commits mailing list