[Mips] Make MipsBranchExpansion::replaceBranch() aware of BBIT family of branch

[djtodoro]

Summary

Octeon branches (bbit0/bbit032/bbit1/bbit132) have an immediate operand, so it is legal to have such replacement within MipsBranchExpansion::replaceBranch().

According to the specification, a branch (e.g. bbit0 ) looks like:

bbit0  rs p offset  // p is an immediate operand
  if !rs<p> then branch

Without this patch, an assertion triggers in the method, and the problem has been found in the real example.

[sdardis]

Two nits inlined. Otherwise this patch looks OK, eyeballing the supplied test.

llvm/lib/Target/Mips/MipsBranchExpansion.cpp

344–345

This might be more cleanly expressed as a switch statement over the kind of operand, e.g.

switch (MO.getKind()) {
  case MachineOperand::MO_Register:
    MIB.addReg(MO.getReg();
    continue;
  case MachineOperand::MO_Immediate:
    if (!TII->isBranchWithImm(Br->getOpcode()))
      llvm_unreachable("Unexpected immediate in branch instruction!");
    MIB.addImm(MO.getImm());
    continue;
  case MachineOperand::MO_MachineBasicBlock:
    MIB.addMBB(MO);
    break;
  default:
    llvm_unreachable("Unknown operand kind in MipsBranchExpansion::replaceBranch(..)!");
}

break;

What do you think? You'll also move line 360 into the loop.

llvm/test/CodeGen/Mips/longbranch/long-branch-octeon.ll

4

Auto generate the checks with update_llc_checks.py.

Add a one line comment explaining the purpose of this test. E.g. "Test that Octeon bbit family of branch can be replaced with the long branch pass."

[djtodoro]

@sdardis Thanks for the review!

llvm/lib/Target/Mips/MipsBranchExpansion.cpp

344–345

By moving the line 360 in the loop, that makes sense to me.

llvm/test/CodeGen/Mips/longbranch/long-branch-octeon.ll

4

Sure, I forgot to add it.

[djtodoro]

-refactoring the code and the test
-addressing comments

[atanasyan]

LGTM. Two nits inlined.

Do you have commit access?

llvm/lib/Target/Mips/MipsBranchExpansion.cpp

348

Now we can use more convenient break here because there are no code inside the loop after the switch statement.

355

Ditto

[djtodoro]

Thanks for the review!

Do you have commit access?

Yes, I do.

llvm/lib/Target/Mips/MipsBranchExpansion.cpp

348

Sorry, I’m not sure I follow :) We need to collect all the operands, so we need to continue with the collecting it until we come to the MBB operand? If that is the case, we can’t use the break here.

[atanasyan]

llvm/lib/Target/Mips/MipsBranchExpansion.cpp

348

IMHO break in the switch statement just goes outside the switch. It doesn't stop the enveloping loop.

[djtodoro]

llvm/lib/Target/Mips/MipsBranchExpansion.cpp

348

Oh, I see.. never mind, I wasn't paying attention as much as I should.

Since I don't have a strong opinion on using break/continue within switch, I'll change it.
Thanks!

[djtodoro]

-addressing comments

llvm/lib/Target/Mips/MipsBranchExpansion.cpp

void MipsBranchExpansion::replaceBranch(MachineBasicBlock &MBB, Iter Br,
                                         MachineBasicBlock *MBBOpnd) {
   unsigned NewOpc = TII->getOppositeBranchOpc(Br->getOpcode());
   const MCInstrDesc &NewDesc = TII->get(NewOpc);
 
   MachineInstrBuilder MIB = BuildMI(MBB, Br, DL, NewDesc);
 
   for (unsigned I = 0, E = Br->getDesc().getNumOperands(); I < E; ++I) {
     MachineOperand &MO = Br->getOperand(I);
 
-    if (!MO.isReg()) {
-      assert(MO.isMBB() && "MBB operand expected.");
+    switch (MO.getType()) {
+    case MachineOperand::MO_Register:
+      MIB.addReg(MO.getReg());
+      break;
+    case MachineOperand::MO_Immediate:
+      // Octeon BBIT family of branch has an immediate operand
+      // (e.g. BBIT0 $v0, 3, %bb.1).
+      if (!TII->isBranchWithImm(Br->getOpcode()))
+        llvm_unreachable("Unexpected immediate in branch instruction");
+      MIB.addImm(MO.getImm());
+      break;
+    case MachineOperand::MO_MachineBasicBlock:
+      MIB.addMBB(MBBOpnd);
       break;
+    default:
+      llvm_unreachable("Unexpected operand type in branch instruction");
     }
-
-    MIB.addReg(MO.getReg());
   }
 
-  MIB.addMBB(MBBOpnd);
-
   if (Br->hasDelaySlot()) {
     // Bundle the instruction in the delay slot to the newly created branch
     // and erase the original branch.
     assert(Br->isBundledWithSucc());
     MachineBasicBlock::instr_iterator II = Br.getInstrIterator();
     MIBundleBuilder(&*MIB).append((++II)->removeFromBundle());
   }
   Br->eraseFromParent();

llvm/lib/Target/Mips/MipsInstrInfo.h

public:
 
   /// getRegisterInfo - TargetInstrInfo is a superset of MRegister info.  As
   /// such, whenever a client has an instance of instruction info, it should
   /// always be able to get register info as well (through this method).
   virtual const MipsRegisterInfo &getRegisterInfo() const = 0;
 
   virtual unsigned getOppositeBranchOpc(unsigned Opc) const = 0;
 
+  virtual bool isBranchWithImm(unsigned Opc) const {
+    return false;
+  }
+
   /// Return the number of bytes of code the specified instruction may be.
   unsigned getInstSizeInBytes(const MachineInstr &MI) const override;
 
   void storeRegToStackSlot(MachineBasicBlock &MBB,
                            MachineBasicBlock::iterator MBBI,
                            Register SrcReg, bool isKill, int FrameIndex,
                            const TargetRegisterClass *RC,
                            const TargetRegisterInfo *TRI) const override {

llvm/lib/Target/Mips/MipsSEInstrInfo.h

void loadRegFromStack(MachineBasicBlock &MBB,
                         MachineBasicBlock::iterator MI,
                         Register DestReg, int FrameIndex,
                         const TargetRegisterClass *RC,
                         const TargetRegisterInfo *TRI,
                         int64_t Offset) const override;
 
   bool expandPostRAPseudo(MachineInstr &MI) const override;
 
+  bool isBranchWithImm(unsigned Opc) const override;
+
   unsigned getOppositeBranchOpc(unsigned Opc) const override;
 
   /// Adjust SP by Amount bytes.
   void adjustStackPtr(unsigned SP, int64_t Amount, MachineBasicBlock &MBB,
                       MachineBasicBlock::iterator I) const override;
 
   /// Emit a series of instructions to load an immediate. If NewImm is a
   /// non-NULL parameter, the last instruction is not emitted, but instead

llvm/lib/Target/Mips/MipsSEInstrInfo.cpp

case Mips::MIPSeh_return64:
     expandEhReturn(MBB, MI);
     break;
   }
 
   MBB.erase(MI);
   return true;
 }
 
+/// isBranchWithImm - Return true if the branch contains an immediate
+/// operand (\see lib/Target/Mips/MipsBranchExpansion.cpp).
+bool MipsSEInstrInfo::isBranchWithImm(unsigned Opc) const {
+  switch (Opc) {
+  default:
+    return false;
+  case Mips::BBIT0:
+  case Mips::BBIT1:
+  case Mips::BBIT032:
+  case Mips::BBIT132:
+    return true;
+  }
+}
+
 /// getOppositeBranchOpc - Return the inverse of the specified
 /// opcode, e.g. turning BEQ to BNE.
 unsigned MipsSEInstrInfo::getOppositeBranchOpc(unsigned Opc) const {
   switch (Opc) {
   default:           llvm_unreachable("Illegal opcode!");
   case Mips::BEQ:    return Mips::BNE;
   case Mips::BEQ_MM: return Mips::BNE_MM;
   case Mips::BNE:    return Mips::BEQ;

llvm/test/CodeGen/Mips/longbranch/long-branch-octeon.ll

+;; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+;; Test that Octeon BBIT family of branch can be replaced by
+;; the long branch expansion pass.
+
+; RUN: llc -O3 -mtriple=mips64-octeon-linux -mcpu=octeon -force-mips-long-branch < %s -o - | FileCheck %s
+
+define i64 @bbit1(i64 %a) nounwind {
+; CHECK-LABEL: bbit1:
+; CHECK:       # %bb.0: # %entry
+; CHECK-NEXT:    bbit0 $4, 3, .LBB0_2
+; CHECK-NEXT:    nop
+; CHECK-NEXT:  # %bb.1: # %entry
+; CHECK-NEXT:    j .LBB0_3
+; CHECK-NEXT:    nop
+; CHECK-NEXT:  .LBB0_2: # %endif
+; CHECK-NEXT:    jr $ra
+; CHECK-NEXT:    daddiu $2, $zero, 12
+; CHECK-NEXT:  .LBB0_3: # %if
+; CHECK-NEXT:    jr $ra
+; CHECK-NEXT:    daddiu $2, $zero, 48
+entry:
+  %bit = and i64 %a, 8
+  %res = icmp eq i64 %bit, 0
+  br i1 %res, label %endif, label %if
+if:
+  ret i64 48
+
+endif:
+  ret i64 12
+}
+
+define i64 @bbit132(i64 %a) nounwind {
+; CHECK-LABEL: bbit132:
+; CHECK:       # %bb.0: # %entry
+; CHECK-NEXT:    bbit032 $4, 3, .LBB1_2
+; CHECK-NEXT:    nop
+; CHECK-NEXT:  # %bb.1: # %entry
+; CHECK-NEXT:    j .LBB1_3
+; CHECK-NEXT:    nop
+; CHECK-NEXT:  .LBB1_2: # %endif
+; CHECK-NEXT:    jr $ra
+; CHECK-NEXT:    daddiu $2, $zero, 12
+; CHECK-NEXT:  .LBB1_3: # %if
+; CHECK-NEXT:    jr $ra
+; CHECK-NEXT:    daddiu $2, $zero, 48
+entry:
+  %bit = and i64 %a, 34359738368
+  %res = icmp eq i64 %bit, 0
+  br i1 %res, label %endif, label %if
+if:
+  ret i64 48
+
+endif:
+  ret i64 12
+}
+
+define i64 @bbit0(i64 %a) nounwind {
+; CHECK-LABEL: bbit0:
+; CHECK:       # %bb.0: # %entry
+; CHECK-NEXT:    bbit1 $4, 3, .LBB2_2
+; CHECK-NEXT:    nop
+; CHECK-NEXT:  # %bb.1: # %entry
+; CHECK-NEXT:    j .LBB2_3
+; CHECK-NEXT:    nop
+; CHECK-NEXT:  .LBB2_2: # %endif
+; CHECK-NEXT:    jr $ra
+; CHECK-NEXT:    daddiu $2, $zero, 12
+; CHECK-NEXT:  .LBB2_3: # %if
+; CHECK-NEXT:    jr $ra
+; CHECK-NEXT:    daddiu $2, $zero, 48
+entry:
+  %bit = and i64 %a, 8
+  %res = icmp ne i64 %bit, 0
+  br i1 %res, label %endif, label %if
+if:
+  ret i64 48
+
+endif:
+  ret i64 12
+}
+
+define i64 @bbit032(i64 %a) nounwind {
+; CHECK-LABEL: bbit032:
+; CHECK:       # %bb.0: # %entry
+; CHECK-NEXT:    bbit132 $4, 3, .LBB3_2
+; CHECK-NEXT:    nop
+; CHECK-NEXT:  # %bb.1: # %entry
+; CHECK-NEXT:    j .LBB3_3
+; CHECK-NEXT:    nop
+; CHECK-NEXT:  .LBB3_2: # %endif
+; CHECK-NEXT:    jr $ra
+; CHECK-NEXT:    daddiu $2, $zero, 12
+; CHECK-NEXT:  .LBB3_3: # %if
+; CHECK-NEXT:    jr $ra
+; CHECK-NEXT:    daddiu $2, $zero, 48
+entry:
+  %bit = and i64 %a, 34359738368
+  %res = icmp ne i64 %bit, 0
+  br i1 %res, label %endif, label %if
+if:
+  ret i64 48
+
+endif:
+  ret i64 12
+}