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

[Lei Wang via Phabricator via llvm-commits]

wlei added inline comments.


================
Comment at: llvm/tools/llvm-profgen/PerfReader.cpp:759
+  if (StripContext) {
+    PerfType = PERF_LBR;
+    LBRPerfReader::generateRawProfile();
----------------
hoy wrote:
> wenlei wrote:
> > 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`?
> > 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.
Sounds good to make unwinder into `HybridPerfReader `.


================
Comment at: llvm/tools/llvm-profgen/PerfReader.cpp:24
+                 cl::ZeroOrMore,
+                 cl::desc("Strip call stack sample for hybrid sample and "
+                          "produce context-insensitive profile."));
----------------
hoy wrote:
> nit: sample -> samples
fixed!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109769