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

[Vasileios Kalintiris via llvm-commits]

vkalintiris requested changes to this revision.
This revision now requires changes to proceed.

================
Comment at: lib/Target/Mips/Mips.h:34
@@ -33,2 +33,3 @@
   FunctionPass *createMipsConstantIslandPass(MipsTargetMachine &tm);
+  FunctionPass *createMipsHazardSchedule(MipsTargetMachine &tm);
 } // end namespace llvm;
----------------
Do we really need this pass? Shouldn't we just take care so that the MipsDelaySlotFiller pass does not create any hazards in the first place?

================
Comment at: lib/Target/Mips/MipsDelaySlotFiller.cpp:507-559
@@ -506,1 +506,55 @@
 
+// For a given branch return the corresponding compact form.
+static int getEquivalentCompactForm(Iter Branch) {
+
+  // Pick the implict zero register operand form of the branch if available
+  // for more readable assembly and a greater offset range of the EQ and NE
+  // cases,
+  if ((unsigned)Branch->getNumExplicitOperands() == 3 &&
+      (unsigned)Branch->getOperand(1).getReg() == Mips::ZERO) {
+    switch ((unsigned)Branch->getOpcode()) {
+    case Mips::BEQ:
+      return Mips::BEQZC;
+    case Mips::BNE:
+      return Mips::BNEZC;
+    case Mips::BGE:
+      return Mips::BGEZC;
+    case Mips::BGT:
+      return Mips::BGTZC;
+    case Mips::BLT:
+      return Mips::BLTZC;
+    case Mips::BLE:
+      return Mips::BLEZC;
+    default:
+      break;
+    }
+  }
+
+  switch ((unsigned)Branch->getOpcode()) {
+  case Mips::B:
+    return Mips::BC;
+  case Mips::BEQ:
+    return Mips::BEQC;
+  case Mips::BNE:
+    return Mips::BNEC;
+  case Mips::BGE:
+    return Mips::BGEC;
+  case Mips::BGEU:
+    return Mips::BGEUC;
+  case Mips::BGEZ:
+    return Mips::BGEZC;
+  case Mips::BGTZ:
+    return Mips::BGTZC;
+  case Mips::BLEZ:
+    return Mips::BLEZC;
+  case Mips::BLT:
+    return Mips::BLTC;
+  case Mips::BLTU:
+    return Mips::BLTUC;
+  case Mips::BLTZ:
+    return Mips::BLTZC;
+  default:
+    return Mips::ZERO;
+  }
+}
+
----------------
We should make this function a member of MipsSEInstrInfo. This is where we keep similar logic for branch instruction operations.

Can you merge the first switch statement into the second? The check for the number of explicit operands becomes unnecessary once you've identified the opcode. Also, this is where we should provide the relevant logic for branches in microMIPS mode.

================
Comment at: lib/Target/Mips/MipsDelaySlotFiller.cpp:557
@@ +556,3 @@
+  default:
+    return Mips::ZERO;
+  }
----------------
I think that we should return 0 here. Mips::ZERO is a positive number.

================
Comment at: lib/Target/Mips/MipsDelaySlotFiller.cpp:564-612
@@ -509,20 +563,51 @@
                                       Iter Branch, DebugLoc DL) {
   const MipsInstrInfo *TII =
       MBB.getParent()->getSubtarget<MipsSubtarget>().getInstrInfo();
+  const MipsSubtarget &STI = MBB.getParent()->getSubtarget<MipsSubtarget>();
 
-  unsigned NewOpcode =
-    (((unsigned) Branch->getOpcode()) == Mips::BEQ) ? Mips::BEQZC_MM
-                                                    : Mips::BNEZC_MM;
+  unsigned NewOpcode;
+  if (STI.inMicroMipsMode())
+    NewOpcode = (((unsigned)Branch->getOpcode()) == Mips::BEQ) ? Mips::BEQZC_MM
+                                                               : Mips::BNEZC_MM;
+  else
+    NewOpcode = getEquivalentCompactForm(Branch);
 
   const MCInstrDesc &NewDesc = TII->get(NewOpcode);
   MachineInstrBuilder MIB = BuildMI(MBB, Branch, DL, NewDesc);
 
-  MIB.addReg(Branch->getOperand(0).getReg());
-  MIB.addMBB(Branch->getOperand(2).getMBB());
+  switch (NewOpcode) {
+  case Mips::BC:
+    MIB.addMBB(Branch->getOperand(0).getMBB());
+    break;
+  case Mips::BEQZC:
+  case Mips::BNEZC:
+  case Mips::BGEZC:
+  case Mips::BGTZC:
+  case Mips::BLTZC:
+  case Mips::BLEZC:
+  case Mips::BEQZC_MM:
+  case Mips::BNEZC_MM:
+    MIB.addReg(Branch->getOperand(0).getReg());
+    MIB.addMBB(
+        Branch->getOperand(Branch->getNumExplicitOperands() - 1).getMBB());
+    break;
+  case Mips::BEQC:
+  case Mips::BNEC:
+  case Mips::BGEC:
+  case Mips::BGEUC:
+  case Mips::BLTC:
+  case Mips::BLTUC:
+    MIB.addReg(Branch->getOperand(0).getReg());
+    MIB.addReg(Branch->getOperand(1).getReg());
+    MIB.addMBB(Branch->getOperand(2).getMBB());
+    break;
+  default:
+    llvm_unreachable("Unknown MIPS compact branch type");
+  }
 
   Iter tmpIter = Branch;
   Branch = std::prev(Branch);
   MBB.erase(tmpIter);
 
   return Branch;
 }
----------------
The same functionality is implemented in MipsInstrInfo::genInstrWithNewOpc(). You can expand the logic of that function, for branches that use the $zero register.


================
Comment at: lib/Target/Mips/MipsDelaySlotFiller.cpp:702-708
@@ -616,9 +701,9 @@
       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:
----------------
We can remove this once we add support for microMIPS in getEquivalentCompactForm().

================
Comment at: lib/Target/Mips/MipsDelaySlotFiller.cpp:723
@@ +722,3 @@
+    // form of the branch. This should save putting in a NOP.
+    if ((STI.hasMips32r6() || STI.hasMips64r6()) &&
+        (getEquivalentCompactForm(I) != Mips::ZERO)) {
----------------
The `STI.hasMips64r6()` will be true if `STI.hasMip32r6()` is true.

================
Comment at: test/CodeGen/Mips/compact-branches.ll:1
@@ +1,2 @@
+; RUN: llc -march=mipsel -mcpu=mips32r6 -relocation-model=static < %s | FileCheck %s
+
----------------
Although it isn't absolutely necessary, it would be better to have a function per instruction check, ie. one function that tests bnec, another one for bgezc etc.


http://reviews.llvm.org/D16353