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

[Vasileios Kalintiris via llvm-commits]

vkalintiris added inline comments.

================
Comment at: lib/Target/Mips/MipsDelaySlotFiller.cpp:506-510
@@ -522,5 +505,7 @@
+  unsigned NewOpcode = TII->getEquivalentCompactForm(Branch);
+  TII->genInstrWithNewOpc(NewOpcode, Branch);
 
   Iter tmpIter = Branch;
   Branch = std::prev(Branch);
   MBB.erase(tmpIter);
 
----------------
If you update `Branch` with the return value of `genInstrWithNewOpc()` then deleting the previous branch is as simple as: `std::next(Branch)->eraseFromParent();`

================
Comment at: lib/Target/Mips/MipsDelaySlotFiller.cpp:533-575
@@ -547,2 +532,45 @@
 
+// Predicate for distingushing between control transfer instructions and all
+// other instructions for handling forbidden slots. Consider inline assembly
+// as unsafe as well.
+static bool safeInForbiddenSlot(const MachineInstr *MI) {
+  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.
+static bool hasForbiddenSlot(const MachineInstr *MI) {
+  if (!MI->isBranch())
+    return false;
+
+  switch ((unsigned)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;
+  }
+}
+
 // For given opcode returns opcode of corresponding instruction with short
----------------
I think that it would be useful to have these functions at `MipsInstrInfo` as we could call them from other passes too.

================
Comment at: lib/Target/Mips/MipsDelaySlotFiller.cpp:557
@@ +556,3 @@
+
+  switch ((unsigned)MI->getOpcode()) {
+  case Mips::BEQC:
----------------
The cast is redundant.

================
Comment at: lib/Target/Mips/MipsDelaySlotFiller.cpp:595-659
@@ -566,2 +594,67 @@
 
+// The forbidden slot is the instruction immediately following a compact
+// branch. 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.
+//
+// In such cases, the processor is required to signal a Reserved Instruction
+// exception. Forbidden slot hazards are defined for MIPSR6, no microMIPS.
+//
+// There are three sources of forbidden slot hazards:
+//
+// A) Transforming a delay slot branch into compact branch.
+// B) A previous pass has created a compact branch directly.
+// C) Filling a delay slot using a backwards search when the instruction
+//    moved was in a forbidden slot. This case will create hazards in already
+//    processed code.
+//
+bool Filler::clearFSHazards(MachineFunction &F) {
+  bool Changed = false;
+  const MipsSubtarget &STI = F.getSubtarget<MipsSubtarget>();
+  const MipsSEInstrInfo *TII =
+      static_cast<const MipsSEInstrInfo *>(STI.getInstrInfo());
+
+  for (MachineFunction::iterator FI = F.begin(), FE = F.end(); FI != FE; ++FI) {
+    for (Iter I = (*FI).begin(); I != (*FI).end(); ++I) {
+      if (hasForbiddenSlot(I)) {
+        if (std::next(I) != (*FI).end() &&
+            !safeInForbiddenSlot(&*std::next(I))) {
+          BuildMI((*FI), std::next(I), I->getDebugLoc(), TII->get(Mips::NOP));
+          Changed = true;
+        } else {
+          for (auto *Succ : (*FI).successors()) {
+            if ((*FI).isLayoutSuccessor(Succ) &&
+                (Succ->getFirstNonDebugInstr()) != Succ->end() &&
+                !safeInForbiddenSlot((Succ->getFirstNonDebugInstr()))) {
+              BuildMI(&(*FI), I->getDebugLoc(), TII->get(Mips::NOP));
+              Changed = true;
+            }
+          }
+        }
+      }
+    }
+  }
+  return Changed;
+}
+
+bool Filler::runOnMachineFunction(MachineFunction &F) {
+  bool Changed = false;
+  for (MachineFunction::iterator FI = F.begin(), FE = F.end(); FI != FE; ++FI)
+    Changed |= runOnMachineBasicBlock(*FI);
+
+  // Make a second pass over the instructions to clear hazards.
+  const MipsSubtarget &STI = F.getSubtarget<MipsSubtarget>();
+  if (STI.hasMips32r6() && !STI.inMicroMipsMode())
+    Changed |= clearFSHazards(F);
+
+  // This pass invalidates liveness information when it reorders
+  // instructions to fill delay slot. Without this, -verify-machineinstrs
+  // will fail.
+  if (Changed)
+    F.getRegInfo().invalidateLiveness();
+
+  return Changed;
+}
+
 /// runOnMachineBasicBlock - Fill in delay slots for the given basic block.
----------------
There's no need to iterate over every instruction twice. We can check whether we have to add a NOP inside `replaceWithCompactBranch()` and bundle it together with the compact branch.

================
Comment at: lib/Target/Mips/MipsInstrInfo.cpp:286
@@ +285,3 @@
+  // branch distance in non-microMIPS mode.
+  if (I->isBranch() && I->getNumExplicitOperands() == 3 &&
+      (unsigned)I->getOperand(1).getReg() == Mips::ZERO) {
----------------
We don't have to check for the number of explicit operands.

================
Comment at: lib/Target/Mips/MipsInstrInfo.cpp:287
@@ +286,3 @@
+  if (I->isBranch() && I->getNumExplicitOperands() == 3 &&
+      (unsigned)I->getOperand(1).getReg() == Mips::ZERO) {
+    ZeroOperandBranch = true;
----------------
You can use MipsABIInfo::GetZeroReg() to test with the right zero register. Also, the cast is unnecessary.

================
Comment at: lib/Target/Mips/MipsInstrInfo.cpp:302-303
@@ +301,4 @@
+      break;
+    default:
+      break;
+    }
----------------
Shouldn't we set `ZeroOperandBranch` to false in the default case?

================
Comment at: lib/Target/Mips/MipsSEInstrInfo.cpp:438
@@ +437,3 @@
+MipsSEInstrInfo::getEquivalentCompactForm(MachineBasicBlock::iterator I) const {
+
+  const MipsSubtarget &STI =
----------------
Redundant newline.

================
Comment at: lib/Target/Mips/MipsSEInstrInfo.cpp:473-482
@@ +472,12 @@
+
+  if (STI.inMicroMipsMode()) {
+    switch (Opcode) {
+    case Mips::BEQ:
+    case Mips::BNE:
+      if ((unsigned)I->getOperand(1).getReg() == Mips::ZERO)
+        return Opcode == Mips::BEQ ? Mips::BEQZC_MM : Mips::BNEZC_MM;
+    default:
+      return 0;
+    }
+  }
+  return 0;
----------------
We will never reach this if `STI.hasMips32r6()` is true. We should merge the logic of opcode selection into the first switch statement.


http://reviews.llvm.org/D16353