[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:1010
+  ContextTracker.createContextLessProfileMap(ContextLessProfiles);
+  ProfileGeneratorBase::computeSummaryAndThreshold(ContextLessProfiles, false);
+}
----------------
hoy wrote:
> wlei wrote:
> > wenlei wrote:
> > > hoy wrote:
> > > > hoy wrote:
> > > > > wlei wrote:
> > > > > > hoy wrote:
> > > > > > > Is `false` needed here? We are passing in contextless profiles so merging them again inside `computeSummaryAndThreshold` should be fine?
> > > > > > Yeah, we can merge the contextless profiles twice, but is there any concern to save one?
> > > > > I guess my questions should be why this the extra parameter needed. Is the original implementation that checks `FunctionSamples::ProfileIsCS` inside `SampleProfileSummaryBuilder::computeSummaryForProfiles` not working?
> > > > OK, I see your point, to save the unnecessary merge. I'd say if this is not at a visible cost, let's avoid changing the interface. Otherwise maybe we can reset `UseContextLessSummary` here? A comment would be helpful.
> > > save/restore UseContextLessSummary sounds like a good idea. 
> > It seems we can not copy the cl::opt<bool> class, I just set UseContextLessSummary=true here, not sure if we need to recover the UseContextLessSummary value.
> Would be better to recover the value since that'll be used by the extbinary writer to compute and flush out the profile summary on the final ProfileMap.
I see, value recovered.


================
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:
> 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?
Sounds good to use name directly from trie. Just one thing is context tri is only in CSProfileGenerator, so I still need to use different function for this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127026