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

[Vasileios Kalintiris via llvm-commits]

vkalintiris added inline comments.

================
Comment at: lib/Target/Mips/MipsDelaySlotFiller.cpp:549-613
@@ -566,1 +548,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 (TII->HasForbiddenSlot(I)) {
+        if (std::next(I) != (*FI).end() &&
+            !TII->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() &&
+                !TII->SafeInForbiddenSlot((Succ->getFirstNonDebugInstr()))) {
+              BuildMI(&(*FI), I->getDebugLoc(), TII->get(Mips::NOP));
+              Changed = true;
+            }
+          }
+        }
+        if (Changed)
+          MIBundleBuilder(*FI, I, std::next(I, 2));
+      }
+    }
+  }
+  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;
+}
----------------
The (C) case will happen when filling the delay slot of a normal branch, ie. inside the MipsDelaySlotFiller pass. We can teach the "search-backwards" part of the delay slot filler (DSF) algorithm to handle this by checking the last instruction of the BB's layout predecessor. If there are other issues that conceptually belong to the DSF pass, then we should add them too by modifying the pass accordingly.

I don't see how (B) can happen as we only emit compact branches at this pass for the time being. From my perspective, if a previous pass would like to add a compact-branch, then I can think of two options: (a) insert the compact branch only if the next instruction is not a CTI-hazard and leave the task of fixing the corner cases that happen during the filling of normal branches by the DSF, or (b) insert a NOP and just offload everything to the DSF. I can't think of any reason for a post-DSF pass to insert new/additional compact-branches or move non-CTI instructions from a forbidden slot.

Even if we find ourselves constrained by having the DSF pass handling everything, then we can add a separate/final pass that fixes every case left over from previous passes. However, with this design we would achieve the minimum number of places were our code is wrong/invalid.

If we'd want to be on the really safe side and make sure that we are aware of every possible case that we might haven't consider yet, then we can add a separate pass and enable it only for debug builds. It could assert upon finding a compact-branch that contains a CTI in its forbidden slot.


================
Comment at: lib/Target/Mips/MipsInstrInfo.cpp:330
@@ +329,3 @@
+  if (I->isBranch() && I->getOperand(1).isReg() &&
+      (I->getOperand(1).getReg() == Subtarget.getABI().GetZeroReg() ||
+      // FIXME: Certain atomic sequences on mips64 generate 32bit references to
----------------
Ok, if I understand correctly then we should just check for Mips::ZERO and keep the FIXME comment, ie. we can remove the `Subtarget.getABI().GetZeroReg()` check as it's not working at the moment.

================
Comment at: lib/Target/Mips/MipsSEInstrInfo.cpp:438
@@ +437,3 @@
+MipsSEInstrInfo::getEquivalentCompactForm(MachineBasicBlock::iterator I) const {
+  unsigned Opcode = I->getOpcode();
+  bool canUseMicroMipsBranches =
----------------
It doesn't insert one for me, so I suggest that we remove the newline.


http://reviews.llvm.org/D16353