MaskRay added a comment. > Context not available
See https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface ================ 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() ---------------- Just make RISCV32 available? Otherwise we wouldn't need `riscv32_SOURCES` ================ 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); ---------------- 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`. ================ Comment at: compiler-rt/lib/xray/xray_riscv.cpp:52 + uint32_t Rd) XRAY_NEVER_INSTRUMENT { + return (Rs2 << 20 | Rs1 << 15 | Rd << 7 | Opcode); +} ---------------- ================ Comment at: compiler-rt/lib/xray/xray_riscv.cpp:286 + const XRaySledEntry &Sled) XRAY_NEVER_INSTRUMENT { + // FIXME: Implement for riscv? + return false; ---------------- Unimplemented features should use TODO instead of FIXME. ================ 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 ---------------- 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. ================ Comment at: llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp:322 +void RISCVAsmPrinter::LowerPATCHABLE_FUNCTION_ENTER(const MachineInstr *MI) { + emitSled(MI, SledKind::FUNCTION_ENTER); ---------------- I have many comments in D140727 (LoongArch port). Many are probably useful here as well. 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
