[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.h:179
+      : ProfileGeneratorBase(Binary), ContextTracker(Profiles, nullptr) {
+    // This is for the case the input is a llvm sample profile.
+    std::unordered_set<const BinaryFunction *> ProfiledFunctions;
----------------
hoy wrote:
> wlei wrote:
> > hoy wrote:
> > > wlei wrote:
> > > > hoy wrote:
> > > > > This piece of code is needed because `Profiles` is not saved as a field of `ProfileGeneratorBase` anymore. Can we still do that to avoid duplicate this code here?
> > > > Before it copied to `ProfileMap` and now we need to make `ProfileMap` empty because the `buildProfileMap`  will convert the optimized trie into ProfileMap. Non-empty ProfileMap will mess up the results.
> > > > 
> > > > Maybe we can add a new field to `ProfileGeneratorBase ` like `SampleProfileMap & LLVMSampleProfiles;`?
> > > > 
> > > > now we need to make ProfileMap empty because the buildProfileMap will convert the optimized trie into ProfileMap. Non-empty ProfileMap will mess up the results.
> > > 
> > > Just clean it inside `buildProfileMap`?
> > If cleaning it before, all the FunctionSamples owned by the map are freed, the FunctionSamples pointer in the ContextTrieNode is dangling.
> I see. Maybe we should just compute a contextless profile from ContextTracker in `collectProfiledFunctions` ? This is less efficient, but should fit better in the pipeline for easier maintenance. The performance of the llvm profile path isn't critical at the end of the day.
On the second thought, we don't really need a contextless profile, just looping on the context tri and collecting function names should be sufficient?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127026