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

[Lei Wang via Phabricator via llvm-commits]

wlei added inline comments.


================
Comment at: llvm/tools/llvm-profgen/PerfReader.h:488
+        Binary->getCallAddrFromFrameAddr(State.getCurrentLBRTarget());
+    return (CallAddr != 0);
+  }
----------------
hoy wrote:
> wlei wrote:
> > hoy wrote:
> > > Is this detecting a general return or just return from external?
> > This is a heuristic to only detect returning from external because the source is external(unknown), so we leverage target address checking if it's the next inst of call inst.
> > 
> > For the general return, the source is known, so we use `addressIsReturn` to determine if it's a return.
> I meant to say the name of this function doesn't exactly match its code. Maybe including the `isSourceExternal` check in this function?
Ah, I see, the function name is not clear. Renamed and merged `isSourceExternal`


================
Comment at: llvm/tools/llvm-profgen/PerfReader.h:514
+    // Check if the return source is from external code.
+    return isSourceExternal(State) && isReturnFromExternal(State);
   }
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118177