Page MenuHomePhabricator

[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

Unit TestsFailed

TimeTest
60,060 msx64 debian > MLIR.Examples/standalone::test.toy
Script: -- : 'RUN: at line 1'; /usr/bin/cmake /var/lib/buildkite-agent/builds/llvm-project/mlir/examples/standalone -G "Ninja" -DCMAKE_CXX_COMPILER=/usr/bin/clang++ -DCMAKE_C_COMPILER=/usr/bin/clang -DLLVM_ENABLE_LIBCXX=OFF -DMLIR_DIR=/var/lib/buildkite-agent/builds/llvm-project/build/lib/cmake/mlir ; /usr/bin/cmake --build . --target check-standalone | tee /var/lib/buildkite-agent/builds/llvm-project/build/tools/mlir/test/Examples/standalone/Output/test.toy.tmp | /var/lib/buildkite-agent/builds/llvm-project/build/bin/FileCheck /var/lib/buildkite-agent/builds/llvm-project/mlir/test/Examples/standalone/test.toy
60,050 msx64 debian > libFuzzer.libFuzzer::large.test
Script: -- : 'RUN: at line 3'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang --driver-mode=g++ -O2 -gline-tables-only -fsanitize=address,fuzzer -I/var/lib/buildkite-agent/builds/llvm-project/compiler-rt/lib/fuzzer -m64 /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/fuzzer/LargeTest.cpp -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/fuzzer/X86_64DefaultLinuxConfig/Output/large.test.tmp-LargeTest

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
503–504

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

558–621

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
736

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
558–621

Sorry, bad auto renaming.

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

Done.

llvm/tools/llvm-profdata/llvm-profdata.cpp
736

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
103

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

375–376

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

llvm/lib/ProfileData/SampleProf.cpp
517

nit: 0->ContextNone

529–530

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
103

Sounds good.

375–376

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
517

Done.

529–530

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
817

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

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

1013

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
817

Here ProfileLayout is a function parameter of type SampleProfileLayout .

1013

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
817

I was talking about Optional<bool> ProfileIsCS.

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

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.