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

[Wenlei He via Phabricator via llvm-commits]

wenlei added inline comments.


================
Comment at: llvm/tools/llvm-profgen/PerfReader.cpp:759
+  if (StripContext) {
+    PerfType = PERF_LBR;
+    LBRPerfReader::generateRawProfile();
----------------
wlei wrote:
> wenlei wrote:
> > 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. 
> 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?
> 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`?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109769/new/

https://reviews.llvm.org/D109769