[PATCH] D89723: [CSSPGO][llvm-profgen]Context-sensitive profile data generation

[Wenlei He via Phabricator via llvm-commits]

wenlei added inline comments.


================
Comment at: llvm/tools/llvm-profgen/PerfReader.h:182
+
+    // Since tail call frame is missing in stack sample, we intentionally
+    // don't want detect tail call now, otherwise we would be trying to
----------------
hoy wrote:
> The comment could be retired since we have a tail call tracker coming that tracks both in-LBR tail calls and out-of-LBR tail calls universally. 
I think the comment needs to be updated, but explanation here is still needed because IIUC missing frame inference happens more like a post process (hence somewhat orthogonal), and here `isCallState` decides the unwind operation on the stack sample (not changed by frame inference) which will always miss tail call frame (unless dwarf stack walking is used by perf).


================
Comment at: llvm/tools/llvm-profgen/llvm-profgen.cpp:37-42
   PerfReader Reader;
   Reader.run();
 
+  std::unique_ptr<ProfileGenerator> Generator =
+      ProfileGenerator::create(&Reader);
+  Generator->generateProfile();
----------------
hoy wrote:
> wlei wrote:
> > wenlei wrote:
> > > If we let ProfileGenerator be the driver, I think we should also let ProfileGenerator initiate the perf loading (line 38); otherwise if we intend to decouple them, and let PerfReader read profile outside of ProfileGenerator, then it's better only pass the loaded profile to ProfileGenerator for cleaner separation. 
> > Good suggestion, change to not include PerfReader in ProfileGenerator, then I also decoupled the unwinder from the reader. for the unwinder, the input is the aggregated hybrid sample, the output is the sample counters which is later forwarded to the generator. 
> Perhaps it's better to include the unwinder in the reader since this driver will also handle non-CS profiles in future.
> 
> The dataflow from the reader to the profile generator may need a flexible definition (currently is `Unwinder.getSampleCounters()`) for future extension. 
Agreed that unwinder better be driven by PerfReader since unwinder is something PerfReader depends on directly (vs depending on its output like ProfileGenerator on PerfReader's output). 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89723