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

[Wenlei He via Phabricator via llvm-commits]

wenlei added inline comments.


================
Comment at: llvm/tools/llvm-profgen/PerfReader.h:493
+  // as a call from external.
+  bool isCallFromExternal(UnwindState &State) const {
+    return isSourceExternal(State) &&
----------------
Check isValidState first.


================
Comment at: llvm/tools/llvm-profgen/PerfReader.h:510
 
   bool isReturnState(UnwindState &State) const {
     if (!isValidState(State))
----------------
Good that we don't need to check for artificial return, but perhaps we could assert that the target is next to a call for internal targets? 


================
Comment at: llvm/tools/llvm-profgen/ProfiledBinary.h:81
   // isn't a real call context the compiler will see.
-  ExternalAddr = 1,
+  ExternalAddr = 0x1000000000000000ULL,
 };
----------------
wlei wrote:
> 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.
Technically, both and 1 and UINT64_MAX can be legal address, but practically that probably will never happen. While overflow is an issue with UINT64_MAX, I think 1 is fine?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118177