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

[Jessica Clarke via Phabricator via llvm-commits]

jrtc27 added inline comments.


================
Comment at: compiler-rt/lib/xray/xray_riscv.cpp:122-123
+  //    lui t2, %highest(__xray_FunctionEntry/Exit)
+  //    slli t2, t2, 32                                 ;lui sign extends values
+  //    srli t2, t2, 32                                 ;set upper 32 bits to 0
+  //    addi t2, t2, %higher(__xray_FunctionEntry/Exit)
----------------
Why? You shift this whole thing left by 32 later


================
Comment at: compiler-rt/lib/xray/xray_riscv.cpp:127-128
+  //    lui t1, t1, %hi(__xray_FunctionEntry/Exit)
+  //    slli t1, t1, 32                                 ;lui sign extends values
+  //    srli t1, t1, 32                                 ;set upper 32 bits to 0
+  //    addi t1, t1, %lo(__xray_FunctionEntry/Exit)
----------------
You might be able to avoid this by adding 0x80000800 before computing "%higher" and "%highest" (feels rather MIPSy... not official things)? Not sure, would need to think this through more, but it feels like it should be possible...


================
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
----------------
This is definitely wrong; you probably mean `(((TracingHook >> 32) + 0x800) >> 12) & 0xfffff`?


================
Comment at: llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp:105-106
 
+  // Handle XRay sleds while keeping changes minimal to avoid breaking
+  // functionality
+  switch (MI->getOpcode()) {
----------------
"Keeping changes minimal" seems like the kind of thing that belongs in a commit message, not a code comment


================
Comment at: llvm/test/CodeGen/RISCV/xray-attribute-instrumentation.ll:1-2
+; RUN: llc -mtriple=riscv32-unknown-elf -mattr=+d -verify-machineinstrs < %s | FileCheck --check-prefix=CHECK %s
+; RUN: llc -mtriple=riscv32-unknown-linux-gnu -mattr=+d -verify-machineinstrs < %s | FileCheck --check-prefix=CHECK %s
+; RUN: llc -mtriple=riscv64-unknown-elf -mattr=+d -verify-machineinstrs < %s | FileCheck --check-prefix=CHECK --check-prefix=CHECK-RISCV64 %s
----------------
Triples are overly verbose; riscv32-unknown-elf is normally just written riscv32, and riscv32-unknown-linux-gnu as riscv64-linux-gnu, though I don't see what point having both serves, we normally only use the bare-metal triples unless something has an OS-specific aspect


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D117929/new/

https://reviews.llvm.org/D117929