(Split of off D67120)
ProfileSummary changes for profile guided size optimization.
Paths
| Differential D67377
[PGO][PGSO] ProfileSummary changes. ClosedPublic Authored by hjyamauchi on Sep 9 2019, 3:36 PM.
Details
Diff Detail
Event TimelineComment Actions 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).
hjyamauchi marked 3 inline comments as done. Comment ActionsUpdate.
Comment Actions
Done. Comment Actions 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 ...
Comment Actions
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 Actions 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. Comment Actions Some unit tests can be added to llvm/unittests/ProfileData/InstrProfTest.cpp -- there are existing profile summary tests that can be extended. This revision is now accepted and ready to land.Sep 24 2019, 9:53 AM Closed by commit rL372783: [PGO][PGSO] ProfileSummary changes. (authored by hjyamauchi). · Explain WhySep 24 2019, 3:17 PM This revision was automatically updated to reflect the committed changes. hjyamauchi added a child revision: D67120: [PGO] Profile guided code size optimization (continued)..Oct 10 2019, 3:42 PM
Revision Contents
Diff 221366 llvm/include/llvm/Analysis/ProfileSummaryInfo.h
llvm/lib/Analysis/ProfileSummaryInfo.cpp
llvm/unittests/Analysis/ProfileSummaryInfoTest.cpp
|
Please add comments for these 2 new data members