Page MenuHomePhabricator

[PGO][PGSO] ProfileSummary changes.
ClosedPublic

Authored by yamauchi on Sep 9 2019, 3:36 PM.

Details

Summary

(Split of off D67120)

ProfileSummary changes for profile guided size optimization.

Diff Detail

Repository
rL LLVM

Event Timeline

yamauchi created this revision.Sep 9 2019, 3:36 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 9 2019, 3:36 PM

The abbreviation PGSO is used throughout the code but never defined. Suggest defining it in a few places (e.g. on the new data members and function declarations).

llvm/include/llvm/Analysis/ProfileSummaryInfo.h
55 ↗(On Diff #219443)

Please add comments for these 2 new data members

llvm/lib/Analysis/ProfileSummaryInfo.cpp
76 ↗(On Diff #219443)

Should this instead be one of the -pgso-cutoff-* options?

yamauchi updated this revision to Diff 219569.Sep 10 2019, 10:48 AM
yamauchi marked 3 inline comments as done.

Update.

llvm/lib/Analysis/ProfileSummaryInfo.cpp
76 ↗(On Diff #219443)

No, this is meant to be like ProfileSummaryHugeWorkingSetSizeThreshold but separately tuned for PGSO to decide whether to have PGSO applied, based on the same profile-summary-cutoff-hot, rather than a different definition of huge working set size that changes based on pgso-cutoff-*. Turning up or down pgso-cutoff-* and choosing whether to having PGSO applied are meant to be independent. Added a comment.

The abbreviation PGSO is used throughout the code but never defined. Suggest defining it in a few places (e.g. on the new data members and function declarations).

Done.

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

llvm/include/llvm/Analysis/ProfileSummaryInfo.h
103 ↗(On Diff #219443)

Why can't PGSO huge working set size be unified with the regular one?

llvm/lib/Analysis/ProfileSummaryInfo.cpp
60 ↗(On Diff #219443)

Call it ProfileSummaryCutoffInstVeryHot

66 ↗(On Diff #219443)

ProfileSummaryCutoffSampleVeryHot

yamauchi marked an inline comment as done.Sep 11 2019, 10:34 AM

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?

llvm/include/llvm/Analysis/ProfileSummaryInfo.h
103 ↗(On Diff #219443)

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.

It is useful to have generic interface like isHotCountNthPercentile(..) to check if a count is hot relative to a given percentile cutoff. In theory, PGSO does not have a fixed hotness threshold as it depends on workingset size. If we partition working set size into ranges like small, medium, large, huge, then PGSO can use different hotness thresholds based on that.

yamauchi updated this revision to Diff 220739.Sep 18 2019, 1:26 PM

Moved 'PGSO' out of ProfileSummary.

davidxl added inline comments.Sep 19 2019, 2:23 PM
llvm/lib/Analysis/ProfileSummaryInfo.cpp
274 ↗(On Diff #220739)

Why Optional?

275 ↗(On Diff #220739)

since illegal PercentilCutoff will result in assertion, there seems no need to cap the cache.

277 ↗(On Diff #220739)

Since you are not checking insert status, it is cleaner to use

ThresholdCache[PercentileCutoff] = CountThreshold;

yamauchi updated this revision to Diff 221055.Sep 20 2019, 10:07 AM
yamauchi marked 4 inline comments as done.

Address comments.

Some unit tests can be added to llvm/unittests/ProfileData/InstrProfTest.cpp -- there are existing profile summary tests that can be extended.

yamauchi updated this revision to Diff 221366.Sep 23 2019, 10:58 AM

Added unit tests.

davidxl accepted this revision.Sep 24 2019, 9:53 AM

lgtm

This revision is now accepted and ready to land.Sep 24 2019, 9:53 AM
This revision was automatically updated to reflect the committed changes.