[PATCH] D127026: [CSSPGO][llvm-profgen] Reimplement computeSummaryAndThreshold using context trie
Hongtao Yu via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Jun 6 11:47:15 PDT 2022
hoy added inline comments.
================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.cpp:435
- } else {
- // This is for the case the input is a llvm sample profile.
- for (const auto &FS : ProfileMap) {
----------------
wlei wrote:
> hoy wrote:
> > why delete this?
> It's not completely deleted, it's moved to the ctor(see: ProfileGenerator.h: 180), this is because we don't ever save the llvm-sample-profile into ProfileMap, I made the llvm-sample-profile a temporary variable, it's converted to trie right in the ctor and dropped. So I also moved this part of code there.
I see. Profiled functions are also used to on-demand decode pseudo probes and it's needed for non-CS profile generator too.
================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.cpp:1010
+ ContextTracker.createContextLessProfileMap(ContextLessProfiles);
+ ProfileGeneratorBase::computeSummaryAndThreshold(ContextLessProfiles, false);
+}
----------------
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?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D127026/new/
https://reviews.llvm.org/D127026
More information about the llvm-commits
mailing list