[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

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

3

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

345

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

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

3

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

349

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

356

Ditto

[djtodoro]

Thanks for the review!

Do you have commit access?

Yes, I do.

llvm/lib/Target/Mips/MipsBranchExpansion.cpp

349

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

349

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

349

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,
   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()) {
+      // Octeon branches "On bit clear/clear plus 32/set/set plus 32"
+      // have an immidiate operand (e.g. BBIT0 $v0, 3, %bb.1).
+      if (MO.isImm() && TII->isBranchWithImm(Br->getOpcode())) {
+        MIB.addImm(MO.getImm());
+        continue;
+      } else {
         assert(MO.isMBB() && "MBB operand expected.");
+      }
       break;
     }
 
     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.

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

+; RUN: llc -O3 -mtriple=mips64-octeon-linux -mcpu=octeon -force-mips-long-branch < %s -o - | FileCheck %s
+
+; CHECK-LABEL: bbit1:
+; CHECK:      %bb.0:
+; CHECK-NEXT:   bbit0 $4, 3, .LBB0_2
+; CHECK-NEXT:   nop
+; CHECK-NEXT: %bb.1:
+; CHECK-NEXT:   j .LBB0_3
+; CHECK-NEXT:   nop
+; CHECK-NEXT: .LBB0_2:
+; CHECK-NEXT:   jr $ra
+; CHECK:      .LBB0_3:
+; CHECK-NEXT:   jr $ra
+
+define i64 @bbit1(i64 %a) nounwind {
+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
+}
+
+; CHECK-LABEL: bbit132:
+; CHECK:      %bb.0:
+; CHECK-NEXT:   bbit032 $4, 3, .LBB1_2
+; CHECK-NEXT:   nop
+; CHECK-NEXT: %bb.1:
+; CHECK-NEXT:   j .LBB1_3
+; CHECK-NEXT:   nop
+; CHECK-NEXT: .LBB1_2:
+; CHECK-NEXT:   jr $ra
+; CHECK:      .LBB1_3:
+; CHECK-NEXT:   jr $ra
+
+define i64 @bbit132(i64 %a) nounwind {
+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
+}
+
+; CHECK-LABEL: bbit0:
+; CHECK:      %bb.0:
+; CHECK-NEXT:   bbit1 $4, 3, .LBB2_2
+; CHECK-NEXT:   nop
+; CHECK-NEXT: %bb.1:
+; CHECK-NEXT:   j .LBB2_3
+; CHECK-NEXT:   nop
+; CHECK-NEXT: .LBB2_2:
+; CHECK-NEXT:   jr $ra
+; CHECK:      .LBB2_3:
+; CHECK-NEXT:   jr $ra
+
+define i64 @bbit0(i64 %a) nounwind {
+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
+}
+
+; CHECK-LABEL: bbit032:
+; CHECK:      %bb.0:
+; CHECK-NEXT:   bbit132 $4, 3, .LBB3_2
+; CHECK-NEXT:   nop
+; CHECK-NEXT: %bb.1:
+; CHECK-NEXT:   j .LBB3_3
+; CHECK-NEXT:   nop
+; CHECK-NEXT: .LBB3_2:
+; CHECK-NEXT:   jr $ra
+; CHECK:      .LBB3_3:
+; CHECK-NEXT:   jr $ra
+
+define i64 @bbit032(i64 %a) nounwind {
+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
+}