[PATCH] D147013: [CSSPGO][Preinliner] Trim cold call edges of the profiled call graph for a more stable profile generation.

[Wenlei He via Phabricator via llvm-commits]

wenlei added inline comments.


================
Comment at: llvm/tools/llvm-profgen/CSPreInliner.cpp:84
+      (Summary->getDetailedSummary()));
+  ProfiledCallGraph ProfiledCG(ContextTracker, ColdCountThreshold + 1);
 
----------------
hoy wrote:
> wenlei wrote:
> > hoy wrote:
> > > wenlei wrote:
> > > > why is +1 needed? if cold threshold is 0, nothing is cold, and we filter out nothing -- it just works? 
> > > There are still zero-weight edges coming from callstack unwinding and they do not show up in the profile. I think. we want to trim them.
> > what I meant is if cold threshold is already zero anyways, that would mean nothing is cold. so nothing should be trimmed. Usually cold threshold is never zero, in which case all those zero-weight edges will be trimmed without +1.
> The cold threshold is mainly computed from LBR samples and it shouldn't be zero. But we are trimming edges with weight below the threshold (line 199 in ProfiledCallGraph.h), so the +1 here is to make sure all cold edges go away. 
> 
> The +1 is not needed here if we use a less-equal check on the profiled call graph check. But there isn't a way to not trim any edges unless we make the zero threshold special in the interface. (well, I have the check in line 192 in ProfiledCallGraph.h to make is special right now, but it's unnecessary as interface-wise zero is not considered special).
I understand what you said. But let me try one more time. :) So you want to differentiate 0 as special value to disable trimming vs 0 as a real threshold for trimming. What I meant we don't need to differentiate, because 0 as a real threshold also means no need for trimming at all. 

If we consider anything "< threshold" as cold, threshold equals 0 means nothing is cold, hence nothing needs to be trimmed. This is not a big deal anyways, but I just feel that things fits together nicely already without needing +1. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D147013/new/

https://reviews.llvm.org/D147013