[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

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

test/CodeGen/Mips/brundef.ll

123

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

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

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

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

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.

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());
+      MIB.addReg(Cond[i].getReg(), getRegState(Cond[i]));
     else if (Cond[i].isImm())
       MIB.addImm(Cond[i].getImm());
     else
        assert(false && "Cannot copy operand");
   }
   MIB.addMBB(TBB);
 }
 
 unsigned MipsInstrInfo::insertBranch(MachineBasicBlock &MBB,
                                      MachineBasicBlock *TBB,
                                      MachineBasicBlock *FBB,
                                      ArrayRef<MachineOperand> Cond,

test/CodeGen/Mips/brundef.ll

+; RUN: llc -march=mips < %s > %t
+define void @ham.928() local_unnamed_addr #0 align 2 {
+bb:
+  %tmp = alloca [100 x i32], align 4
+  %tmp13 = ptrtoint [100 x i32]* %tmp to i32
+  switch i32 undef, label %bb35 [
+    i32 1, label %bb18
+    i32 0, label %bb19
+    i32 3, label %bb20
+    i32 2, label %bb21
+    i32 4, label %bb17
+  ]
+
+bb17:                                             ; preds = %bb
+  unreachable
+
+bb18:                                             ; preds = %bb
+  unreachable
+
+bb19:                                             ; preds = %bb
+  unreachable
+
+bb20:                                             ; preds = %bb
+  unreachable
+
+bb21:                                             ; preds = %bb
+  unreachable
+
+bb35:                                             ; preds = %bb
+  br label %bb69
+
+bb40:                                             ; preds = %bb171, %bb72, %bb71
+  %tmp41 = phi i32 [ %tmp13, %bb71 ], [ %tmp13, %bb72 ], [ %tmp13, %bb171 ]
+  %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
+
+bb69:                                             ; preds = %bb35
+  %tmp70 = icmp eq i32 undef, -1
+  br i1 %tmp70, label %bb72, label %bb71
+
+bb71:                                             ; preds = %bb69
+  br label %bb40
+
+bb72:                                             ; preds = %bb69
+  br i1 undef, label %bb40, label %bb73
+
+bb73:                                             ; preds = %bb72
+  switch i32 undef, label %bb81 [
+    i32 1, label %bb79
+    i32 0, label %bb80
+    i32 3, label %bb76
+    i32 2, label %bb75
+    i32 4, label %bb74
+  ]
+
+bb74:                                             ; preds = %bb73
+  unreachable
+
+bb75:                                             ; preds = %bb73
+  unreachable
+
+bb76:                                             ; preds = %bb73
+  unreachable
+
+bb79:                                             ; preds = %bb73
+  ret void
+
+bb80:                                             ; preds = %bb73
+  unreachable
+
+bb81:                                             ; preds = %bb73
+  switch i32 undef, label %bb171 [
+    i32 1, label %bb169
+    i32 0, label %bb170
+    i32 3, label %bb168
+    i32 2, label %bb167
+    i32 4, label %bb83
+  ]
+
+bb83:                                             ; preds = %bb81
+  unreachable
+
+bb167:                                            ; preds = %bb81
+  unreachable
+
+bb168:                                            ; preds = %bb81
+  unreachable
+
+bb169:                                            ; preds = %bb81
+  unreachable
+
+bb170:                                            ; preds = %bb81
+  unreachable
+
+bb171:                                            ; preds = %bb81
+  switch i32 undef, label %bb40 [
+    i32 4, label %bb175
+    i32 2, label %bb180
+    i32 3, label %bb181
+  ]
+
+bb175:                                            ; preds = %bb171
+  unreachable
+
+bb180:                                            ; preds = %bb171
+  unreachable
+
+bb181:                                            ; preds = %bb171
+  unreachable
+}
+
+attributes #0 = { nounwind "correctly-rounded-divide-sqrt-fp-math"="false" "disable-tail-calls"="false" "less-precise-fpmad"="false" "no-frame-pointer-elim"="true" "no-frame-pointer-elim-non-leaf" "no-infs-fp-math"="false" "no-jump-tables"="false" "no-nans-fp-math"="false" "no-signed-zeros-fp-math"="false" "no-trapping-math"="false" "stack-protector-buffer-size"="8" "target-cpu"="generic" "unsafe-fp-math"="false" "use-soft-float"="false" }
+
+!llvm.ident = !{!0}
+
+!0 = !{!"DESRES-37 clang version 4.0.0  (based on LLVM 4.0.0)"}