Page MenuHomePhabricator

[llvm-profgen] Strip context to support non-CS profile generation for hybrid sample
ClosedPublic

Authored by wlei on Sep 14 2021, 9:29 AM.

Details

Summary

The patch introduces strip-context mode for generating non-CS profiles for hybrid samples. After perf reader of hybrid sample, it strips the call stack(context) sample. With this, we can generate both CS and non-CS profiles using the same perf script input.

Diff Detail

Event Timeline

wlei created this revision.Sep 14 2021, 9:29 AM
wlei requested review of this revision.Sep 14 2021, 9:29 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 14 2021, 9:29 AM
wlei edited the summary of this revision. (Show Details)Sep 14 2021, 9:36 AM
wlei added reviewers: hoy, wenlei, wmi.
wenlei added inline comments.Sep 21 2021, 11:50 PM
llvm/tools/llvm-profgen/PerfReader.cpp
25

Use ignore-stack-samples?

680–687

assert SampleCounters.size is zero then?

785

Where do we check PerfType later? If PerfType is meant to be input type, can we not set it to PERF_LBR since input type which is indeed hybrid?

If we meant to use PerfType to indicate "processing mode", perhaps we should then set it to PERF_LBR earlier in extractPerfType, then create LBRPerfReader and teach it to ignore stack samples. That also avoids changing to have HybridPerfReader derive from LBRPerfReader..

Change the type half way seems inconsistent.

wlei updated this revision to Diff 375345.Sep 27 2021, 11:07 AM

addressing feedbacks

refactor to move the generateRawProfile code out of parsePerfTraces and introduce a new flag(ProfileIsCS) for the profile generation creator

llvm/tools/llvm-profgen/PerfReader.cpp
25

Sounds good!

680–687

added!

785

Agree with you except I still prefer LBRPerfReader only responsible for PERF_LBR and HybridPerfReader only for PERF_LBR_STACK otherwise we need to maintain two part of code for the hybrid sample parsing.

On second thought, I think we already know whether it's a CS/Non-CS [raw] profile in PerfReader, so we can move the genRawProfiles logic out of parsePerfTraces. In parsePerfTraces , we only do populate perfSample. Then for genRawProfiles based on the perf type or ignore-stack-samples , it can decide to generate a CS raw profile or a non-CS raw profile. With this, we now pass a new flag ProfileIsCS to the generator instead of the perf_type.

Updated the code here, see if this looks good to you?

wenlei added inline comments.Sep 27 2021, 1:38 PM
llvm/tools/llvm-profgen/PerfReader.cpp
785

I still prefer LBRPerfReader only responsible for PERF_LBR and HybridPerfReader only for PERF_LBR_STACK otherwise we need to maintain two part of code for the hybrid sample parsing.

Yeah, we don't want to duplicate the code for sure.

I'm not sure about the refactoring to move unwind etc all into the base class though. I feel the unwind and hybrid sample parsing belong to HybridPerfReader, otherwise why do we need a HybridPerfReader..

Can we call LBRPerfReader::generateRawProfile() here in HybridPerfReader::generateRawProfile() just like what you had before, but without setting PerfType to PERF_LBR?

hoy added inline comments.Sep 27 2021, 1:51 PM
llvm/tools/llvm-profgen/PerfReader.cpp
26

nit: sample -> samples

785

Can we call LBRPerfReader::generateRawProfile() here in HybridPerfReader::generateRawProfile() just like what you had before, but without setting PerfType to PERF_LBR?

This sounds good to me. We still need to parse stack samples even with IgnoreStackSamples , but we don't need to unwind them.

wlei updated this revision to Diff 375438.Sep 27 2021, 5:01 PM

Updating D109769: [llvm-profgen] Strip context to support non-CS profile generation for hybrid sample

llvm/tools/llvm-profgen/PerfReader.cpp
26

fixed!

785

Sounds good to make unwinder into HybridPerfReader .

wenlei added inline comments.Mon, Sep 27, 5:35 PM
llvm/tools/llvm-profgen/PerfReader.cpp
784

nit: make sure we update ProfileIsCS properly for both cases and not relying on current value.

ProfileIsCS = !IgnoreStackSamples;
if (ProfileIsCS)
  unwindSamples();
else
  LBRPerfReader::generateRawProfile();
llvm/tools/llvm-profgen/PerfReader.h
614

This is now defined both in LBRPerfReader and PerfReaderBase? Is that intended? And it's not virtual, and yet declared as protected..

llvm/tools/llvm-profgen/llvm-profgen.cpp
92

Can we revert this change too and move generateRawProfile back into parsePerfTraces?

wlei added inline comments.Mon, Sep 27, 5:48 PM
llvm/tools/llvm-profgen/PerfReader.cpp
784

Good idea!

llvm/tools/llvm-profgen/PerfReader.h
614

removed

llvm/tools/llvm-profgen/llvm-profgen.cpp
92

fixed!

wlei updated this revision to Diff 375442.Mon, Sep 27, 5:48 PM

Updating D109769: [llvm-profgen] Strip context to support non-CS profile generation for hybrid sample

wenlei added inline comments.Mon, Sep 27, 5:52 PM
llvm/tools/llvm-profgen/PerfReader.h
646

Can this be private just like before?

wlei updated this revision to Diff 375447.Mon, Sep 27, 5:55 PM

protected --> private

wenlei accepted this revision.Mon, Sep 27, 5:56 PM

lgtm, thanks!

This revision is now accepted and ready to land.Mon, Sep 27, 5:56 PM
hoy accepted this revision.Tue, Sep 28, 12:15 PM

lgtm.

This revision was landed with ongoing or failed builds.Tue, Sep 28, 12:21 PM
This revision was automatically updated to reflect the committed changes.