[PATCH] D16353: [mips] MIPS32R6 compact branch support

Daniel Sanders via llvm-commits llvm-commits at lists.llvm.org
Thu Mar 3 06:16:04 PST 2016


dsanders accepted this revision.
dsanders added a comment.

LGTM with a couple nits.


================
Comment at: lib/Target/Mips/MipsDelaySlotFiller.cpp:557-624
@@ -566,70 +556,70 @@
 
 /// 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();
 
   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;
+    }
+
     // Bundle the NOP to the instruction with the delay slot.
----------------
For #1, I'm trying to say that, in some circumstances, delay-slot branches can be a better choice than compact branches when both are available.

For example:
  BB1:
    ...
    bne $2, BB3
    nop
  BB2:
    jal g
  BB3:
    ...
introduces a bubble on both the taken and not-taken paths while:
  BB1:
    ...
    bnec $2, BB3
    nop
  BB2:
    jal g
  BB3:
    ...
introduces a bubble on the taken path and possibly two bubbles on the not-taken path depending on implementation.
I'm told this kind of thing occurs between delay-slot returns and compact-branch returns but we need to verify that.

My comment was intended to be something to think about rather than something to do in this patch. Forbidden slots is a correctness issue so it's important that we get a solution in place and we can expand on performance decisions in later patches.

> The hazard is due to instruction layout, not execution.

That's right, but we aren't forced to choose code that has this hazard.

================
Comment at: lib/Target/Mips/MipsHazardSchedule.cpp:98
@@ +97,3 @@
+  if (!STI->hasMips32r6() || STI->inMicroMipsMode())
+    return false;
+
----------------
Ok then. I'm pretty sure it had them at one point but it doesn't now

================
Comment at: lib/Target/Mips/MipsInstrInfo.cpp:259-260
@@ -258,1 +258,4 @@
 
+/// getEquivalentCompactForm - Return the corresponding compact (no delay slot)
+/// form of a branch.
+unsigned MipsInstrInfo::getEquivalentCompactForm(
----------------
Repeated name in comment.

================
Comment at: lib/Target/Mips/MipsInstrInfo.td:1118-1122
@@ -1107,2 +1117,7 @@
 
+// Wait
+class WAIT_FT<string opstr> :
+  InstSE<(outs), (ins), opstr, [], NoItinerary, FrmOther, opstr>;
+}
+
 // Interrupts
----------------
Why move this above DEI_FT?

================
Comment at: lib/Target/Mips/MipsInstrInfo.td:1321-1325
@@ -1311,4 +1320,7 @@
+let isReturn=1, isTerminator=1, isBarrier=1, hasCtrlDep=1, isCTI=1 in {
+let hasDelaySlot=1 in
 def RetRA : PseudoSE<(outs), (ins), [(MipsRet)]>;
 
-let isReturn=1, isTerminator=1, isBarrier=1, hasCtrlDep=1, hasSideEffects=1 in
+let hasSideEffects=1 in
 def ERet : PseudoSE<(outs), (ins), [(MipsERet)]>;
+}
----------------
Could you indent this since we have nested 'let' statements

================
Comment at: lib/Target/Mips/MipsSEInstrInfo.cpp:451
@@ +450,3 @@
+      if (canUseMicroMipsBranches)
+        return Mips::BEQZC_MM;
+      else
----------------
sdardis wrote:
> dsanders wrote:
> > This doesn't look equivalent to BEQ to me. What happens if the second operand of the comparison isn't $zero?
> If the second operand of the comparison is not $zero canUseMicroMipsBranches is false as that explicitly checks for conditions that beqz16 can be used in. However, if we have microMIPSR6 we can generate beqc (long version). if both canUseMicroMipsBranches and hasMips32r6() is false we return 0 since there's no equivalent form.
> 
> I've renamed canUseMicroMipsBranches to canUseShortMMBranches to better reflect what it's doing.
Ok, thanks.

================
Comment at: lib/Target/Mips/MipsSEInstrInfo.cpp:456
@@ +455,3 @@
+      if (canUseMicroMipsBranches)
+        return Mips::BNEZC_MM;
+      else
----------------
sdardis wrote:
> dsanders wrote:
> > This doesn't look equivalent to BEQ to me. What happens if the second operand of the comparison isn't $zero?
> See above.
Ok, thanks


http://reviews.llvm.org/D16353





More information about the llvm-commits mailing list