This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Add support for _interrupt attribute
ClosedPublic

Authored by apazos on Jun 20 2018, 6:46 PM.

Details

Summary
  • Save/restore only registers that are used. This includes Callee saved registers and Caller saved registers (arguments and temporaries) for integer and FP registers.
  • If there is a call in the interrupt handler, save/restore all Caller saved registers and all FP registers.
  • Emit special return instructions depending on "interrupt" attribute type.

Based on initial patch by Zhaoshi Zheng.

Diff Detail

Repository
rL LLVM

Event Timeline

apazos created this revision.Jun 20 2018, 6:46 PM
apazos updated this revision to Diff 152565.Jun 22 2018, 5:38 PM
apazos retitled this revision from [RISCV] Support for __attribute__((interrupt)) to [RISCV] Add support for _interrupt attribute.
apazos edited the summary of this revision. (Show Details)

Initial code was saving all registers.
Modified it to save only the ones used, or all registers if there is a call.
Still needs some fixes and tests update.
Sharing the WIP to collect feedback.

apazos updated this revision to Diff 153595.Jun 29 2018, 3:44 PM

Updated tests and registers saved with extensions F and D

asb added a comment.Jul 2 2018, 7:42 AM

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

lib/Target/RISCV/RISCVCallingConv.td
22–23 ↗(On Diff #153595)

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.

lib/Target/RISCV/RISCVRegisterInfo.cpp
37–40 ↗(On Diff #153595)

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.

test/CodeGen/RISCV/interrupt-attr.ll
20–23 ↗(On Diff #153595)

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.

42–44 ↗(On Diff #153595)

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.

apazos added inline comments.Jul 10 2018, 2:50 PM
lib/Target/RISCV/RISCVCallingConv.td
22–23 ↗(On Diff #153595)

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.

lib/Target/RISCV/RISCVRegisterInfo.cpp
37–40 ↗(On Diff #153595)

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?

test/CodeGen/RISCV/interrupt-attr.ll
20–23 ↗(On Diff #153595)

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.

42–44 ↗(On Diff #153595)

Sure, will add a function test case with the attribute being set.

apazos updated this revision to Diff 154889.Jul 10 2018, 2:54 PM
apazos edited the summary of this revision. (Show Details)

Updated patch to address review comments.

apazos added inline comments.Jul 10 2018, 3:01 PM
lib/Target/RISCV/RISCVCallingConv.td
31 ↗(On Diff #154889)

Note that CSR_Interrupt_With_Call is only used to provide a list of registers that need to be unconditionally saved. It is not used by
getCalleeSavedRegs nor getCallPreservedMask. Maybe the reuse of the class CalleeSavedRegs is confusing? I wanted to avoid listing each register in RISCVFrameLowering::determineCalleeSave.

asb added inline comments.Jul 12 2018, 7:44 AM
lib/Target/RISCV/RISCVRegisterInfo.cpp
37–40 ↗(On Diff #153595)

Thanks for questioning this because this was a 'thinko' on my part and I was incorrect. If F/D are enabled for codegen then we _should_ save and restore F+D, as the function that was interrupted might have been using them, as could a function that we call from the interrupt handler.

apazos updated this revision to Diff 155437.Jul 13 2018, 11:11 AM
apazos edited the summary of this revision. (Show Details)

Added back code to save FP registers.

asb added a comment.Jul 19 2018, 6:31 AM

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.

lib/Target/RISCV/RISCVCallingConv.td
32 ↗(On Diff #155437)

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.

lib/Target/RISCV/RISCVFrameLowering.cpp
217 ↗(On Diff #155437)

callee-saved

218 ↗(On Diff #155437)

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.

apazos updated this revision to Diff 156359.Jul 19 2018, 2:36 PM
apazos added inline comments.
lib/Target/RISCV/RISCVCallingConv.td
32 ↗(On Diff #155437)

Sure, will change the name to XLEN.

lib/Target/RISCV/RISCVFrameLowering.cpp
217 ↗(On Diff #155437)

This code is forcing the saving of all arguments and temporaries plus all the floating point registers, even if they are not used.
Arguments and temporaries are ra, t0-2, a0-a7, t3-t6 (which are all Caller-saved registers).
So the comment is correct (includes Caller and floating point registers only).

Here is the logic I am enforcing:

  • If interrupt is enabled:
    • If no calls Save all registers if they are used. That includes callee saved, caller saved, and FP registers. When you call getCalleeSavedRegs and getCallPreservedMask, it returns the data for the lists CSR_XLEN_F32 and CSR_XLEN_F64. In my last patch I missed getCallPreservedMask. I will update that function. ----If has calls Force saving all arguments and temporaries (these are the Caller saved registers), even if not used. Force saving all FP registers, even if not used. RISCVFrameLowering::determineCalleeSaves is the place to force saving these registers.

I checked GCC and it seems to implement this behavior from what I could test.

Let me know if you agree.

asb added inline comments.Jul 24 2018, 7:47 AM
lib/Target/RISCV/RISCVFrameLowering.cpp
217 ↗(On Diff #155437)

I agree that's the desired logic and your original comment is correct.

asb accepted this revision.Jul 26 2018, 6:22 AM

Looks good to me, thanks!

This revision is now accepted and ready to land.Jul 26 2018, 6:22 AM
This revision was automatically updated to reflect the committed changes.