[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:239
+  if (Branch.Source == ExternalAddr) {
+    State.getDummyRootPtr()->recordBranchCount(Branch.Source, Branch.Target,
+                                               Repeat);
----------------
hoy wrote:
> This can be an interrupt branch which does not correspond to any external frame on the call stack. Once such a branch is see, should the state have already been reset to dummy? I'm not seeing the resetting code anywhere.  Maybe the stack resetting code being removed from `VirtualUnwinder::unwindCall can be reused to handle such case?
Good point. There can be two patterns for the interrupt branch, I fixed them in other parts.

1) LBR target is an external address caused by interrupt. This will be a regular branch, Resetting the call stack there if a regular branch's target is external address. see PerfReader.cpp:293


2) LBR source is an external address caused by interrupt. This will go into `unwindCall`, we will find a mismatch call source and the call stack leaf. Resetting the call stack there. see PerfReader.cpp:67


================
Comment at: llvm/tools/llvm-profgen/PerfReader.cpp:284
+        // Push an extra external frame on the call stack.
+        unwindReturn(State);
+      } else {
----------------
hoy wrote:
> Would be nice to make the two cases fit into the call and return case below, respectively.
Sounds good. Merging all the code into `isCallState` or `isReturnState`, now the four pattern: external-call-internal/internal-call-external/external-return-internal/internal-return-external are all handled by them.


================
Comment at: llvm/tools/llvm-profgen/ProfiledBinary.h:81
   // isn't a real call context the compiler will see.
-  ExternalAddr = 1,
+  ExternalAddr = 0x1000000000000000ULL,
 };
----------------
hoy wrote:
> This could be a potential legal address. Use UINT64_MAX instead?
Okay, Before I didn't use UINT64_MAX because there can be add operations to address value and adding value to UINT64_MAX caused overflow, we might always keep in mind of this. I changed to UINT64_MAX and fixed one overflow issue, also wondering if we have a better number for this so that overflow can't be issues in the future.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118177