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

[Ashwin Poduval via Phabricator via llvm-commits]

ashwin98 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)
----------------
jrtc27 wrote:
> Why? You shift this whole thing left by 32 later
Right, I'll update this


================
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)
----------------
jrtc27 wrote:
> 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...
Adding 0x80000000 may be enough, the lower 12 bits should be taken care of when we construct the lower 32 bits, if we choose to use two registers. If we wish to use one register to load all values, then 0x80000800 may be needed - I'm not too sure. About the MIPS and AArch terminology in some places - yeah if there's anything that is not official or consistent with RISCV, please let me know, I frequently consulted the files for the other ISAs to figure out XRay's implementation and ended up using inconsistent terminology at some places.


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


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

https://reviews.llvm.org/D117929