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

[Limin Zhang via Phabricator via llvm-commits]

Ami-zhang 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,
----------------
xen0n wrote:
> 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...
Ok,  use decimal numbers to declare the register ABI names.


================
Comment at: compiler-rt/lib/xray/xray_loongarch64.cpp:32
+                       uint32_t Imm) XRAY_NEVER_INSTRUMENT {
+  return (Opcode | Rj << 5 | Rd | Imm << 10);
+}
----------------
xen0n wrote:
> 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.
What you said makes sense. Thanks for your suggestion and attentiveness. I have adjusted parens and operands order.


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


================
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);
+}
----------------
xen0n wrote:
> 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.
It is indeed similar to `encodeInstruction2RI12`. Given its exclusive use in this context, I prefer the unified `2RIx` format.

Thanks for the suggestion. While we appreciate it, we are more inclined to use one `2RIx` format helper here.


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


================
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
----------------
xen0n wrote:
> All-lowercase i.e. `tracing` for consistency? Or am I missing something?
It should be a copy from `mips`. I think it can be written `tracing hook` or `TracingHook`. To maintain all-lowercase consistency here, I use `tracing hook`.


================
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
+  //
----------------
xen0n wrote:
> 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.
Ok, a space after the `;`   is added.  About article `the`,  i also added it. But i am not sure whether the usage of `the` is proper. If  there are any further issues , I would greatly appreciate your feedback.

Thanks for your effort.


================
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
----------------
xen0n wrote:
> ("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.)
Done.


================
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).
----------------
xen0n wrote:
> Use pseudo-instruction for idiomatic expression of intent.
Done.


================
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)
----------------
xen0n wrote:
> 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.
Done.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140727