[PATCH] D140727: [XRay] Add initial support for loongarch64

[WÁNG Xuěruì via Phabricator via cfe-commits]

xen0n added inline comments.


================
Comment at: compiler-rt/lib/xray/xray_loongarch64.cpp:22-25
+  RN_T0 = 0xC,
+  RN_T1 = 0xD,
+  RN_RA = 0x1,
+  RN_SP = 0x3,
----------------
I think people usually just declare the register ABI names with decimal numbers for readability?

This is minor but I personally find it slightly distracting to have to think about what's `0xc` in decimal (that's the case even though I'm already very familiar with these kinds of thing). Not sure if others also prefer decimals though...


================
Comment at: compiler-rt/lib/xray/xray_loongarch64.cpp:32
+                       uint32_t Imm) XRAY_NEVER_INSTRUMENT {
+  return (Opcode | Rj << 5 | Rd | Imm << 10);
+}
----------------
Parens not needed in this case. (If you want to enhance readability for those people having difficulties remembering precedence and associativeness of operators, you should instead put the parens around `Rj << 5` and `Imm << 10`...)

I'd suggest re-ordering the operands too, to imitate the expected bit layout of the result.


================
Comment at: compiler-rt/lib/xray/xray_loongarch64.cpp:39
+                       uint32_t Imm) XRAY_NEVER_INSTRUMENT {
+  return (Opcode | Rd | Imm << 5);
+}
----------------
Similarly here.


================
Comment at: compiler-rt/lib/xray/xray_loongarch64.cpp:42-47
+// Encode instructions in 2RI16 format, e.g. jirl.
+inline static uint32_t
+encodeInstruction2RI16(uint32_t Opcode, uint32_t Rd, uint32_t Rj,
+                       uint32_t Imm) XRAY_NEVER_INSTRUMENT {
+  return (Opcode | Rj << 5 | Rd | Imm << 10);
+}
----------------
Does this look *very* similar to `encodeInstruction2RI12`? ;-)

Actually, if you don't bounds-check the operands, you only need one helper for each combination of slots involved. Check for example the `static uint32_t insn` implementation in my D138135, or [[ https://gitlab.com/qemu-project/qemu/-/blob/master/tcg/loongarch64/tcg-insn-defs.c.inc | the auto-generated encoding helpers for the QEMU TCG LoongArch64 port ]], for some alternative approaches.


================
Comment at: compiler-rt/lib/xray/xray_loongarch64.cpp:58
+  //	11 NOPs (44 bytes)
+  //	.tmpN
+  //
----------------
`.tmpN:` for it to not look like a directive?


================
Comment at: compiler-rt/lib/xray/xray_loongarch64.cpp:71
+  //   ori     t1, t1, %abs_lo12(function_id)    ;pass function id
+  //   jirl    ra, t0, 0                         ;call Tracing hook
+  //   ld.d    ra, sp, 8                         ;restore return address
----------------
All-lowercase i.e. `tracing` for consistency? Or am I missing something?


================
Comment at: compiler-rt/lib/xray/xray_loongarch64.cpp:73
+  //   ld.d    ra, sp, 8                         ;restore return address
+  //   addi.d  sp, sp, 16                        ;delete stack frame
+  //
----------------
I think idiomatically it's not "create/delete stack frame" but rather "(de-)allocate" or "setup/teardown"... anyway please fix the usage of articles (put the "the"'s properly) and also add a space after the `;` because it's usually aesthetically better.

This is all minor though, and it's not like people will misunderstand anything otherwise, but rather it's mostly just me pushing for more natural English usage.


================
Comment at: compiler-rt/lib/xray/xray_loongarch64.cpp:80
+  //
+  // When |Enable|==false, we set back the first instruction in the sled to be
+  //   B #48
----------------
("set back" happens to be an idiomatic phrase verb so I got confused here for ~1 second, so I think it's probably easier to just avoid this coincidence.)


================
Comment at: compiler-rt/lib/xray/xray_trampoline_loongarch64.S:83
+  // a1=1 means that we are tracing an exit event
+  ori     $a1, $zero, 1
+  // Function ID is in t1 (the first parameter).
----------------
Use pseudo-instruction for idiomatic expression of intent.


================
Comment at: llvm/lib/Target/LoongArch/LoongArchAsmPrinter.cpp:152-177
+  const int8_t NoopsInSledCount = 11;
+  // For loongarch64 we want to emit the following pattern:
+  //
+  // .Lxray_sled_beginN:
+  //   ALIGN
+  //   B .Lxray_sled_endN
+  //   11 NOP instructions (44 bytes)
----------------
We don't have to repeat the instruction pattern when we can just refer people to the source file containing this. Also the comment seems to be  explanation for the magic number 11, so it should probably come before the definition.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140727