[PATCH] D94111: [CSSPGO][llvm-profgen] Merge and trim profile for cold context to reduce profile size

Wed Jan 20 09:29:24 PST 2021

wenlei added inline comments.

================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.cpp:33
+    "csprof-cold-thres", cl::init(100), cl::ZeroOrMore,
+    cl::desc("Specified the total samples threshold for a context profile to "
+             "be considered cold, any cold profiles will be merged into "
----------------
hoy wrote:
> wlei wrote:
> > hoy wrote:
> > > Nit: Specify
> > > 
> > > I was wondering in the future if we'd like a consistent way of computing cold threshold with `SampleProfileSummaryBuilder`, i.e., counts below 99.9% (aka cut-off) are considered cold, so that the profile generator and the profile consumer will be on the same page.
> > Thanks for sharing this. I'm not sure how we get the 100 threshold, I guess this is the result of many iterations. Let me dig into the `SampleProfileSummaryBuilder` to see the chance.
> Yes, it is a number from experiments. The logic in the profile summary builder is a bit different in that it sorts all samples by weight and treats the samples below a given percentage (say 0.01%) as cold. In order words, if there are more than 99.99% samples that are hotter than the current one, then the current is treated as cold. That's the logic used in the compiler.
Yeah, +1 for what Hongtao said, I wanted to use %-based cutoff, but didn't have access to SampleProfileSummaryBuilder from early prototype. 

I think create_llvm_profile tool have both `sample_threshold_frac` and `sample_threshold_absolute`? Similarly both could be useful here. We could refine them as we broaden the adoption and continue tuning - I don't have an opinion whether we make all these improvements here or in another patch. 

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94111