[PATCH] D118177: [llvm-profgen] Decouple artificial branch from LBR parser and fix external address related issues

[Hongtao Yu via Phabricator via llvm-commits]

hoy added inline comments.


================
Comment at: llvm/tools/llvm-profgen/PerfReader.cpp:293
       unwindReturn(State);
-    } else if (isValidState(State)) {
+    } else if (isValidState(State) && !isTargetExternal(State)) {
       // Unwind branches
----------------
What kind of branch is not a call or return but with an external target address?


================
Comment at: llvm/tools/llvm-profgen/PerfReader.h:514
+    // Check if the return source is from external code.
+    return isSourceExternal(State) && isReturnFromExternal(State);
   }
----------------
wlei wrote:
> hoy wrote:
> > wlei wrote:
> > > hoy wrote:
> > > > Can we have an internal-to-external return here? I'm wondering if it should be filtered out.
> > > The  internal-to-external return pattern is handled by the right above branch(`if (Binary->addressIsReturn(State.getCurrentLBRSource()))`) which checking the source if it's a return.
> > Ok, a return to external is considered as a return. But in `unwindReturn`, the pushing external frame code is being removed. Should we keep it?
> Before the `ArtificialBranch` is actually from two branches [internal-to-external, external-to-internal], so at that time, we take two operations in one call to `unwindReturn`, first push an external frame, then push an internal address. the stack is [external, internal]
> 
> Now after decoupling `ArtificialBranch` into two branches, it calls `unwindReturn` twice: In the first call to `unwindReturn `, say the internal-to-external will push the first address which is the external address(the stack is [external]) and for the second call to `unwindReturn `, the external-to-internal will push the internal address.(the stack is [external, internal]).
> 
> The current one and the previous are expected to be equivalent, just right now it's split into two steps.
> 
> Same to`unwindCall`, you can also find it removes the code to pop the extra external frame.
Thanks for the explanation. I was trying to enumerate new cases we may see here. Previously with artificial branches, unpaired internal/external transitions may be thrown away, as well as samples following them. Now with this change, we never throw away any samples except for external/external transitions. This means the unwinder will see new patterns like unpaired I/E or E/I transitions, and I'm trying to make sense of them.


Perhaps we should bring this offline for a complete understanding and verification of the problem (you may already did that).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118177