[MIPS] BuildCondBr should preserve MO flags

[npjdesres]

Summary

While simplifying branches in the MachineInstr representation, the
routine BuildCondBr must preserve flags on register MachineOperands. In
particular, it must preserve the <undef> flag.

This fixes a bug that is unlikely to occur in any real scenario, but
which bugpoint is likely to introduce.

[npjdesres]

Updated patch to include greater context.

[ahatanak]

I haven't looked at this code recently, so someone currently working on mips should review the patch.

lib/Target/Mips/MipsInstrInfo.cpp

107 ↗

(On Diff #101940)

I wonder why MachineInstrBuilder::add wasn't called to add regs and imms?

test/CodeGen/Mips/brundef.ll

123 ↗

(On Diff #101940)

Test case can be simpler. You can probably remove some of the basic blocks and attributes and metadata that don't affect code-gen.

[sdardis]

Some small changes requested, inlined. Can you also simplify the test case?

Thanks for doing this.
Simon

lib/Target/Mips/MipsInstrInfo.cpp

106–111 ↗

(On Diff #101940)

As Akira notes, this can all be refactored to:

assert((Cond[i].isImm() || Cond[i].isReg()) && "Cannot copy operand for conditional branch!");
MIB.add(Cond[i]);

107 ↗

(On Diff #101940)

It appears the code was simply extended to accept immediates for mips16, suggesting that at one point perhaps there was some pseudo branch instruction that took an immediate?

I quickly tested removing the immediate case and it passed the codegen section of test-suite, suggesting the immediate case is dead code (or that our test coverage doesn't cover that case).

test/CodeGen/Mips/brundef.ll

1 ↗

(On Diff #101940)

This runline should be: llc -march=mips -mcpu=mips32 < %s -verify-machineinstrs -o /dev/null

The '> %t' is un-necessary if we're testing that the program can be compiled without a verifier error, so we don't need to preserve the output.

The -mcpu=mips32 silences a warning from llc about there being no 'generic' processor for mips.

Please also add a short comment describing what the purpose of this test.

2 ↗

(On Diff #101940)

You can remove the 'local_unnamed_addr #0 align 2' here.

[npjdesres]

Update to reflect reviews. Shorter testcase.

[sdardis]

LGTM.

Do you need me to commit this for you?

Thanks for doing this.
Simon

[npjdesres]

I do not have commit access, so if you could commit this I would appreciate it. Thank you.

llvm/trunk/lib/Target/Mips/MipsInstrInfo.cpp

 void MipsInstrInfo::BuildCondBr(MachineBasicBlock &MBB, MachineBasicBlock *TBB,
                                 const DebugLoc &DL,
                                 ArrayRef<MachineOperand> Cond) const {
   unsigned Opc = Cond[0].getImm();
   const MCInstrDesc &MCID = get(Opc);
   MachineInstrBuilder MIB = BuildMI(&MBB, DL, MCID);
 
   for (unsigned i = 1; i < Cond.size(); ++i) {
-    if (Cond[i].isReg())
-      MIB.addReg(Cond[i].getReg());
-    else if (Cond[i].isImm())
-      MIB.addImm(Cond[i].getImm());
-    else
-       assert(false && "Cannot copy operand");
+    assert((Cond[i].isImm() || Cond[i].isReg()) &&
+           "Cannot copy operand for conditional branch!");
+    MIB.add(Cond[i]);
   }
   MIB.addMBB(TBB);
 }
 
 unsigned MipsInstrInfo::insertBranch(MachineBasicBlock &MBB,
                                      MachineBasicBlock *TBB,
                                      MachineBasicBlock *FBB,
                                      ArrayRef<MachineOperand> Cond,

llvm/trunk/test/CodeGen/Mips/brundef.ll

+; RUN: llc -march=mips -mcpu=mips32 -verify-machineinstrs -o /dev/null < %s 
+; Confirm that MachineInstr branch simplification preserves
+; register operand flags, such as the <undef> flag.
+
+define void @ham() {
+bb:
+  %tmp = alloca i32, align 4
+  %tmp13 = ptrtoint i32* %tmp to i32
+  %tmp70 = icmp eq i32 undef, -1
+  br i1 %tmp70, label %bb72, label %bb40
+
+bb72:                                             ; preds = %bb72, %bb
+  br i1 undef, label %bb40, label %bb72
+
+bb40:                                             ; preds = %bb72, %bb
+  %tmp41 = phi i32 [ %tmp13, %bb72 ], [ %tmp13, %bb ]
+  %tmp55 = inttoptr i32 %tmp41 to i32*
+  %tmp58 = insertelement <2 x i32*> undef, i32* %tmp55, i32 1
+  br label %bb59
+
+bb59:                                             ; preds = %bb59, %bb40
+  %tmp60 = phi <2 x i32*> [ %tmp61, %bb59 ], [ %tmp58, %bb40 ]
+  %tmp61 = getelementptr i32, <2 x i32*> %tmp60, <2 x i32> <i32 -1, i32 1>
+  %tmp62 = extractelement <2 x i32*> %tmp61, i32 1
+  br label %bb59
+}