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

[Ana Pazos via Phabricator via llvm-commits]

apazos added inline comments.


================
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,
----------------
asb wrote:
> 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.
I initially thought about only saving the Caller saved registers that are either arguments (X10-X17) or temporaries (X5-X7 and X28-X31).

But this list is being used not only to force the registers that need to be saved unconditionally (when the interrupt handler calls another function)  but also in the call to getCalleeSavedRegs. So it has to have all Callee and Caller saved registers. I will update the list and add the additional registers X3, X4, X8, X9.


================
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;
----------------
asb wrote:
> 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.
In that case I think we should at least warn that interrupt attribute in the presence of F and D extensions will not preserve the registers correctly. What do you think?


================
Comment at: test/CodeGen/RISCV/interrupt-attr.ll:20-23
+; CHECK-LABEL: foo_user:
+; CHECK:       # %bb.0:
+; CHECK-NEXT:    uret
+  ret void
----------------
asb wrote:
> 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.
Sure, we can create the test for riscv32 for now, but riscv64 global address lowering will fail. I will add the riscv32 test case and leave a TODO for the riscv64 test case.


================
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
----------------
asb wrote:
> 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.
Sure, will add a function test case with the attribute being set.


https://reviews.llvm.org/D48411