[PATCH] D148385: [RISCV] Implement KCFI operand bundle lowering for RV64

[Sami Tolvanen via Phabricator via cfe-commits]

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