[PATCH] D127026: [CSSPGO][llvm-profgen] Reimplement computeSummaryAndThreshold using context trie

Wed Jun 8 16:06:10 PDT 2022

hoy added inline comments.

================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.cpp:1010
+  ContextTracker.createContextLessProfileMap(ContextLessProfiles);
+  ProfileGeneratorBase::computeSummaryAndThreshold(ContextLessProfiles, false);
+}
----------------
wlei wrote:
> wenlei wrote:
> > hoy wrote:
> > > hoy wrote:
> > > > wlei wrote:
> > > > > hoy wrote:
> > > > > > Is `false` needed here? We are passing in contextless profiles so merging them again inside `computeSummaryAndThreshold` should be fine?
> > > > > Yeah, we can merge the contextless profiles twice, but is there any concern to save one?
> > > > I guess my questions should be why this the extra parameter needed. Is the original implementation that checks `FunctionSamples::ProfileIsCS` inside `SampleProfileSummaryBuilder::computeSummaryForProfiles` not working?
> > > OK, I see your point, to save the unnecessary merge. I'd say if this is not at a visible cost, let's avoid changing the interface. Otherwise maybe we can reset `UseContextLessSummary` here? A comment would be helpful.
> > save/restore UseContextLessSummary sounds like a good idea. 
> It seems we can not copy the cl::opt<bool> class, I just set UseContextLessSummary=true here, not sure if we need to recover the UseContextLessSummary value.
Would be better to recover the value since that'll be used by the extbinary writer to compute and flush out the profile summary on the final ProfileMap.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127026