[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.cpp:240
+
+  // Record external-call-internal pattern on the trie root, it later can be
+  // used for generating head samples.
----------------
hoy wrote:
> nit: external-to-internal
fixed!


================
Comment at: llvm/tools/llvm-profgen/PerfReader.cpp:856
 
-    if (!LBR.IsArtificial && TargetOffset != ExternalAddr) {
+    if (TargetOffset != ExternalOffset) {
       Counter.recordBranchCount(SourceOffset, TargetOffset, Repeat);
----------------
hoy wrote:
> Please leave a comment about why such branch is still useful, i.e, when sourceOffset is external.
comments added.


================
Comment at: llvm/tools/llvm-profgen/PerfReader.h:488
+        Binary->getCallAddrFromFrameAddr(State.getCurrentLBRTarget());
+    return (CallAddr != 0);
+  }
----------------
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.


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


================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.cpp:771
+    if (CtxKey->Context.empty()) {
+      populateHeadSamplesForExternalCall(CI.second.BranchCounter);
+      continue;
----------------
hoy wrote:
> Can this be fused into `populateBoundarySamplesForFunction`? 
> 
> Looking at how this is handled on the non-CS side, especially in `populateBoundarySamplesForAllFunctions`, there is a check `    if (!FrameVec.empty()) `, can we do something similar in `populateBoundarySamplesForFunction`? Of course we need the check here to bypass `populateBodySamplesForFunction` call, or perhaps we can move the `getFunctionProfileForContext` call into the populating functions.
> 
> 
> 
Yes, fused into `populateBoundarySamplesForFunction`.

A little difference is I didn't move `getFunctionProfileForContext ` into the populating functions because this is an optimization we did before to reuse the `FunctionSamples` of one context, otherwise we need to fetch the `FunctionSamples` twice(one in populateBodySamplesForFunction, one in populateBoundarySamplesForFunction). 

Here I just directly bypassed the `populateBodySamplesForFunction ` if the context is empty.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118177