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

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to