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

[Simon Dardis via llvm-commits]

sdardis added inline comments.

================
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;
     }
----------------
dsanders wrote:
> 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.
For #1, I'm not quite following you here. We have to place the nop after the compact branch as part the same basic block. For example:

   BB1:
      <no instructions to go in a delay slot>
      bne v0,L3
   BB2:
      jal g
      * move a0, v0
   BB3:
   ....

The bne here will be transformed into a compact form. Since the next instruction is unsafe in the forbidden slot, we have to put a nop in the slot. We can't put the nop in BB3 as the first instruction as that doesn't clear the hazard. The hazard is due to instruction layout, not execution.

For #2, it's somewhat trivial to block delay slot filling for branches which have corresponding delay slot forms.


================
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;
----------------
dsanders wrote:
> We shouldn't have the '|| inMicroMipsMode()' here. microMIPSR6 has forbidden slots too.
microMIPSR6 doesn't have forbidden slots. The MD00582-2B-microMIPS32-AFP-06.03 says that "Any instruction, including a branch or jump, may immediately follow a branch or jump, that is, delay slot restrictions do not apply in Release 6". Confusingly MIPSR6 ISA has them.

================
Comment at: lib/Target/Mips/MipsSEInstrInfo.cpp:451
@@ +450,3 @@
+      if (canUseMicroMipsBranches)
+        return Mips::BEQZC_MM;
+      else
----------------
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.

================
Comment at: lib/Target/Mips/MipsSEInstrInfo.cpp:456
@@ +455,3 @@
+      if (canUseMicroMipsBranches)
+        return Mips::BNEZC_MM;
+      else
----------------
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.


http://reviews.llvm.org/D16353