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

Wenlei He via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Oct 27 16:03:11 PDT 2021


wenlei added inline comments.


================
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,
----------------
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. 



================
Comment at: llvm/tools/llvm-profgen/CSPreInliner.cpp:273
+  // Remove cold profiles from ProfileMap.
+  if (CSProfTrimColdContext) {
+    LLVM_DEBUG(dbgs() << "Trimming cold base profiles with HotCountThreshold="
----------------
hoy wrote:
> wenlei wrote:
> > hoy wrote:
> > > wenlei wrote:
> > > > I think it's cleaner if we defer all trimming to SampleContextTrimmer. We can add TrimBaseProfileOnly flag to trimAndMergeColdContextProfiles interface, or have a separate interface (will still need to make sure we reuse code). We create SampleContextTrimmer right above anyways. 
> > > > 
> > > > With this change, CSProfMergeColdContext will always be ignored when preinliner is on, but that's not the case before. 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.
> > > Sounds good. I'll defer SampleContextTrimmer to do the trimming.
> > > 
> > > There is a gray area with previous implementation when the preinliner and the trim are both on. In that case the previous check below will accidentally cause the profile merger to run. The profile merger defaulted to on but did not actually run when preilniner was on and neither merger/trimmer switch was given. Turning on the trimmer via the switch caused the merger to run which otherwise would not run. 
> > > 
> > > ```
> > > if (!EnableCSPreInliner || CSProfTrimColdContext.getNumOccurrences() ||
> > >       CSProfMergeColdContext.getNumOccurrences())
> > > ```
> > > 
> > > I think we want to run the trimmer based on the command line switch, regardless of whether preilniner is on. How about the merger? 
> > Maybe we should do this below. That makes sure we don't accidentally run the merging if only trimming is asked for. That is - make preinliner and merging either-or unless explicitly specified, but leave trimming orthogonal. 
> > 
> > ```
> > if (EnableCSPreInliner && !CSProfMergeColdContext.getNumOccurrences())
> >    CSProfMergeColdContext = false;
> > ```
> That should work. I was thinking if we should completely disable separate merging for preinliner, since we can tune the hotness cutoff to have preinliner do more merge.
Yeah, that would be the default behavior. But if user asks for merging on top of preinliner, it rarely makes sense but I assume they know what they're doing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112489



More information about the llvm-commits mailing list