[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;
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127026