[PATCH] D10768: [mips] Interrupt attribute support for mips32.
Vasileios Kalintiris
Vasileios.Kalintiris at imgtec.com
Sun Jul 12 01:34:04 PDT 2015
vkalintiris added a subscriber: vkalintiris.
vkalintiris added a comment.
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.
================
Comment at: lib/Target/Mips/MipsAsmPrinter.cpp:200-203
@@ -192,1 +199,6 @@
+ if (I->getOpcode() == Mips::ERet) {
+ emitERet(*OutStreamer, &*I);
+ continue;
+ }
+
----------------
We don't need this special case if we derive the ERet def from the PseudoInstExpansion class too.
================
Comment at: lib/Target/Mips/MipsAsmPrinter.h:50
@@ +49,3 @@
+ void emitERet(MCStreamer &OutStreamer,
+ const MachineInstr *MI);
+
----------------
Nit: Fix indentation.
================
Comment at: lib/Target/Mips/MipsMachineFunction.h:148
@@ +147,3 @@
+ /// ISR - Whether the function is an Interrupt Service Routine.
+ bool ISR;
+
----------------
Nit: Rename ISR to IsISR.
================
Comment at: lib/Target/Mips/MipsRegisterInfo.cpp:90-98
@@ +89,11 @@
+ if (Subtarget.hasMips64()){
+ if (Subtarget.hasMips64r6())
+ return CSR_Interrupt_64R6_SaveList;
+ else
+ return CSR_Interrupt_64_SaveList;
+ } else {
+ if (Subtarget.hasMips32r6())
+ return CSR_Interrupt_32R6_SaveList;
+ else
+ return CSR_Interrupt_32_SaveList;
+ }
----------------
Nit: Can we replace nested if/else with ternary operator?
================
Comment at: lib/Target/Mips/MipsSEFrameLowering.cpp:528-529
@@ -524,1 +527,4 @@
+void MipsSEFrameLowering::emitInterruptPrologueStub(MachineFunction &MF,
+ MachineBasicBlock &MBB) const {
+ MipsFunctionInfo *MipsFI = MF.getInfo<MipsFunctionInfo>();
----------------
Nit: Fix indentation (You'll have to consult clang-format for that).
================
Comment at: lib/Target/Mips/MipsSEFrameLowering.cpp:532-535
@@ +531,6 @@
+
+ const MipsSEInstrInfo &TII =
+ *static_cast<const MipsSEInstrInfo *>(STI.getInstrInfo());
+ const MipsRegisterInfo &RegInfo =
+ *static_cast<const MipsRegisterInfo *>(STI.getRegisterInfo());
+
----------------
If you don't use the sub-classed MipsSEInstrInfo, then the casts are unnecessary (likewise for emitInterruptEpilogueStub).
================
Comment at: lib/Target/Mips/MipsSEFrameLowering.cpp:546-550
@@ +545,7 @@
+
+ // Mips16 lacks the ehb instruction as well.
+ if (!STI.hasMips32r2() || STI.inMips16Mode())
+ report_fatal_error(
+ "\"interrupt\" attribute is not supported on pre-r2 MIPS or"
+ "Mips16 targets.");
+
----------------
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.
================
Comment at: lib/Target/Mips/MipsSEFrameLowering.cpp:606-627
@@ +605,24 @@
+ // masking off interrupts to the cause register.
+ if (IntKind == "eic") {
+ SrcReg = Mips::K0;
+ InsPosition = 10;
+ InsSize = 6;
+ } else if (IntKind == "sw0")
+ InsSize = 1;
+ else if (IntKind == "sw1")
+ InsSize = 2;
+ else if (IntKind == "hw0")
+ InsSize = 3;
+ else if (IntKind == "hw1")
+ InsSize = 4;
+ else if (IntKind == "hw2")
+ InsSize = 5;
+ else if (IntKind == "hw3")
+ InsSize = 6;
+ else if (IntKind == "hw4")
+ InsSize = 7;
+ else if (IntKind == "hw5")
+ InsSize = 8;
+ else
+ llvm_unreachable("Unknown Mips interrupt type!");
+
----------------
We can use StringSwitch here and just assert on InsSize's value.
================
Comment at: lib/Target/Mips/MipsSEFrameLowering.cpp:738-739
@@ -581,2 +737,4 @@
+
+}
bool MipsSEFrameLowering::
spillCalleeSavedRegisters(MachineBasicBlock &MBB,
----------------
Nit: Missing new line.
================
Comment at: lib/Target/Mips/MipsSEFrameLowering.cpp:768-785
@@ +767,20 @@
+
+ if (Reg == Mips::HI0 || Reg == Mips::LO0){
+ if (Reg == Mips::HI0)
+ BuildMI(MBB, MI, dl, TII.get(Mips::MFLO), Mips::K0)
+ .setMIFlag(MachineInstr::FrameSetup);
+ else
+ BuildMI(MBB, MI, dl, TII.get(Mips::MFHI), Mips::K0)
+ .setMIFlag(MachineInstr::FrameSetup);
+
+ Reg = Mips::K0;
+ } else {
+ if (Reg == Mips::HI0_64)
+ BuildMI(MBB, MI, dl, TII.get(Mips::MFLO64), Mips::K0_64)
+ .setMIFlag(MachineInstr::FrameSetup);
+ else
+ BuildMI(MBB, MI, dl, TII.get(Mips::MFHI64), Mips::K0_64)
+ .setMIFlag(MachineInstr::FrameSetup);
+
+ Reg = Mips::K0_64;
+ }
----------------
We can avoid the duplicate BuildMIs by figuring out the opcode and the register first. We can use MipsABIInfo's API here too.
================
Comment at: lib/Target/Mips/MipsSEInstrInfo.cpp:192-204
@@ +191,15 @@
+ if (Func->hasFnAttribute("interrupt")){
+ if (Mips::HI32RegClass.hasSubClassEq(RC)){
+ BuildMI(MBB, I, DL, get(Mips::MFHI), Mips::K0);
+ SrcReg = Mips::K0;
+ } else if (Mips::HI64RegClass.hasSubClassEq(RC)){
+ BuildMI(MBB, I, DL, get(Mips::MFHI64), Mips::K0_64);
+ SrcReg = Mips::K0_64;
+ } else if (Mips::LO32RegClass.hasSubClassEq(RC)){
+ BuildMI(MBB, I, DL, get(Mips::MFLO), Mips::K0);
+ SrcReg = Mips::K0;
+ } else if (Mips::LO64RegClass.hasSubClassEq(RC)){
+ BuildMI(MBB, I, DL, get(Mips::MFLO64), Mips::K0_64);
+ SrcReg = Mips::K0_64;
+ }
+ }
----------------
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.
================
Comment at: lib/Target/Mips/MipsSEInstrInfo.cpp:258
@@ +257,3 @@
+ const Function * Func = MBB.getParent()->getFunction();
+ bool reqIndirectLoad = Func->hasFnAttribute("interrupt")
+ && (DestReg == Mips::LO0 || DestReg == Mips::LO0_64
----------------
vkalintiris wrote:
> Nit: clang-format places the logical operators && and || at the end of the line.
Nit: reqIndirectLoad -> ReqIndirectLoad
================
Comment at: lib/Target/Mips/MipsSEInstrInfo.cpp:258-260
@@ +257,5 @@
+ const Function * Func = MBB.getParent()->getFunction();
+ bool reqIndirectLoad = Func->hasFnAttribute("interrupt")
+ && (DestReg == Mips::LO0 || DestReg == Mips::LO0_64
+ || DestReg == Mips::HI0 || DestReg == Mips::HI0_64);
+
----------------
Nit: clang-format places the logical operators && and || at the end of the line.
================
Comment at: lib/Target/Mips/MipsSEInstrInfo.cpp:303-314
@@ +302,14 @@
+ else {
+
+ unsigned Reg = Mips::K0;
+ unsigned LdOp = Mips::MTLO;
+ if (DestReg == Mips::HI0) LdOp = Mips::MTHI;
+
+ if (DestReg == Mips::HI0_64 || DestReg == Mips::LO0_64){
+ Reg = Mips::K0_64;
+ if (DestReg == Mips::HI0_64)
+ LdOp = Mips::MTHI64;
+ else
+ LdOp = Mips::MTLO64;
+ }
+
----------------
We can simplify this by using MipsABIInfo::ArePtrs64bit().
================
Comment at: lib/Target/Mips/MipsSEInstrInfo.cpp:504
@@ +503,3 @@
+void MipsSEInstrInfo::expandERet(MachineBasicBlock &MBB,
+ MachineBasicBlock::iterator I) const {
+ BuildMI(MBB, I, I->getDebugLoc(), get(Mips::ERet));
----------------
Nit: Fix indentation.
================
Comment at: lib/Target/Mips/MipsSERegisterInfo.cpp:129
@@ -128,3 +128,3 @@
bool EhDataRegFI = MipsFI->isEhDataRegFI(FrameIndex);
-
+ bool ISRRegFI = MipsFI->isISRRegFI(FrameIndex);
// The following stack frame objects are always referenced relative to $sp:
----------------
Nit: Rename ISRRegFI -> IsISRRegFI.
================
Comment at: test/CodeGen/Mips/interrupt-attr-fail.ll:1
@@ +1,2 @@
+; RUN: llc -mcpu=mips32 -march=mipsel -relocation-model=static -o - %s | FileCheck --check-prefix=CHECK %s
+; XFAIL: *
----------------
The default prefix is CHECK. You don't have to specify it explicitly (likewise for interrupt-attr.ll).
================
Comment at: test/CodeGen/Mips/interrupt-attr.ll:3-4
@@ +2,4 @@
+
+define void @isr_sw0() #0 {
+; CHECK: mfc0 $27, $14, 0
+; CHECK: sw $27, {{[0-9]+}}($sp)
----------------
Use CHECK-LABEL at the entry point of each function.
================
Comment at: test/CodeGen/Mips/interrupt-attr.ll:5-11
@@ +4,9 @@
+; CHECK: mfc0 $27, $14, 0
+; CHECK: sw $27, {{[0-9]+}}($sp)
+; CHECK: mfc0 $27, $12, 0
+; CHECK: sw $27, {{[0-9]+}}($sp)
+; CHECK: ins $27, $zero, 8, 1
+; CHECK: ins $27, $zero, 1, 4
+; CHECK: ins $27, $zero, 29, 1
+; CHECK: mtc0 $27, $12, 0
+ ; Must save all registers
----------------
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).
http://reviews.llvm.org/D10768
More information about the llvm-commits
mailing list