[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:131-138
-ProfileGeneratorBase::create(ProfiledBinary *Binary,
-                             const SampleProfileMap &&Profiles,
+ProfileGeneratorBase::create(ProfiledBinary *Binary, SampleProfileMap &Profiles,
                              bool ProfileIsCS) {
   std::unique_ptr<ProfileGeneratorBase> Generator;
   if (ProfileIsCS) {
     if (Binary->useFSDiscriminator())
       exitWithError("FS discriminator is not supported in CS profile.");
----------------
wenlei wrote:
> The move is for `llvm-sample-profile` path. Do we fall back from move to copy for that path now? 
There is no extra copy here, we don't update ProfileMap for CS profile.  For `llvm-sample-profile` path, the input is a reference, the map is converted to a trie in the ctor, so the lifetime of the reference end in the ctor. The ProfileMap is always empty for CS profile until we call `buildProfileMap`.


================
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) {
----------------
hoy wrote:
> 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.
I see, good catch! I added them back for non-CS path.


================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.cpp:1009
+  SampleProfileMap ContextLessProfiles;
+  ContextTracker.createContextLessProfileMap(ContextLessProfiles);
+  ProfileGeneratorBase::computeSummaryAndThreshold(ContextLessProfiles, false);
----------------
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.


================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.cpp:1010
+  ContextTracker.createContextLessProfileMap(ContextLessProfiles);
+  ProfileGeneratorBase::computeSummaryAndThreshold(ContextLessProfiles, false);
+}
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127026