samitolvanen marked 9 inline comments as done. samitolvanen added a comment.
In D148385#4275617 <https://reviews.llvm.org/D148385#4275617>, @jrtc27 wrote: > I can't help but feel the core of this should be target-independent, with TLI > or similar hooks to actually do the target-specific bits? I agree, that might be a nice improvement. The AArch64 version at least is nearly identical to this one, although there's a bit more going on in the X86 version. I'll take a look. ================ Comment at: llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp:277 +void RISCVAsmPrinter::LowerKCFI_CHECK(const MachineInstr &MI) { + assert(STI->is64Bit() && "KCFI_CHECK requires RV64"); + ---------------- jrtc27 wrote: > What about this code relies on it being 64-bit? Good point, this doesn't require much tweaking to also work on RV32. We just need to avoid `ADDIW`. ================ Comment at: llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp:299-300 + ++NextReg; + Reg = NextReg++; + if (Reg > RISCV::X31) + report_fatal_error("Unable to find scratch registers for KCFI_CHECK"); ---------------- nickdesaulniers wrote: > if (++NextReg > RISCV::X31) That's not quite the same. We want to check the current register, not the next one, which we might not even need. I can certainly not post-increment the `NextReg` value if it's confusing. ================ Comment at: llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp:310 + EmitToStreamer(*OutStreamer, MCInstBuilder(RISCV::ADDI) + .addReg(ScratchRegs[0]) + .addReg(RISCV::X0) ---------------- craig.topper wrote: > craig.topper wrote: > > Are there two many operands here? > Err. "too many" Yes there are, thanks for noticing. ================ Comment at: llvm/lib/Target/RISCV/RISCVKCFI.cpp:106-107 + for (MachineBasicBlock &MBB : MF) { + for (MachineBasicBlock::instr_iterator MII = MBB.instr_begin(), + MIE = MBB.instr_end(); + MII != MIE; ++MII) { ---------------- nickdesaulniers wrote: > for (MachineInstr &MI : MBB) We don't want to skip bundles here, so we have to use `instr_iterator` explicitly. ================ Comment at: llvm/test/CodeGen/RISCV/kcfi.ll:1 +; RUN: llc -mtriple=riscv64 -verify-machineinstrs -riscv-no-aliases < %s | FileCheck %s --check-prefix=ASM +; RUN: llc -mtriple=riscv64 -verify-machineinstrs -stop-after=finalize-isel < %s | FileCheck %s --check-prefixes=MIR,ISEL ---------------- jrtc27 wrote: > Please try not to introduce tests with hand-written CHECK lines, but if you > have to, try and make them look similar in style to the auto-generated lines. `update_llc_test_checks.py` doesn't play well with testing function prefix data, but I updated the style to match its output, added RV32 (without duplicating identical code), and moved the MIR tests to separate files with auto-generated checks. Please let me know if there's something specific I'm missing here. ================ Comment at: llvm/utils/gn/secondary/llvm/lib/Target/RISCV/BUILD.gn:81 "RISCVInstrInfo.cpp", + "RISCVKCFI.cpp", "RISCVMCInstLower.cpp", ---------------- craig.topper wrote: > Isn't this done automatically by the GN Syncbot when CMakeLists.txt is > updated? I remember updating this manually before, but if that's the case, I'm happy to drop this line. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D148385/new/ https://reviews.llvm.org/D148385 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
