This is an archive of the discontinued LLVM Phabricator instance.

[mips] Interrupt attribute support for mips32r2+.
ClosedPublic

Authored by sdardis on Jun 26 2015, 9:29 AM.

Details

Summary

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

Diff Detail

Event Timeline

sdardis updated this revision to Diff 28566.Jun 26 2015, 9:29 AM
sdardis retitled this revision from to [mips] Interrupt attribute upport for mips32..
sdardis updated this object.
sdardis edited the test plan for this revision. (Show Details)
sdardis added reviewers: mpf, dsanders.
sdardis retitled this revision from [mips] Interrupt attribute upport for mips32. to [mips] Interrupt attribute support for mips32..
sdardis added a subscriber: Unknown Object (MLST).
sdardis updated this revision to Diff 28664.Jun 29 2015, 5:56 AM

Fix for MipsInstrInfo.td which wasn't tested properly.

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).

sdardis updated this revision to Diff 38100.Oct 22 2015, 1:57 AM
sdardis retitled this revision from [mips] Interrupt attribute support for mips32. to [mips] Interrupt attribute support for mips32r2+..
sdardis updated this object.
sdardis marked 18 inline comments as done.

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.

vkalintiris accepted this revision.Oct 25 2015, 3:16 PM
vkalintiris added a reviewer: vkalintiris.

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.

This revision is now accepted and ready to land.Oct 25 2015, 3:16 PM

I don't have commit access. Thanks.

This revision was automatically updated to reflect the committed changes.