This is mostly straightforward, with an additional optimization: XPACIuntied allows strips of LR (which we emit in __builtin_return_address) to avoid clobbering LR. (This can be useful, e.g., for a certain kind of wrapper functions that just forward the caller address, for logging and such)
I guess having an enum for the valid key descriptors for AArch64 would make this and other code working with keys more readable. I'm not sure if that's typically done or where to define that enum?
I see these constants 0,1,2,3 are also needed in tablegen files. Not sure if something similar to an enum exists in Tablegen?
|1227 ↗||(On Diff #454524)|
Yeah, XPACIuntied is really intended to be used in @llvm.returnaddress when return address signing (/PAC-RET) is enabled. I pulled it out of this patch into that one, where it actually gets exercised.
A later patch in the series actually does introduce helper functions in AArch64BaseInfo.h/AArch64InstrInfo.h, so it's a simple matter of using them here! I combined the two in this review, to be committed separately.
I just added a few minor nitpicks - I'll leave it to you to decide if following those suggestions would result in an improvement or not.
|514 ↗||(On Diff #456771)|
nitpick, maybe do s/ptrauth auth/pointer authentication/?
|526 ↗||(On Diff #456771)|
nitpick, maybe do s/ptrauth auth/pointer signing/?
|798 ↗||(On Diff #456771)|
I guess that if we added "LAST = 3" here, the somewhat cryptic if statement in AArch64InstructionSelector.cpp that says
if (Key > 3) return false;
could instead say something like
if (AArch64PACKey::LAST > 3) return false;
It seems the word "LAST" is already used in a few other places in LLVM to indicate the highest defined value in an enum. Maybe doing this change would be a minor improvement in readability/maintainability?