[PATCH] D153320: [XRay][AArch64] Suppport __xray_customevent/__xray_typedevent
Peter Smith via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Jun 21 07:44:49 PDT 2023
peter.smith added a comment.
I'm not familiar with xray, ideally someone that knows that better will be able to spot anything I've missed. On the AArch64 instruction side it looks good. Maybe better to use a mov rather than an add if the addend is always zero. If we can't find anyone familiar with xray to review I'm happy to approve as I think this looks like an improvement, and is not at risk of breaking anything.
================
Comment at: compiler-rt/lib/xray/xray_AArch64.cpp:109
bool patchCustomEvent(const bool Enable, const uint32_t FuncId,
- const XRaySledEntry &Sled)
- XRAY_NEVER_INSTRUMENT { // FIXME: Implement in aarch64?
+ const XRaySledEntry &Sled) XRAY_NEVER_INSTRUMENT {
+ // Enable: jmp +24 => nop
----------------
For the AArch64 specific file I think it would be better to say
// B . + 24 => nop
// nop => B . + 24
As B is the AArch64 instruction that matches 0x14......
Is it worth mentioning why 24 and 36 in this file (2 or 3 parameters). This is mentioned in AArch64AsmPrinter.cpp but that is separate from compiler-rt.
================
Comment at: llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp:343
+ O.emitLabel(CurSled);
+ MCInst MovX0Op0 = MCInstBuilder(AArch64::ADDXri)
+ .addReg(AArch64::X0)
----------------
IIUC these two instructions are `add x0, <rn>, #0` and `add x1, <rn>, #0` if the addend is always 0 then while this will work it isn't idiomatic. Would a mov instruction work? I think this may be an alias of the register form with one of the sources the zero register Xzr.
================
Comment at: llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp:374
+ EmitToStreamer(O, MovX1Op1);
+ EmitToStreamer(O, MCInstBuilder(AArch64::ADDXri)
+ .addReg(AArch64::X2)
----------------
Similar comment about whether MOV could be used here?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D153320/new/
https://reviews.llvm.org/D153320
More information about the llvm-commits
mailing list