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


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


================
Comment at: llvm/tools/llvm-profgen/PerfReader.h:488
+        Binary->getCallAddrFromFrameAddr(State.getCurrentLBRTarget());
+    return (CallAddr != 0);
+  }
----------------
Is this detecting a general return or just return from external?


================
Comment at: llvm/tools/llvm-profgen/PerfReader.h:514
+    // Check if the return source is from external code.
+    return isSourceExternal(State) && isReturnFromExternal(State);
   }
----------------
Can we have an internal-to-external return here? I'm wondering if it should be filtered out.


================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.cpp:771
+    if (CtxKey->Context.empty()) {
+      populateHeadSamplesForExternalCall(CI.second.BranchCounter);
+      continue;
----------------
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.





Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118177