[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:26
+cl::opt<bool>
+    StripContext("strip-context", cl::ReallyHidden, cl::init(false),
+                 cl::ZeroOrMore,
----------------
wenlei wrote:
> Use `ignore-stack-samples`?
Sounds good!


================
Comment at: llvm/tools/llvm-profgen/PerfReader.cpp:653
 void LBRPerfReader::generateRawProfile() {
-  assert(SampleCounters.size() == 1 && "Must have one entry of sample counter");
+  // There is no context for LBR only sample, so initialize one entry with
+  // fake "empty" context key.
----------------
wenlei wrote:
> assert SampleCounters.size is zero then?
added!


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109769