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

[Lei Wang via Phabricator via llvm-commits]

wlei added inline comments.


================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.cpp:442
 
+void ProfileGenerator::collectPFForLLVMSampleProfile(
+    std::unordered_set<const BinaryFunction *> &ProfiledFunctions) {
----------------
hoy wrote:
> wlei wrote:
> > hoy wrote:
> > > Sorry for not making it clear. Can we do something like below? Basically keep the original `collectProfiledFunctions` as is and add a new override for CSProfileGenerator.
> > > 
> > > 
> > > ```
> > > void ProfileGeneratorBase::collectProfiledFunctions() {
> > >   std::unordered_set<const BinaryFunction *> ProfiledFunctions;
> > >   if (SampleCounters) {
> > >      ....
> > >   } else if (!ProfileMap.empty())
> > >      ....
> > >   }
> > >   Binary->setProfiledFunctions(ProfiledFunctions);
> > > }
> > > 
> > > void CSProfileGenerator::collectProfiledFunctions() {
> > >   deal with ContextTrieNode;
> > >   Binary->setProfiledFunctions(ProfiledFunctions);
> > > }
> > > ```
> > > 
> > > 
> > ```
> >   if (SampleCounters) {
> >      ....
> >   }
> > ```
> > This part of code is also needed for `CSProfileGenerator` because `collectProfiledFunctions ` is called before the `generateProbeBasedProfile`, at that time none of `ContextTrieNode` is created.
> I see, would this work?
> 
> ```
> 
> void CSProfileGenerator::collectProfiledFunctions() {
>   if (getRootContext) {
>     ... 
>     Binary->setProfiledFunctions(ProfiledFunctions);
>    }
>    else {
>      ProfileGeneratorBase::collectProfiledFunctions();
>    }
> }
> ```
> 
This one should work, but it might introduce more complexity.  We only have two modes 1) from Sample counter 2) from llvm-sample-profile.  Before we use one condition(`SampleCounters == null`) to differentiate  this two mode. But this one adds another condition(getRootContext()) which intend to do the same thing.

Is there any concern for the current version? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127026