[PATCH] D117929: [XRay] Add support for RISCV

[Fangrui Song via Phabricator via cfe-commits]

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