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

[Limin Zhang via Phabricator via cfe-commits]

Ami-zhang added inline comments.


================
Comment at: compiler-rt/lib/xray/xray_loongarch64.cpp:23
+enum PatchOpcodes : uint32_t {
+  PO_ADDID = 0x02c00000,  // addi.d rd, rj, imm
+  PO_SD = 0x29c00000,     // st.d rd, base, offset
----------------
MaskRay wrote:
> I think the `PO_` style actually harms readability. Most instructions are used just once. It's more readable just using the magic number when it is needed paired with an inline comment
> 
> `Address[10] = insn2RI12(0x02c00000, RegNum::RN_S, ...) // addi.d ...`
Ok, updated it. Thanks for your suggestion.


================
Comment at: compiler-rt/lib/xray/xray_loongarch64.cpp:153
+                           const XRaySledEntry &Sled) XRAY_NEVER_INSTRUMENT {
+  // FIXME: In the future we'd need to distinguish between non-tail exits and
+  // tail exits for better information preservation.
----------------
MaskRay wrote:
> I know you copy the pattern from other patterns, but we should use TODO here. An unimplemented minor feature is not FIXME.
Done.


================
Comment at: compiler-rt/lib/xray/xray_trampoline_loongarch64.S:27
+  .cfi_offset 1, -8
+  st.d    $a7, $sp, 120
+  st.d    $a6, $sp, 112
----------------
MaskRay wrote:
> You can use `.irpc` to simplify this a bit. See libunwind/src/UnwindRegistersSave.S
Done. 


================
Comment at: compiler-rt/test/xray/TestCases/Posix/arg1-arg0-logging.cpp:9
 // TODO: Support these in ARM and PPC
-// XFAIL: target={{(arm|aarch64|mips).*}}
+// XFAIL: target={{(arm|aarch64|loongarch64|mips).*}}
 // UNSUPPORTED: target=powerpc64le{{.*}}
----------------
MaskRay wrote:
> I added aarch64 support. You may rebase and add loongarch64 on the `REQUIRES:` line.
Rebased.


================
Comment at: compiler-rt/test/xray/TestCases/Posix/basic-filtering.cpp:26
 //
-// REQUIRES: x86_64-target-arch
+// REQUIRES: x86_64-target-arch || loongarch64-target-arch
 // REQUIRES: built-in-llvm-tree
----------------
MaskRay wrote:
> Most `REQUIRES: x86_64-target-arch` are unnecessarily constrained. I just removed them. You may rebase and avoid changes to these test files.
Rebased.


================
Comment at: llvm/lib/Target/LoongArch/LoongArchAsmPrinter.cpp:187
+  OutStreamer->emitLabel(EndOfSled);
+  recordSled(BeginOfSled, MI, Kind); // FIXME: use version 2
 }
----------------
SixWeining wrote:
> should use version 2:
> 
> ```
> recordSled(BeginOfSled, MI, Kind, 2);
> ```
Yes, updated it.


================
Comment at: llvm/test/CodeGen/LoongArch/xray-attribute-instrumentation.ll:11
+; CHECK-NEXT:  b .Lxray_sled_end0
+; CHECK-NEXT:  nop
+; CHECK-NEXT:  nop
----------------
SixWeining wrote:
> CHECK-COUNT-11
Done.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140727