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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
