[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.h:488
+        Binary->getCallAddrFromFrameAddr(State.getCurrentLBRTarget());
+    return (CallAddr != 0);
+  }
----------------
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?


================
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:
> > 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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118177