[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:787
+void HybridPerfReader::generateRawProfile() {
+  if (!IgnoreStackSamples) {
+    ProfileIsCS = true;
----------------
nit: make sure we update ProfileIsCS properly for both cases and not relying on current value. 

```
ProfileIsCS = !IgnoreStackSamples;
if (ProfileIsCS)
  unwindSamples();
else
  LBRPerfReader::generateRawProfile();
```  


================
Comment at: llvm/tools/llvm-profgen/PerfReader.h:615
   virtual void parseSample(TraceStream &TraceIt, uint64_t Count) = 0;
-  // Post process the profile after trace aggregation, we will do simple range
-  // overlap computation for AutoFDO, or unwind for CSSPGO(hybrid sample).
-  virtual void generateRawProfile() = 0;
+  void computeCounterFromLBR(const PerfSample *Sample, uint64_t Repeat);
   void writeRawProfile(StringRef Filename);
----------------
This is now defined both in LBRPerfReader and PerfReaderBase? Is that intended? And it's not virtual, and yet declared as protected.. 


================
Comment at: llvm/tools/llvm-profgen/llvm-profgen.cpp:92
   Reader->parsePerfTraces(PerfTraceFilenames);
+  Reader->generateRawProfile();
 
----------------
Can we revert this change too and move `generateRawProfile` back into `parsePerfTraces`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109769