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
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 | ||
128 | Nit: Rename ISR to IsISR. | |
lib/Target/Mips/MipsRegisterInfo.cpp | ||
90–98 | Nit: Can we replace nested if/else with ternary operator? | |
lib/Target/Mips/MipsSEFrameLowering.cpp | ||
537–538 | Nit: Fix indentation (You'll have to consult clang-format for that). | |
541–544 | If you don't use the sub-classed MipsSEInstrInfo, then the casts are unnecessary (likewise for emitInterruptEpilogueStub). | |
555–559 | 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. | |
615–636 | We can use StringSwitch here and just assert on InsSize's value. | |
735–736 | Nit: Missing new line. | |
764–781 | 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 | ||
191–203 | 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. | |
259 | Nit: reqIndirectLoad -> ReqIndirectLoad | |
259–261 | Nit: clang-format places the logical operators && and || at the end of the line. | |
304–315 | We can simplify this by using MipsABIInfo::ArePtrs64bit(). | |
507 | Nit: Fix indentation. | |
lib/Target/Mips/MipsSERegisterInfo.cpp | ||
129 | Nit: Rename ISRRegFI -> IsISRRegFI. | |
test/CodeGen/Mips/interrupt-attr-fail.ll | ||
2 | The default prefix is CHECK. You don't have to specify it explicitly (likewise for interrupt-attr.ll). | |
test/CodeGen/Mips/interrupt-attr.ll | ||
4–5 | Use CHECK-LABEL at the entry point of each function. | |
6–12 | 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.
Nit: Rename ISR to IsISR.