[PATCH] D89723: [AutoFDO][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.cpp:63
+  // Add extra frame as we unwind through the return
+  const LBREntry &LBR = State.getCurrentLBR();
+  uint64_t CallAddr = Binary->getCallAddrFromFrameAddr(LBR.Target);
----------------
Let `getCurrentLBR` return reference? or why would you want a temporary to bind const-reference to?


================
Comment at: llvm/tools/llvm-profgen/PerfReader.cpp:97
+  // Do not let 'random' IP from leaf frame skew LBR ranges
+  State.CallStack.front() = State.LBRStack[State.LBRIndex].Target;
+  State.InstPtr.update(State.CallStack.front());
----------------
Is this still needed now that we normalize traces before aggregation (line 259)?


================
Comment at: llvm/tools/llvm-profgen/PerfReader.cpp:106
+  // unwind the sample stack as we walk through LBR entries.
+  while (State.LBRIndex < State.LBRStack.size()) {
+    State.checkStateConsistency();
----------------
Perhaps add a wrapper `State.hasMoreLBRs()` for this?


================
Comment at: llvm/tools/llvm-profgen/PerfReader.cpp:117
+    // Save the LBR branch before it gets unwound.
+    const LBREntry &Branch = State.LBRStack[State.LBRIndex];
+
----------------
Use `getCurrentLBR` instead of `State.LBRStack[State.LBRIndex]`?


================
Comment at: llvm/tools/llvm-profgen/PerfReader.cpp:133
+      // Unwind branches - for regular intra function branches, we only
+      // need to record branch with context, push/pop for stack needed.
+      unwindBranchWithinFrame(State);
----------------
wmi wrote:
> It is needed or unneeded? Without call/ret, I assume there is no need to push/pop callstack?
There's no need to adjust stack for intra-function branches, though conceptually we still need to unwind through the LBRs (updates the `State`). Agreed that the mention of push/pop stack isn't accurate..


================
Comment at: llvm/tools/llvm-profgen/PerfReader.h:1
+//===-- PerfReader.h - perfscript reader -----------------------*- C++ -*-===//
+//
----------------
Would be good to add more comments for the classes/types defined, especially non-trivial ones. 


================
Comment at: llvm/tools/llvm-profgen/PerfReader.h:56
+  // Call stack recorded in anti-execution(leaf to entry) order
+  std::list<uint64_t> CallStack;
+  // LBR stack recorded in anti-execution order
----------------
Curious why use list for Stack but SmallVector (and Index) for LBRStack? Seem a bit inconsistent for similar use case.


================
Comment at: llvm/tools/llvm-profgen/PerfReader.h:104
+    uint64_t StackLeaf = CallStack.front();
+    if (StackLeaf < LBRLeaf || StackLeaf >= LBRLeaf + 0x100) {
+      WithColor::warning() << "Bogus trace: stack tip = "
----------------
wmi wrote:
> What is the rationale behind the condition?
Ideally we want tip of LBR target and tip of stack leaf to align with the help of PEBS. When we take a stack sample, the leaf IP of stack could be last LBR target address +N bytes, and N shouldn't be too large because N is essentially the sampling skid distance. So I had this in my original prototype as a sanity check to filter out broken records. However the distance chosen was somewhat arbitrary.. In reality, I don't think we've seen this firing with PEBS, but it could happen without PEBS, or if cycles is instead of branch_retired as triggering event. 


================
Comment at: llvm/tools/llvm-profgen/PerfReader.h:205
+
+  static bool checkHybridPerfScript(StringRef FileName) {
+    // Context-sensitive profile traces are seprated by double line break,
----------------
wmi wrote:
> Rename it to 'isCtxSensitivePerfScript'?
I think context-sensitivity is a concept that only exists at FDO profile level. Thus for perf script, we used the term hybrid which faithfully represents the fact that both LBR and stack are sampled together. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89723