[PATCH] D16353: [mips] MIPS32R6 compact branch support
Daniel Sanders via llvm-commits
llvm-commits at lists.llvm.org
Tue Feb 16 05:39:36 PST 2016
dsanders requested changes to this revision.
dsanders added a reviewer: dsanders.
dsanders added a comment.
This revision now requires changes to proceed.
Most of these are nits but there a a couple important ones.
Regarding testing this pass. Ideally we would be using MIR but I'm not going to require that since I haven't had chance to try it myself.
Could you put the new tests in a subdirectory so that we keep forbidden slot tests together and can migrate them to MIR tests later?
================
Comment at: lib/Target/Mips/MipsDelaySlotFiller.cpp:512
@@ +511,3 @@
+ const MipsSubtarget &STI = MBB.getParent()->getSubtarget<MipsSubtarget>();
+ const MipsSEInstrInfo *TII =
+ static_cast<const MipsSEInstrInfo *>(STI.getInstrInfo());
----------------
Mips16 uses the delay slot filler too as far as I know. If so, this could be a Mips16InstrInfo
================
Comment at: lib/Target/Mips/MipsDelaySlotFiller.cpp:559-623
@@ -566,69 +558,67 @@
/// runOnMachineBasicBlock - Fill in delay slots for the given basic block.
/// We assume there is only one delay slot per delayed instruction.
bool Filler::runOnMachineBasicBlock(MachineBasicBlock &MBB) {
bool Changed = false;
const MipsSubtarget &STI = MBB.getParent()->getSubtarget<MipsSubtarget>();
bool InMicroMipsMode = STI.inMicroMipsMode();
- const MipsInstrInfo *TII = STI.getInstrInfo();
+ const MipsSEInstrInfo *TII =
+ static_cast<const MipsSEInstrInfo *>(STI.getInstrInfo());
for (Iter I = MBB.begin(); I != MBB.end(); ++I) {
if (!hasUnoccupiedSlot(&*I))
continue;
++FilledSlots;
Changed = true;
// Delay slot filling is disabled at -O0.
if (!DisableDelaySlotFiller && (TM.getOptLevel() != CodeGenOpt::None)) {
bool Filled = false;
if (searchBackward(MBB, I)) {
Filled = true;
} else if (I->isTerminator()) {
if (searchSuccBBs(MBB, I)) {
Filled = true;
}
} else if (searchForward(MBB, I)) {
Filled = true;
}
if (Filled) {
// Get instruction with delay slot.
MachineBasicBlock::instr_iterator DSI(I);
if (InMicroMipsMode && TII->GetInstSizeInBytes(&*std::next(DSI)) == 2 &&
DSI->isCall()) {
// If instruction in delay slot is 16b change opcode to
// corresponding instruction with short delay slot.
DSI->setDesc(TII->get(getEquivalentCallShort(DSI->getOpcode())));
}
continue;
}
}
// If instruction is BEQ or BNE with one ZERO register, then instead of
// adding NOP replace this instruction with the corresponding compact
// branch instruction, i.e. BEQZC or BNEZC.
- unsigned Opcode = I->getOpcode();
if (InMicroMipsMode) {
- switch (Opcode) {
- case Mips::BEQ:
- case Mips::BNE:
- if (((unsigned) I->getOperand(1).getReg()) == Mips::ZERO) {
- I = replaceWithCompactBranch(MBB, I, I->getDebugLoc());
- continue;
- }
- break;
- case Mips::JR:
- case Mips::PseudoReturn:
- case Mips::PseudoIndirectBranch:
- // For microMIPS the PseudoReturn and PseudoIndirectBranch are allways
- // expanded to JR_MM, so they can be replaced with JRC16_MM.
- I = replaceWithCompactJump(MBB, I, I->getDebugLoc());
- continue;
- default:
- break;
+ if (TII->getEquivalentCompactForm(I)) {
+ I = replaceWithCompactBranch(MBB, I, I->getDebugLoc());
+ continue;
}
+ if (I->isIndirectBranch() || I->isReturn())
+ // For microMIPS the PseudoReturn and PseudoIndirectBranch are always
+ // expanded to JR_MM, so they can be replaced with JRC16_MM.
+ I = replaceWithCompactJump(MBB, I, I->getDebugLoc());
+ continue;
+ }
+
+ // For MIPSR6 attempt to produce the corresponding compact (no delay slot)
+ // form of the branch. This should save putting in a NOP.
+ if ((STI.hasMips32r6()) && TII->getEquivalentCompactForm(I)) {
+ I = replaceWithCompactBranch(MBB, I, I->getDebugLoc());
+ continue;
}
----------------
The separation makes sense to me given that we're only using compact branches when delay slots go unfilled.
There are a couple cases I can think of where this may not be the right thing to do:
# If we need a nop for both the delay slot and the forbidden slot then we need to decide which path should have the bubble. Ideally, it should be on the coldest path.
# Some machines may generally prefer compact branches.
For #1, we should allow the hazard scheduler to revert the branch to a delay slot branch if the BB probabilities suggest that the taken path is colder than the not-taken path.
For #2, I think it's reasonable to expect that such machine will exist in the future but I'm not aware of any yet. It shouldn't be difficult to deal with that if/when it arises.
================
Comment at: lib/Target/Mips/MipsDelaySlotFiller.cpp:565-566
@@ -572,3 +564,4 @@
bool InMicroMipsMode = STI.inMicroMipsMode();
- const MipsInstrInfo *TII = STI.getInstrInfo();
+ const MipsSEInstrInfo *TII =
+ static_cast<const MipsSEInstrInfo *>(STI.getInstrInfo());
----------------
This could be a Mips16InstrInfo
================
Comment at: lib/Target/Mips/MipsDelaySlotFiller.cpp:612-615
@@ -633,1 +611,6 @@
}
+ if (I->isIndirectBranch() || I->isReturn())
+ // For microMIPS the PseudoReturn and PseudoIndirectBranch are always
+ // expanded to JR_MM, so they can be replaced with JRC16_MM.
+ I = replaceWithCompactJump(MBB, I, I->getDebugLoc());
+ continue;
----------------
Could you put braces around this?
================
Comment at: lib/Target/Mips/MipsHazardSchedule.cpp:11-43
@@ +10,35 @@
+//
+// This pass is used to workaround certain pipeline hazards. For now, this covers
+// compact branch hazards. In future this pass can be extended to other pipeline
+// hazards, such as various MIPS1 hazards, processor errata that require
+// instruction reorganization, etc.
+//
+// This pass has to run after the delay slot filler as that pass can introduce
+// pipeline hazards, hence the existing hazard recognizer is not suitable.
+//
+// Hazards handled: forbidden slots for MIPSR6.
+//
+// A forbidden slot hazard occurs when a compact branch instruction is executed
+// and the adjacent instruction in memory is a control transfer instruction such
+// as a branch or jump, ERET, ERETNC, DERET, WAIT and PAUSE.
+//
+// For example:
+//
+// 0x8004 bnec a1,v0,<P+0x18>
+// 0x8008 beqc a1,a2,<P+0x54>
+//
+// In such cases, the processor is required to signal a Reserved Instruction
+// exception.
+//
+// Here, if the instruction at 0x8004 is executed, the processor will raise an
+// exception as there is a control transfer instruction at 0x8008.
+//
+// There are two sources of forbidden slot hazards:
+//
+// A) A previous pass has created a compact branch directly.
+// B) Transforming a delay slot branch into compact branch. This case can be
+// difficult to process as lookahead for hazards is insufficent, as
+// backwards delay slot fillling can also produce hazards in previously
+// processed instuctions.
+//
+
----------------
Could you change this into doxygen documentation? (see '\file')
================
Comment at: lib/Target/Mips/MipsHazardSchedule.cpp:86
@@ +85,3 @@
+
+// createMipsMipsHazardSchedule - Returns a pass that clears pipeline hazards.
+FunctionPass *llvm::createMipsHazardSchedule(MipsTargetMachine &tm) {
----------------
Don't repeat the function name in new code. Also, please use doxygen comments ('///').
================
Comment at: lib/Target/Mips/MipsHazardSchedule.cpp:97
@@ +96,3 @@
+ // Forbidden slot hazards are only defined for MIPSR6.
+ if (!STI->hasMips32r6() || STI->inMicroMipsMode())
+ return false;
----------------
We shouldn't have the '|| inMicroMipsMode()' here. microMIPSR6 has forbidden slots too.
================
Comment at: lib/Target/Mips/MipsHazardSchedule.cpp:104
@@ +103,3 @@
+ for (MachineFunction::iterator FI = MF.begin(); FI != MF.end(); ++FI) {
+ for (Iter I = (*FI).begin(); I != (*FI).end(); ++I) {
+
----------------
Use the arrow operator instead of '(*FI).'. There's a few other cases of this below
================
Comment at: lib/Target/Mips/MipsHazardSchedule.cpp:122
@@ +121,3 @@
+ for (auto *Succ : (*FI).successors()) {
+ if (FI->isLayoutSuccessor(Succ) &&
+ Succ->getFirstNonDebugInstr() != Succ->end() &&
----------------
We can only have one fallthrough. We should stop iterating once we find it.
================
Comment at: lib/Target/Mips/MipsHazardSchedule.cpp:125-128
@@ +124,6 @@
+ !TII->SafeInForbiddenSlot(Succ->getFirstNonDebugInstr())) {
+ BuildMI(&(*FI), I->getDebugLoc(), TII->get(Mips::NOP));
+ Changed = true;
+ MIBundleBuilder(*FI, I, std::next(I, 2));
+ NumInsertedNops++;
+ }
----------------
Please factor out the duplicate code.
================
Comment at: lib/Target/Mips/MipsInstrInfo.cpp:260-302
@@ -259,2 +259,45 @@
+/// Predicate for distingushing between control transfer instructions and all
+/// other instructions for handling forbidden slots. Consider inline assembly
+/// as unsafe as well.
+bool MipsInstrInfo::SafeInForbiddenSlot(const MachineInstr *MI) const {
+ if (MI->isCall() || MI->isBranch() || MI->isReturn() || MI->isInlineAsm())
+ return false;
+
+ switch (MI->getOpcode()) {
+ case Mips::ERET:
+ case Mips::ERETNC:
+ case Mips::DERET:
+ case Mips::PAUSE:
+ case Mips::WAIT:
+ return false;
+ default:
+ return true;
+ }
+}
+
+/// Predicate for distingushing instructions that have forbidden slots.
+bool MipsInstrInfo::HasForbiddenSlot(const MachineInstr *MI) const {
+ if (!MI->isBranch())
+ return false;
+
+ switch (MI->getOpcode()) {
+ case Mips::BEQC:
+ case Mips::BNEC:
+ case Mips::BLTC:
+ case Mips::BGEC:
+ case Mips::BLTUC:
+ case Mips::BGEUC:
+ case Mips::BEQZC:
+ case Mips::BNEZC:
+ case Mips::BGEZC:
+ case Mips::BGTZC:
+ case Mips::BLEZC:
+ case Mips::BLTZC:
+ return true;
+ default:
+ return false;
+ }
+}
+
/// Return the number of bytes of code the specified instruction may be.
----------------
We ought to tablegen-erate these.
================
Comment at: lib/Target/Mips/MipsInstrInfo.cpp:324
@@ -280,1 +323,3 @@
MachineInstrBuilder MIB;
+ bool ZeroOperandBranch = false;
+
----------------
I read this as being a branch with no operands. Can you make it clearer?
================
Comment at: lib/Target/Mips/MipsInstrInfo.cpp:330
@@ +329,3 @@
+ if (I->isBranch() && I->getOperand(1).isReg() &&
+ // FIXME: Certain atomic sequences on mips64 generate 32bit references to
+ // Mips::ZERO, which is incorrect. This test should be updated to use
----------------
We'll should test for both ZERO and ZERO_64 so that this is still correct when the atomics are fixed.
================
Comment at: lib/Target/Mips/MipsInstrInfo.h:74
@@ -73,1 +73,3 @@
+ /// Forbidden slot analysis
+ bool SafeInForbiddenSlot(const MachineInstr *MI) const;
----------------
dsanders wrote:
> Please make the arguments references since nullptr is not permitted
Please make this doxygen comment document the function
================
Comment at: lib/Target/Mips/MipsInstrInfo.h:74-78
@@ -73,2 +73,7 @@
+ /// Forbidden slot analysis
+ bool SafeInForbiddenSlot(const MachineInstr *MI) const;
+
+ bool HasForbiddenSlot(const MachineInstr *MI) const;
+
/// Insert nop instruction when hazard condition is found
----------------
Please make the arguments references since nullptr is not permitted
================
Comment at: lib/Target/Mips/MipsSEInstrInfo.cpp:451
@@ +450,3 @@
+ if (canUseMicroMipsBranches)
+ return Mips::BEQZC_MM;
+ else
----------------
This doesn't look equivalent to BEQ to me. What happens if the second operand of the comparison isn't $zero?
================
Comment at: lib/Target/Mips/MipsSEInstrInfo.cpp:456
@@ +455,3 @@
+ if (canUseMicroMipsBranches)
+ return Mips::BNEZC_MM;
+ else
----------------
This doesn't look equivalent to BEQ to me. What happens if the second operand of the comparison isn't $zero?
================
Comment at: lib/Target/Mips/MipsSEInstrInfo.h:69
@@ -68,1 +68,3 @@
+ unsigned getEquivalentCompactForm(MachineBasicBlock::iterator I) const;
+
----------------
The iterator should be const too.
http://reviews.llvm.org/D16353
More information about the llvm-commits
mailing list