[PATCH] D107800: [CSSPGO][llvm-profgen] Trim and merge context beforehand to reduce memory usage

Wed Aug 11 15:14:00 PDT 2021

spupyrev added inline comments.

================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.cpp:73

+int CSProfileGenerator::MaxContextDepth = -1;
+
----------------
hoy wrote:
> wlei wrote:
> > spupyrev wrote:
> > > The default value of "no trimming" sounds strange to me. Do we know if trimming may affect performance? If yes, we shouldn't trim. If no, we should have a reasonable bound here.
> > > Do we know if trimming may affect performance?
> > It depends on how compiler consumes the context, if the profile context is [a @ b @ c @ d] but compiler only inline d to c and stop inlining at c, then the [a @ b @] can be trimmed. Another things is the profile size tradeoff, before we did merge and trim cold profile after all samples is generated. This patch tried to move the merging ahead as we encountered the memory issues. it doesn't drop the sample.
> > 
> > >If no, we should have a reasonable bound here.
> > Good question, but I don't know what's the reasonable bound yet. As you see the above comments, SPEC benchmark's max inline depth can vary from 5 to 26, the clang-10 binary even have a 51 max depth. and we didn't get memory issue for SPEC, so SPEC can set to -1.
> > 
> > so it's the tradeoff between many things, what's in my mind is to default it `-1` and we can tune it on-demand. what do you think?
> > 
> > 
> > 
> Default -1 (means no trimming) sounds good to me.  An otherwise decent default value would need quite some experiments to justify.
I dislike tunable flags that may or may not affect the performance. I assume most (all?) of the end-users won't know about the flag and will utilize the default value. So whatever you set here will be used in the vast majority of cases.

If course, if you do plan to run some perf experiments in the (near) future and tune the default value, then the current value of "-1" is OK.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107800