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

[Hongtao Yu via Phabricator via llvm-commits]

hoy added inline comments.


================
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="
----------------
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? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112489