This is an archive of the discontinued LLVM Phabricator instance.

[llvm-profdata] Convert nested profile to CS flat profile.
Needs ReviewPublic

Authored by hoy on Mar 14 2022, 4:43 PM.

Details

Reviewers
wenlei
wlei
Summary

Support of convert a regular nested profile to CS flat profile. This is useful for comparing the calling contexts in two different types of profiles.

Diff Detail

Event Timeline

hoy created this revision.Mar 14 2022, 4:43 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 14 2022, 4:43 PM
hoy requested review of this revision.Mar 14 2022, 4:43 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 14 2022, 4:43 PM
hoy retitled this revision from [llvm-profdata] Convert nested propfile to CS flat profile to [llvm-profdata] Convert nested profile to CS flat profile..Mar 14 2022, 4:47 PM
hoy edited the summary of this revision. (Show Details)
hoy added reviewers: wenlei, wlei.

Some linter seems legit.

llvm/lib/ProfileData/SampleProf.cpp
501–502

I'm a bit confused by the name convertToCSFlatProfiles, looking at the implementation isn't this converting to nested profile?

553–617

Here the caller name is convertToCSNestedProfiles, but the callee on line 552 is convertToCSFlatProfiles.

llvm/test/tools/llvm-profdata/cs-sample-flat-profile.test
2

Add round trip test for conversion?

llvm/tools/llvm-profdata/llvm-profdata.cpp
750–751

Are the two flags (GenCSNestedProfile/GenCSFlatProfile and the corresponding switches) supposed to be mutually exclusive? If yes, perhaps we should make it explicit?

one way to make then mutual exclusive is to use a single switch: gen-cs-nested-profile==true means nested, false means flat. Or we can have -gen-cs-profile={nest|flat}.

hoy updated this revision to Diff 418905.Mar 29 2022, 9:32 AM

Updating D121651: [llvm-profdata] Convert nested profile to CS flat profile.

hoy added inline comments.Mar 29 2022, 9:33 AM
llvm/lib/ProfileData/SampleProf.cpp
553–617

Sorry, bad auto renaming.

llvm/test/tools/llvm-profdata/cs-sample-flat-profile.test
2

Done.

llvm/tools/llvm-profdata/llvm-profdata.cpp
750–751

Introduced a --sample-profile-layout=cs|nest switch.

hoy updated this revision to Diff 418906.Mar 29 2022, 9:41 AM

Updating D121651: [llvm-profdata] Convert nested profile to CS flat profile.

wenlei added inline comments.Mar 29 2022, 11:28 PM
llvm/include/llvm/ProfileData/SampleProf.h
101

I think flat vs nest is better here (than cs vs nest).

382–383

addCalledTarget does not add to NumSamples, but removeCalledTarget subtracts NumSamples. What's the reason for the inconsistency?

llvm/lib/ProfileData/SampleProf.cpp
515

nit: 0->ContextNone

527–528

It looks like the discrepancy comes from the fact that flat profile has target counts included in total samples, but nest profile doesn't? Is this something that should be unified? I don't remember why we choose to include target counts (rather than SampleRecord::NumSamples) in total counts for flat profile. Was it intentional?

hoy added inline comments.Mar 30 2022, 2:35 PM
llvm/include/llvm/ProfileData/SampleProf.h
101

Sounds good.

382–383

I think. It's due to the implementation of the profile generator. Callsite block samples are computed by analyzing LBR ranges, and NumSamples is updated there, while call target samples are computed based on branch samples. D122609 provides an option to make them consistent.

removeCalledTarget handles both in one place.

llvm/lib/ProfileData/SampleProf.cpp
515

Done.

527–528

The profile generator, for both flat and nest profiles, only includes callsite counts in the total count but not target counts. Ideally they would be the same. Here we are excluding target counts from callsite counts, which in turn excluded from total counts.

hoy updated this revision to Diff 419270.Mar 30 2022, 2:36 PM

Updating D121651: [llvm-profdata] Convert nested profile to CS flat profile.

hoy updated this revision to Diff 425390.Apr 26 2022, 6:46 PM

Rebasing

hoy updated this revision to Diff 429764.May 16 2022, 10:22 AM

Rebasing.

wenlei added inline comments.Jun 8 2022, 12:05 PM
llvm/tools/llvm-profdata/llvm-profdata.cpp
831

The boolean conversion seem to point to hasValue? I think the convention is mostly *ProfileIsCS.

constexpr explicit operator bool() const { return hasValue(); }

1027

nit: name this convert-sample-profile-layout. otherwise having SPL_None as default can be confusing as there's no None layout. But it makes sense for conversion to be None.

hoy updated this revision to Diff 435294.Jun 8 2022, 12:33 PM

Updating D121651: [llvm-profdata] Convert nested profile to CS flat profile.

llvm/tools/llvm-profdata/llvm-profdata.cpp
831

Here ProfileLayout is a function parameter of type SampleProfileLayout .

1027

Sounds good.

hoy updated this revision to Diff 435324.Jun 8 2022, 2:16 PM

Updating D121651: [llvm-profdata] Convert nested profile to CS flat profile.

wenlei added inline comments.Jun 10 2022, 3:29 PM
llvm/tools/llvm-profdata/llvm-profdata.cpp
831

I was talking about Optional<bool> ProfileIsCS.

hoy added inline comments.Jun 10 2022, 6:28 PM
llvm/tools/llvm-profdata/llvm-profdata.cpp
831

Oh I see. Yes, *ProfileIsCS should be used instead.

Also fixed another place like this.

hoy updated this revision to Diff 436089.Jun 10 2022, 6:28 PM

Updating D121651: [llvm-profdata] Convert nested profile to CS flat profile.