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


================
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.
----------------
assert SampleCounters.size is zero then?


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109769