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 | We don't need this special case if we derive the ERet def from the PseudoInstExpansion class too. | |
| lib/Target/Mips/MipsAsmPrinter.h | ||
| 50 | Nit: Fix indentation. | |
| lib/Target/Mips/MipsMachineFunction.h | ||
| 148 | 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 | ||
| 528–529 | Nit: Fix indentation (You'll have to consult clang-format for that). | |
| 532–535 | If you don't use the sub-classed MipsSEInstrInfo, then the casts are unnecessary (likewise for emitInterruptEpilogueStub). | |
| 546–550 | 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 | We can use StringSwitch here and just assert on InsSize's value. | |
| 728–729 | Nit: Missing new line. | |
| 758–775 | 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 | 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 | Nit: reqIndirectLoad -> ReqIndirectLoad | |
| 258–260 | Nit: clang-format places the logical operators && and || at the end of the line. | |
| 303–314 | We can simplify this by using MipsABIInfo::ArePtrs64bit(). | |
| 504 | 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: Fix indentation.