[PATCH] D67377: [PGO][PGSO] ProfileSummary changes.

[Hiroshi Yamauchi via Phabricator via llvm-commits]

yamauchi marked an inline comment as done.
yamauchi added a comment.

In D67377#1664995 <https://reviews.llvm.org/D67377#1664995>, @davidxl wrote:

> I don't think we should use 'PGSO' in the profile summary class. Instead, we should differentiate it from 'hot'   with other categories like VeryHot,  LukeWarm etc ...


I used to do similar to that, but I got to the current form where the notion of "the PGSO cutoff" is built into PSI and that cutoff is specific to PGSO because it's simple (the code to handle this profile percentile is all in PSI, etc.).

We could move it outside PSI and add the notion of VeryHot in PSI and map the PGSO cutoff choice that we want to those Cold/Hot/VeryHot and adjust the right flag, but I thought that that seemed complicated (if we want the 25 percentile as the cutoff, know that it's above Hot and set the PGSO cutoff to VeryHot and adjust ProfileSummaryCutoffVeryHot=25, etc. The thinking was why not just say "the PGSO cutoff = 25 (or 99.98 or 0)" regardless of the values of Cold/Hot/..?

In addition, we could try to achieve the same thing instead by set the PGSO cutoff to Hot and adjust ProfileSummaryCutoffHot=25 (and adjust ProfileSummaryCutoffVeryHot < 25 so that to maintain the invariant ProfileSummaryCutoffHot > ProfileSummaryCutoffVeryHot) but that would be odd (why allow this odd flexibility?) and would affect a lot of other optimizations. As "VeryHot" would be more generic than 'PGSO' is implied to be used by other optimizations, it'd affect them even when we want to tune PGSO specifically.

An alternative that occurs to me would be to avoid using the 'PGSO' name in PSI by adding a more generic interface to PSI like "isNPercentileCount(25)" and "isNPercentileBlock(50)" and use it from PGSO.

Thoughts?



================
Comment at: llvm/include/llvm/Analysis/ProfileSummaryInfo.h:103
+  /// PGSO.
+  bool pgsoHasHugeWorkingSetSize();
   /// Returns true if \p F has hot function entry.
----------------
davidxl wrote:
> Why can't PGSO huge working set size be unified with the  regular one?
So it could be tuned separately from the regular one. One of the internal apps had a working set size lower than the regular threshold but still benefited more from PGSO under the huge working set size condition. It'd be simpler to have one for sure. If it's unified, the app still benefits even without the huge working set size condition. It could be unified.

Or we could add a generic PSI::getWorkingSetSize() and use it from PGSO.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67377