This patch adds support for using the "interrupt" attribute on Mips
for interrupt handling functions. At this time only mips32r2+ with the
o32 ABI with the static relocation model is supported. Unsupported
configurations will be rejected
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
I added some comments and few minor nits.
Also, I suppose that functions with the "interrupt" attribute should not take any arguments. If this is the case then we should report an error.
lib/Target/Mips/MipsAsmPrinter.cpp | ||
---|---|---|
200–203 ↗ | (On Diff #28664) | We don't need this special case if we derive the ERet def from the PseudoInstExpansion class too. |
lib/Target/Mips/MipsAsmPrinter.h | ||
50 ↗ | (On Diff #28664) | Nit: Fix indentation. |
lib/Target/Mips/MipsMachineFunction.h | ||
148 ↗ | (On Diff #28664) | Nit: Rename ISR to IsISR. |
lib/Target/Mips/MipsRegisterInfo.cpp | ||
90–98 ↗ | (On Diff #28664) | Nit: Can we replace nested if/else with ternary operator? |
lib/Target/Mips/MipsSEFrameLowering.cpp | ||
528–529 ↗ | (On Diff #28664) | Nit: Fix indentation (You'll have to consult clang-format for that). |
532–535 ↗ | (On Diff #28664) | If you don't use the sub-classed MipsSEInstrInfo, then the casts are unnecessary (likewise for emitInterruptEpilogueStub). |
546–550 ↗ | (On Diff #28664) | We are in the StandardEncoding (SE) sub-class. STI.inMips16Mode() will always return false. Also, in the commit message you mention that we support the interrupt attribute for MIPS32. However, here you test for MIPS32R2. |
606–627 ↗ | (On Diff #28664) | We can use StringSwitch here and just assert on InsSize's value. |
738–739 ↗ | (On Diff #28664) | Nit: Missing new line. |
768–785 ↗ | (On Diff #28664) | We can avoid the duplicate BuildMIs by figuring out the opcode and the register first. We can use MipsABIInfo's API here too. |
lib/Target/Mips/MipsSEInstrInfo.cpp | ||
192–204 ↗ | (On Diff #28664) | It would be nice if we could find a test that hits this code path. Also, I believe that the "interrupt" attribute check and the BuildMIs, should happen below (lines 233-240), in order to keep the function's "form" consistent. |
258 ↗ | (On Diff #28664) | Nit: reqIndirectLoad -> ReqIndirectLoad |
258–260 ↗ | (On Diff #28664) | Nit: clang-format places the logical operators && and || at the end of the line. |
303–314 ↗ | (On Diff #28664) | We can simplify this by using MipsABIInfo::ArePtrs64bit(). |
504 ↗ | (On Diff #28664) | Nit: Fix indentation. |
lib/Target/Mips/MipsSERegisterInfo.cpp | ||
129 ↗ | (On Diff #28664) | Nit: Rename ISRRegFI -> IsISRRegFI. |
test/CodeGen/Mips/interrupt-attr-fail.ll | ||
1 ↗ | (On Diff #28664) | The default prefix is CHECK. You don't have to specify it explicitly (likewise for interrupt-attr.ll). |
test/CodeGen/Mips/interrupt-attr.ll | ||
3–4 ↗ | (On Diff #28664) | Use CHECK-LABEL at the entry point of each function. |
5–11 ↗ | (On Diff #28664) | We should match the offsets of the store instructions in the prologue with the offsets of the load instructions in the epilogue (at least for the interrupt-specific registers). |
Comments addressed.
Now errors out for mips16 and when functions with the interrupt attribute have arguments. Additional XFAIL test for mips64r6.
The codepath for saving/restoring hi/lo is tested with interrupt-attr.ll, specifically the isr_sw0 function.
Thanks.
This LGTM. However, we need to clang-format the patch. Do you have commit access? If not then I can format it and commit it by myself.