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

Wed Apr 27 23:10:39 PDT 2022

wlei added inline comments.

================
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:
> wlei wrote:
> > wenlei wrote:
> > > 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?
> > This is the address not offset, it will be minus the base address, this is more common than "add" overflow. Both 1 and UINT64_MAX can overflow, that's why I choose the value in the middle 0x1000000000000000ULL. But I'm fine with either value.
> How can UINT64_MAX overflow? Do we add anything to it? 
> 
> 
> I guess with value 1 we would need special handling when converting it to offset right?
> 
> Actually IIRC, the upper half of the 64-bit mem space, i.e, FFFF8000'00000000 through FFFFFFFF'FFFFFFFF, is used as kernel space on Linux, so using UINT64_MAX to represent external address should be fine.
> 
Uh.. when it detect bogus trace ` LeafAddr >= LBRLeaf + 0x100`, I already fixed this.

yeah, UINT64_MAX overflow are very unlikely, it should be fine.

1 is also okay I guess, minus a number, then become a negative value, but still doesn't conflict to binary offset.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118177