[PATCH] D115538: [CSSPGO][llvm-profgen] Fix external address issues of perf reader (leading external LBR part)

[Wenlei He via Phabricator via llvm-commits]

wenlei added inline comments.


================
Comment at: llvm/test/tools/llvm-profgen/inline-noprobe2.test:97
 ;CHECK:  65499: 0
+;CHECK: quick_sort:903:25
+;CHECK:  1: 24
----------------
Ok, so this happens with existing test cases too. I didn't realize the entire function profile for small function could be missing due to external addresses even in contrived tests. Did you verify what is the external address (what function from what library) leads to samples involving `quick_sort` being ignored? 


================
Comment at: llvm/tools/llvm-profgen/PerfReader.cpp:258-259
+    // external address(kernel, dynamic library), the call stack is truncated
+    // since we can not optimize code outside of the binary. The remaining LBRs
+    // continue to be unwinded using a context-less frame stack.
+    if (IsLeaf && State.CurrentLeafFrame->isExternalFrame()) {
----------------
A bit confused by the comment on "unwinded using a context-less frame stack" - I thought after we strip out the leading external frame and external LBR, we would still use the remaining stack as context, hence it's not context-less. 


================
Comment at: llvm/tools/llvm-profgen/PerfReader.cpp:261
+    if (IsLeaf && State.CurrentLeafFrame->isExternalFrame()) {
+      State.InstPtr.update(State.getCurrentLBRSource());
+      State.switchToFrame(State.getCurrentLBRSource());
----------------
I think we can do some sanity check here:
1. assert that we have a paired LBR here? ie. LBR target is external.
2. also assert that the next frame on stack aligns with LBR source?


================
Comment at: llvm/tools/llvm-profgen/PerfReader.cpp:262
+      State.InstPtr.update(State.getCurrentLBRSource());
+      State.switchToFrame(State.getCurrentLBRSource());
+      State.advanceLBR();
----------------
Should this be `pushFrame`/`popFrame` instead of `switchToFrame` depending on the instruction at the source of LBR? 
Basically we are unwinding through the internal->external LBR here, which should be either a call or return, right?


================
Comment at: llvm/tools/llvm-profgen/PerfReader.cpp:624
+        NumTopExternalFrame++;
+        CallStack.emplace_back(UnwindState::ProfiledFrame::ExternalAddr);
+      }
----------------
If only leaf frames are external, can we keep the remaining frames for unwinding?


================
Comment at: llvm/tools/llvm-profgen/PerfReader.cpp:979
+  else {
+    NumTotalSample++;
     parseSample(TraceIt);
----------------
nit: move this into `parseSample`


================
Comment at: llvm/tools/llvm-profgen/PerfReader.cpp:1165
 
+  emitWarningSummary(NumTopExternalFrame, NumTotalSample,
+                     "of samples have top external frame in call stack.");
----------------
nit: top->leaf to make it explicit, for message and variable name. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115538