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. | |
738–739 | Nit: Missing new line. | |
768–785 | 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 | ||
1 | 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 | Use CHECK-LABEL at the entry point of each function. | |
5–11 | 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.