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


================
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);
----------------
wenlei wrote:
> This is now defined both in LBRPerfReader and PerfReaderBase? Is that intended? And it's not virtual, and yet declared as protected.. 
removed


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109769