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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
