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

[Alex Bradbury via Phabricator via llvm-commits]

asb added a comment.

This is looking good to me, I've added some comments. The logic around callee/caller save registers is a little confusing but the way you're handling it seems sensible. A comment clarifying things in determineCalleeSaves would be really useful I think.



================
Comment at: lib/Target/RISCV/RISCVCallingConv.td:32
+// Same as CSR_Interrupt, but including all 32-bit FP registers.
+def CSR_X32_F32_Interrupt: CalleeSavedRegs<(add X1,
+    (sequence "X%u", 3, 9),
----------------
Would be more accurately called `CSR_XLEN_F32` or `CSR_GPR_F32` as the X registers are a variable-length register class. i.e. we won't need to define `CSR_X64_F32`. Same for `CSR_X32_F64`.


================
Comment at: lib/Target/RISCV/RISCVFrameLowering.cpp:217
+  // If interrupt is enabled and there are calls in the handler,
+  // unconditionally save all Caller-saved registers and
+  // all FP registers, regardless whether they are used.
----------------
callee-saved


================
Comment at: lib/Target/RISCV/RISCVFrameLowering.cpp:218
+  // unconditionally save all Caller-saved registers and
+  // all FP registers, regardless whether they are used.
+  MachineFrameInfo &MFI = MF.getFrameInfo();
----------------
Just for clarity, the reason we can't use getCalleeSavedRegs is that we want to force-save only those registers that are callee-saved under the standard ABI, while the array returned by getCalleeSaved regs contains the full set of registers that are either callee or caller saved under the standard ABI? It would be useful to add a comment explaining this I think.


https://reviews.llvm.org/D48411