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
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
