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

[Wenlei He via Phabricator via llvm-commits]

wenlei 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:
> > > 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.
> 
Consider this to give it more structure and help readability? 

```
ProfileGeneratorBase::collectProfiledFunctions() {
    if (collectFunctionsFromRawProfile(ProfiledFunctions)) 
        Binary->setProfiledFunctions(ProfiledFunctions);
    else if (collectFunctionsFromLLVMProfile(ProfiledFunctions)) 
        Binary->setProfiledFunctions(ProfiledFunctions);
    else 
        llvm_unreachable(...);
}

bool ProfileGeneratorBase::collectFunctionsFromRawProfile(...) {
    if (!SampleCounters)
       return false;
    // read raw profile sample counters
    return true;
}

bool ProfileGenerator::collectFunctionsFromLLVMProfile(..) {
    // read ProfileMap
    return true;
}

bool CSProfileGenerator::collectFunctionsFromLLVMProfile(..) {
    // read trie nodes
    return true;
}
```


================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.cpp:1009
+  SampleProfileMap ContextLessProfiles;
+  ContextTracker.createContextLessProfileMap(ContextLessProfiles);
+  ProfileGeneratorBase::computeSummaryAndThreshold(ContextLessProfiles, false);
----------------
wlei wrote:
> wenlei wrote:
> > Not critical, but in the past we have the ability to use context profile as well when `profile-summary-contextless=0` is used. It seems like we now lost that ability? 
> I see, I wasn't aware that we can use `profile-summary-contextless=0`. Yes, now we don't have the ability. I guess we don't need this before pre-inliner?
> 
> To support this, we can create a full CS map here like we call `buildProfileMap`, but we need to make a copy instead of move in `buildProfileMap`, we don't have that for now. it seems it will increase the complexity of the code. or we can call to compute the summary again after pre-inliner.
I think it's ok to not support `profile-summary-contextless=0` given we haven't really use it much. Maybe add an assertion for `UseContextLessSummary` to be true?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127026