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

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added a comment.

1. Please upload patches with full context
2. You should not need to have separate xray_riscv32/64.cpp, a single shared file with a small amount of XLEN-based conditions should suffice and reduce a whole load of code duplication. Possibly also applies to the trampoline assembly but maybe not, there are lots of constants and lw/ld's in there... though would be nice if that were macro'd so they don't get out of sync
3. Why comment out riscv32? At least uncomment everything except the one CMake place that says it exists, having ~10 different places with commented-out code is ugly.



================
Comment at: compiler-rt/cmake/Modules/AllSupportedArchDefs.cmake:77
 set(ALL_XRAY_SUPPORTED_ARCH ${X86_64} ${ARM32} ${ARM64} ${MIPS32} ${MIPS64}
-		powerpc64le ${HEXAGON})
+		powerpc64le ${HEXAGON} #[[${RISCV32}]] ${RISCV64})
 endif()
----------------
Does that actually only comment out RISCV32? Phabricator's syntax highlighting thinks not, but it could just be overly simplistic.


================
Comment at: compiler-rt/lib/xray/xray_riscv32.cpp:11
+//
+// Implementation of riscv32-specific routines (64-bit).
+//
----------------
You sure about that 64-bit?


================
Comment at: compiler-rt/lib/xray/xray_riscv32.cpp:24
+  PO_ADDI = 0x00000013,   // addi rd, rs1, imm
+  PO_ADD =   0x00000033,  // add rd, rs1, rs2
+  PO_SW = 0x00002023,     // sw rt, base(offset)
----------------
Whitespace here is a mess


================
Comment at: compiler-rt/lib/xray/xray_riscv32.cpp:33
+  PO_B = 0x0000006f,      // jal #n_bytes
+  PO_NOP = 0x00000013,    // nop - pseduo-instruction, same as addi r0, r0, 0
+};
----------------
Register names have an x prefix not an r prefix


================
Comment at: compiler-rt/lib/xray/xray_riscv32.cpp:96
+  // xray_sled_n (64-bit):
+  //    addi sp, sp, -16                                                        ;create stack frame
+  //    sw ra, 12(sp)                                                           ;save return address
----------------
Why are these comments way off to the right like that? This is borderline unreadable.


================
Comment at: compiler-rt/lib/xray/xray_riscv32.cpp:101
+  //    sw a0, 0(sp)                                                            ;save register a0
+  //    if higher was negative, i.e msb was 1 
+  //    lui t1, %hi(__xray_FunctionEntry/Exit) + 1 else lui t1, %hi(__xray_FunctionEntry/Exit)
----------------
No. %hi(0x800) *is* 1. Having the if/else results in double-counting were this to be treated as actual assembly. The +1 is part of how hi/lo relocation pairs are defined.


================
Comment at: compiler-rt/lib/xray/xray_riscv32.cpp:127
+    uint32_t HiTracingHookAddr =
+        (reinterpret_cast<int64_t>(TracingHook) >> 12) & 0xfffff;
+    uint32_t LoFunctionID = FuncId & 0xfff;
----------------
is how you implement hi/lo pairs in a branchless manner, exploiting carry propagation


================
Comment at: llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp:255
+void RISCVAsmPrinter::emitSled(const MachineInstr *MI, SledKind Kind) {
+  const uint8_t NoopsInSledCount = MI->getParent()->getParent()->getSubtarget<RISCVSubtarget>().is64Bit() ? 24 : 14;
+  // We want to emit the jump instruction and the nops
----------------
These magic numbers need explaining


================
Comment at: llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp:257
+  // We want to emit the jump instruction and the nops
+  // constituting the sled. The format will be similar
+  // .Lxray_sled_N
----------------
This sentence ends abruptly (and why is the paragraph wrapped at such a tiny character count?)


================
Comment at: llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp:260
+  //   ALIGN
+  //   J #100 or #60 bytes (depending on ISA)
+  //   29 or 18 NOP instructions
----------------
`#` before immediates is an AArch64-ism, on RISC-V the syntax is just the plain integer


================
Comment at: llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp:261
+  //   J #100 or #60 bytes (depending on ISA)
+  //   29 or 18 NOP instructions
+  // .tmpN
----------------
This doesn't match what you actually do


================
Comment at: llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp:272
+  // start of function. I wasn't sure which immediate to add, therefore used
+  // addExpr to the Target label, which is also more readable.
+  EmitToStreamer(*OutStreamer, MCInstBuilder(RISCV::JAL)
----------------
addExpr seems fine? That's what you use for an MCSRE. Don't see why it needs commentary here.


================
Comment at: llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp:276
+		                   .addExpr(TargetExpr));
+		                   //.addImm(NoopsInSledCount*4));
+		                   //.addImm(NoopsInSledCount*2));
----------------
?


================
Comment at: llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp:284
+			                   .addReg(RISCV::X0)
+		                           .addImm(0));
+
----------------
Indentation is off


================
Comment at: llvm/test/CodeGen/RISCV/xray-attribute-instrumentation.ll:1
+; RUN: llc -filetype=asm -o - -mtriple=riscv32-unknown-elf < %s | FileCheck --check-prefix=CHECK %s
+; RUN: llc -filetype=asm -o - -mtriple=riscv32-unknown-linux-gnu  < %s | FileCheck --check-prefix=CHECK %s
----------------
-filetype=asm is redundant, as is -o -, and you should probably be passing -verify-machineinstrs like our other tests.

Can this be made to work with update_llc_test_checks.py? My guess is probably not due to the filtering it does, but it would be nice if it can be.


================
Comment at: llvm/test/CodeGen/RISCV/xray-attribute-instrumentation.ll:6
+
+define i32 @foo() nounwind noinline uwtable "function-instrument"="xray-always" {
+; CHECK:		.p2align 2
----------------
Do you really need all of those attributes? I highly doubt noinline is needed, sane for uwtable. I don't think you'll get CFI either without nounwind, but could be wrong, never quite know when it's needed to squash CFI.


================
Comment at: llvm/test/CodeGen/RISCV/xray-attribute-instrumentation.ll:66
+}
+; CHECK:		.section xray_instr_map,{{.*}}
+; CHECK-LABEL:		.Lxray_sleds_start0:
----------------
Not sure what this `{{.*}}` achieves


================
Comment at: llvm/test/CodeGen/RISCV/xray-attribute-instrumentation.ll:68
+; CHECK-LABEL:		.Lxray_sleds_start0:
+; CHECK:		.Lxray_sled_0-.Ltmp{{.*}}
+; CHECK:		.Lxray_sled_1-.Ltmp{{.*}}
----------------
Given you've already explicitly stated the label names in the CHECK lines for the function body it would seem prudent to verify the right ones are used here


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117929