[PATCH] D48411: [RISCV] Add support for _interrupt attribute

[Alex Bradbury via Phabricator via llvm-commits]

asb added a comment.

Thanks Ana. I've added some initial review comments. Sorry for the slight delay, I've been on vacation the past week.



================
Comment at: lib/Target/RISCV/RISCVCallingConv.td:22-23
+
+// Interrupt handler needs to save/restore caller-saved registers
+// as callee-saved registers.
+def CSR_Interrupt : CalleeSavedRegs<(add X1,
----------------
Shouldn't these sets include the normal callee-saved registers? (X1, X3, X4, X8, X9). As it stands, I think an interrupt handler with high register pressure would fail to save these registers before dirtying them.


================
Comment at: lib/Target/RISCV/RISCVRegisterInfo.cpp:37-40
+    if (MF->getSubtarget<RISCVSubtarget>().hasStdExtD())
+      return CSR_X32_F64_Interrupt_SaveList;
+    if (MF->getSubtarget<RISCVSubtarget>().hasStdExtF())
+      return CSR_X32_F32_Interrupt_SaveList;
----------------
F and D registers should be saved based on the target ABI rather than the presence of F/D. As support for the hard-float RISC-V ABIs hasn't landed in upstream LLVM yet, leaving save/restore of floating point registers out of this patch might be sensible.


================
Comment at: test/CodeGen/RISCV/interrupt-attr.ll:20-23
+; CHECK-LABEL: foo_user:
+; CHECK:       # %bb.0:
+; CHECK-NEXT:    uret
+  ret void
----------------
It would be good to extend these tests so they do something simple that will require registers to be allocated (load a value from a global, add 1 and store again?). Then it should demonstrate the registers are being saved and restored as expected.


================
Comment at: test/CodeGen/RISCV/interrupt-attr.ll:42-44
+; CHECK-RV32-LABEL: foo:
+; CHECK-RV32:       # %bb.0:
+; CHECK-RV32-NEXT:    addi sp, sp, -80
----------------
In the interests of completeness it would be good to have on test for an interrupt handler with "no-frame-pointer-elim"="true" as well, demonstrating that there are no problems saving the frame pointer.


https://reviews.llvm.org/D48411