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.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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. |
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? |
llvm/tools/llvm-profgen/PerfReader.cpp | ||
---|---|---|
785 |
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? |
llvm/tools/llvm-profgen/PerfReader.cpp | ||
---|---|---|
26 | nit: sample -> samples | |
785 |
This sounds good to me. We still need to parse stack samples even with IgnoreStackSamples , but we don't need to unwind them. |
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? |
Updating D109769: [llvm-profgen] Strip context to support non-CS profile generation for hybrid sample
llvm/tools/llvm-profgen/PerfReader.h | ||
---|---|---|
646 | Can this be private just like before? |
This is now defined both in LBRPerfReader and PerfReaderBase? Is that intended? And it's not virtual, and yet declared as protected..