[PATCH] D115550: [CSSPGO][llvm-profgen] Fix external address issues of perf reader (return to external addr part)

[Lei Wang via Phabricator via llvm-commits]

wlei added inline comments.


================
Comment at: llvm/tools/llvm-profgen/PerfReader.cpp:120-132
+  if (LBR.IsArtificial && CallAddr == 0) {
+    // In a callback case, a return from internal code, say A, to external
+    // runtime can happen. The external runtime can then call back to
+    // another internal routine, say B. Making an artificial branch that
+    // looks like a return from A to B can confuse the unwinder to treat
+    // the instruction before B as the call instruction. Here we detect this
+    // case if the return target is not the next inst of call inst, then we just
----------------
wenlei wrote:
> Can we move this check into `isReturnState`? This should not be considered as return state, then we would naturally fall back to `unwindBranchWithinFrame` which does what we want. We may need to rename `unwindBranchWithinFrame` to simply `unwindBranch` because this now can be inter-function too. 
That's a good point! moved to `isReturnState `


================
Comment at: llvm/tools/llvm-profgen/PerfReader.cpp:196
   if (!Cur->isDummyRoot()) {
-    if (!Stack.pushFrame(Cur)) {
+    // Truncate the context for artificial frame sine we won't do inlining for
+    // external addresses.
----------------
hoy wrote:
> nit: ... since this isn't a real call context the compiler will see
Moved to  `pushFrame`


================
Comment at: llvm/tools/llvm-profgen/PerfReader.h:298
 
-  void pushFrame(uint64_t Address) {
+  void pushFrame(uint64_t Address, bool IsArtificial = false) {
+    // Push an artificial frame for the case of returning to external address
----------------
wenlei wrote:
> Comment for the definition and purpose for artificial frame.
changed to reuse `external frame` instead of artificail frame. Added some comments near it's definition.


================
Comment at: llvm/tools/llvm-profgen/PerfReader.h:317
+      CurrentLeafFrame = CurrentLeafFrame->Parent;
+    CurrentLeafFrame = CurrentLeafFrame->Parent;
+  }
----------------
hoy wrote:
> wenlei wrote:
> > hoy wrote:
> > > nit: switch the two statements for readability? A comment would also be helpful.
> > iiuc, switching changes the semantic though - we will be checking IsArtificialFrame the grand parent instead of parent ..
> Right, the code needs to be slightly tweaked. I was thinking of reading this routine like we unwind once first. When an artificial frame is found, we unwind once more to skip that frame. Not a strong preference though.
ok, I realized that moving this check`if (CurrentLeafFrame->Parent->IsArtificialFrame())` to its caller seems better, then we can have better handling the unmatched cases.


================
Comment at: llvm/tools/llvm-profgen/PerfReader.h:433
   bool pushFrame(UnwindState::ProfiledFrame *Cur) {
+    assert(!Cur->IsArtificialFrame() && "Push an artificial frame.");
     Stack.push_back(Cur->Address);
----------------
hoy wrote:
> wenlei wrote:
> > nit: if we don't expect pushing artificial frame, the message can be explicit, something like "Artificial frame not expected for context stack."
> Or we just return false for artificial frames so that it doesn't need to be handled on the caller side?
Sounds good, changed to return false for external frame.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115550