[PATCH] D112489: [CSSPGO] Trim cold profiles with the CS preinliner.

[Wenlei He via Phabricator via llvm-commits]

wenlei accepted this revision.
wenlei added a comment.
This revision is now accepted and ready to land.

lgtm, thanks.



================
Comment at: llvm/lib/ProfileData/SampleProf.cpp:335
+    uint32_t ColdContextFrameLength, bool TrimBaseProfileOnly) {
   if (!TrimColdContext && !MergeColdContext)
     return;
----------------
hoy wrote:
> wlei wrote:
> > what if `TrimBaseProfileOnly` is true but those two are false, shall we skip the trim?
> > 
> > maybe change to `if (!TrimColdContext && !MergeColdContext && !TrimBaseProfileOnly)`
> `TrimBaseProfileOnly` should only be effective when `TrimColdContext` is true. Trimming means to trim cold profile. On top of that we can choose to trim all cold profiles or only base cold profiles. Does that make sense? I can add a comment for that.
Yeah, sounds good to me. A comment would be helpful.  


================
Comment at: llvm/tools/llvm-profgen/CSPreInliner.cpp:275-283
+  if (!CSProfMergeColdContext.getNumOccurrences())
+    CSProfMergeColdContext = false;
+
+  // Trim or merge cold profiles from ProfileMap.
+  if (CSProfTrimColdContext || CSProfMergeColdContext) {
+    Trimmer.trimAndMergeColdContextProfiles(
+        HotCountThreshold, CSProfTrimColdContext, CSProfMergeColdContext,
----------------
hoy wrote:
> hoy wrote:
> > wenlei wrote:
> > > Sorry if I wasn't clear.
> > > 
> > > > So alternatively, if TrimBaseProfileOnly is added to the interface, we can call the trimmer outside of preinliner like before, but set TrimBaseProfileOnly to true when preinliner is on.
> > > 
> > > What I meant by the above to do the trimming outside of preinliner. So preinliner does strictly preinlining, and trimming is done at the original place, with TrimBaseProfileOnly set to EnableCSPreInliner. This way we don't have trimming functionality scattered.
> > > 
> > > The setting of CSProfMergeColdContext also better be done outside of preinliner. 
> > > 
> > I found that separating the logic for the preinlining and non-preinlining path is easier to read and less error prone. Don't have a strong preference. Can do what you suggested.
> Actually the logic looks better after moving this out.
Yes, I think separating them out is cleaner. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112489