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

[Ashwin Poduval via Phabricator via cfe-commits]

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