This is an archive of the discontinued LLVM Phabricator instance.

[llvm-profdata] Improve profile supplementation
ClosedPublic

Authored by xur on Aug 24 2022, 1:36 PM.

Details

Summary

Current implementation promotes a non-cold function in the SampleFDO profile
into a hot function in the FDO profile. This is too aggressive. This patch
promotes a hot functions in the SampleFDO profile into a hot function, and a
warm function in SampleFDO into a warm function in FDO.

Diff Detail

Event Timeline

xur created this revision.Aug 24 2022, 1:36 PM
xur requested review of this revision.Aug 24 2022, 1:36 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 24 2022, 1:36 PM
davidxl added inline comments.Aug 24 2022, 2:13 PM
llvm/lib/ProfileData/ProfileSummaryBuilder.cpp
229–230

probably worth defining symbolic constants for these values.

Also how can we prevent these values are accidentally used somewhere else?

llvm/tools/llvm-profdata/llvm-profdata.cpp
515

comment mismatches.

517–524

why using coldthreshold?

xur added inline comments.Aug 24 2022, 2:46 PM
llvm/lib/ProfileData/ProfileSummaryBuilder.cpp
229–230

Symbolic constants seem reasonable.

These values should not be encountered in real profiles. 2^64 is too large for any counter values to come close.
But we do need to special handling of these values.
It's possible to misuse -- I think overlap function does not handle this well.
There are not too many places to use the counters directly. We just need to check all of them.

There might be other ways to implement -- like extending the profile format. But this is the simplest way.

llvm/tools/llvm-profdata/llvm-profdata.cpp
517–524

We want to set the entry count so that the function will be warm

The entry count needs to be greater than ColdThreshold but less than HotThreshold.

I think I need to add code to make sure it's less than HotThreshold.

davidxl added inline comments.Aug 24 2022, 3:02 PM
llvm/lib/ProfileData/ProfileSummaryBuilder.cpp
229–230

profile counter reader should do something when reading those values into instrProfRecord so that the special values are not leaked.

llvm/tools/llvm-profdata/llvm-profdata.cpp
528

are the counts all -1 or -2 in this case? what is the point of scaling?

xur added inline comments.Aug 25 2022, 10:02 AM
llvm/lib/ProfileData/ProfileSummaryBuilder.cpp
229–230

Agreed. We can have some APIs to InstrProfRecord to encapsulate the values.

The other alternative is to extend the profile format to have a information section for each function. Right now we are overload some information in the function hash function. I think have an optional information section for each function would be useful.

llvm/tools/llvm-profdata/llvm-profdata.cpp
515

will fix

528

If we assign -1 or -2, we will not do scaling, it will early return on line 504.
(that happens all zero counters or zero counters percentage exceed the threshold.)

This part is doing the scaling on the original counters.

xur added a comment.Aug 25 2022, 11:19 AM

This patches adds the support for mapping to warm function, just following the existing practice (where it maps to hot functions).
The constant of -1 is already in llvm.

Since we are talking about hiding these values within InstrProfRecord, I would also change the way how the information pass to to down streams: instead of using entry counts,
For a function with InstrProfRecord value of -1, we would set the function attribute hot,
For a function with InstrProfRecord value of -2, we would remove function attribute cold if it has, otherwise do nothing.

For warm functions, we could also delete the function profile in the supplementation step. But it's less optimal:
(1) we will have a no-profile-found warning (it is not a big problem, as the default is off)
(2) we will not be able to fix the incorrect user annotation (where the user sets the function to cold).

xur updated this revision to Diff 455811.Aug 26 2022, 12:11 AM

This is more complex change that hide the pseudo count values in InstroProfRecord.
Needs to touch more files. But I think this improves the original patch.

davidxl added inline comments.Aug 26 2022, 8:42 AM
llvm/include/llvm/ProfileData/InstrProf.h
844

pass CountPseudoKind

llvm/lib/ProfileData/InstrProf.cpp
739

typo: proifiles

740

typo peseudo

743

We should probably assume/require both profiles are (or not) supplemented, and always take the non pseudo one.

Or do not support merge with supplemented profile (i.e. only supplement after merging).

xur added inline comments.Aug 26 2022, 10:31 AM
llvm/include/llvm/ProfileData/InstrProf.h
844

Ack.

llvm/lib/ProfileData/InstrProf.cpp
739

Ack.

740

Ack.

743

Supplementing can produce a normal profile (only scaling), or a profile with pseudo counts.
So it's hard to force not merging supplemented profiles.

Probably the fist suggestion is the way to go: give a hard error when merging profile with pseudo counts and without pseudo counts.

emit an error for merging profiles of different kind is ok.

xur updated this revision to Diff 456082.Aug 26 2022, 10:44 PM

Integrated David's most recent review comments.

davidxl added inline comments.Aug 26 2022, 10:58 PM
llvm/lib/ProfileData/InstrProf.cpp
754

should this loop be skipped for pseudo kind?

llvm/lib/ProfileData/ProfileSummaryBuilder.cpp
238

add assert on count value (not pseudo values).

xur added inline comments.Aug 29 2022, 9:32 AM
llvm/lib/ProfileData/InstrProf.cpp
754

Yes. We can skip this loop for pesudo kind as the value will not be used.

llvm/lib/ProfileData/ProfileSummaryBuilder.cpp
238

Ack.

xur updated this revision to Diff 456423.Aug 29 2022, 11:23 AM

Integrated David's review comment.
For the merge profile merge, also skip the value profile merge if the count is pseudo kind.

Also added some tests to test pseudo profile merge.

This revision is now accepted and ready to land.Aug 29 2022, 11:44 AM
This revision was landed with ongoing or failed builds.Aug 29 2022, 4:58 PM
This revision was automatically updated to reflect the committed changes.