[PATCH] D153320: [XRay][AArch64] Suppport __xray_customevent/__xray_typedevent

[Peter Smith via Phabricator via llvm-commits]

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