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

[Hongtao Yu via Phabricator via llvm-commits]

hoy added inline comments.


================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.cpp:442
 
+void ProfileGenerator::collectPFForLLVMSampleProfile(
+    std::unordered_set<const BinaryFunction *> &ProfiledFunctions) {
----------------
wlei wrote:
> 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? 
Not a really concern,  the current version looks good. I'm trying to see if the code can be simplified furthermore. Now that the context tri and ProfileMap are mutual-exclusive, perhaps using virtual functions to implement them is the best way. Adding an extra check is not worth.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127026