ashwin98 added inline comments.
================
Comment at: compiler-rt/cmake/Modules/AllSupportedArchDefs.cmake:83
set(ALL_XRAY_SUPPORTED_ARCH ${X86_64} ${ARM32} ${ARM64} ${MIPS32} ${MIPS64}
- powerpc64le ${HEXAGON} ${LOONGARCH64})
+ powerpc64le ${HEXAGON} ${LOONGARCH64} #[[${RISCV32}]]
${RISCV64})
endif()
----------------
MaskRay wrote:
> Just make RISCV32 available? Otherwise we wouldn't need `riscv32_SOURCES`
Done
================
Comment at: compiler-rt/lib/xray/xray_riscv.cpp:51
+encodeRTypeInstruction(uint32_t Opcode, uint32_t Rs1, uint32_t Rs2,
+ uint32_t Rd) XRAY_NEVER_INSTRUMENT {
+ return (Rs2 << 20 | Rs1 << 15 | Rd << 7 | Opcode);
----------------
MaskRay wrote:
> For new functions, consider dropping `XRAY_NEVER_INSTRUMENT`. The runtime
> library cannot be instrumented with `-fxray-instrument` and I an unsure why
> the original impl adds a lot of `XRAY_NEVER_INSTRUMENT`.
Done
================
Comment at: compiler-rt/lib/xray/xray_riscv.cpp:52
+ uint32_t Rd) XRAY_NEVER_INSTRUMENT {
+ return (Rs2 << 20 | Rs1 << 15 | Rd << 7 | Opcode);
+}
----------------
MaskRay wrote:
>
Done
================
Comment at: compiler-rt/lib/xray/xray_riscv.cpp:286
+ const XRaySledEntry &Sled) XRAY_NEVER_INSTRUMENT {
+ // FIXME: Implement for riscv?
+ return false;
----------------
MaskRay wrote:
> Unimplemented features should use TODO instead of FIXME.
Done
================
Comment at: compiler-rt/lib/xray/xray_riscv.cpp:162
+ uint32_t HighestTracingHookAddr =
+ (reinterpret_cast<int64_t>(TracingHook + 0x800) >> 44) & 0xfffff;
+ // We typecast the Tracing Hook to a 32 bit value for RISCV32
----------------
MaskRay wrote:
> ashwin98 wrote:
> > jrtc27 wrote:
> > > This is definitely wrong; you probably mean `(((TracingHook >> 32) +
> > > 0x800) >> 12) & 0xfffff`?
> > True
> `(x + 0x800) >> 12` is used many times. We need a helper like
> `lld/ELF/Arch/RISCV.cpp hi20`. Ditto for lo12.
Done. I think I'd frequently looked at the MIPS trampolines while writing this
code - I noticed that they were casting the Tracing Hook to int64_t, instead of
uint64_t (and int32/uint32 for the 32 bit ISA), which requires the & operation,
since >> is an arithmetic shift operation for signed integers. Changing the
cast operations to uint64_t should eliminate those & operations without
breaking anything else, right?
================
Comment at: llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp:322
+void RISCVAsmPrinter::LowerPATCHABLE_FUNCTION_ENTER(const MachineInstr *MI) {
+ emitSled(MI, SledKind::FUNCTION_ENTER);
----------------
MaskRay wrote:
> I have many comments in D140727 (LoongArch port). Many are probably useful
> here as well.
I had a look at this. Two things which stuck out to me (both in xray_riscv.cpp):
- changing inline static to static inline (done)
- your note about the PO_ style hurting readability when instructions are only
used once. In this case, we're using some instructions repeatedly, so I'm
guessing it makes sense to continue with the enum, but I can get rid of it if
that works better.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D117929/new/
https://reviews.llvm.org/D117929
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
