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

Tue Jan 25 18:55:33 PST 2022

hoy added a comment.

Thanks for working on this! The refactoring does make the code base a lot cleaner.

================
Comment at: llvm/tools/llvm-profgen/PerfReader.cpp:239
+  if (Branch.Source == ExternalAddr) {
+    State.getDummyRootPtr()->recordBranchCount(Branch.Source, Branch.Target,
+                                               Repeat);
----------------
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?

================
Comment at: llvm/tools/llvm-profgen/PerfReader.cpp:284
+        // Push an extra external frame on the call stack.
+        unwindReturn(State);
+      } else {
----------------
Would be nice to make the two cases fit into the call and return case below, respectively.

================
Comment at: llvm/tools/llvm-profgen/ProfiledBinary.h:81
   // isn't a real call context the compiler will see.
-  ExternalAddr = 1,
+  ExternalAddr = 0x1000000000000000ULL,
 };
----------------
This could be a potential legal address. Use UINT64_MAX instead?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118177