[PATCH] D34041: [MIPS] BuildCondBr should preserve MO flags

[Simon Dardis via Phabricator via llvm-commits]

sdardis requested changes to this revision.
sdardis added a comment.
This revision now requires changes to proceed.

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

Thanks for doing this.
Simon



================
Comment at: lib/Target/Mips/MipsInstrInfo.cpp:106-111
     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");
----------------
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]);


================
Comment at: lib/Target/Mips/MipsInstrInfo.cpp:107
     if (Cond[i].isReg())
-      MIB.addReg(Cond[i].getReg());
+      MIB.addReg(Cond[i].getReg(), getRegState(Cond[i]));
     else if (Cond[i].isImm())
----------------
ahatanak wrote:
> I wonder why MachineInstrBuilder::add wasn't called to add regs and imms?
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).


================
Comment at: test/CodeGen/Mips/brundef.ll:1
+; RUN: llc -march=mips < %s > %t
+define void @ham.928() local_unnamed_addr #0 align 2 {
----------------
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.


================
Comment at: test/CodeGen/Mips/brundef.ll:2
+; RUN: llc -march=mips < %s > %t
+define void @ham.928() local_unnamed_addr #0 align 2 {
+bb:
----------------
You can remove the 'local_unnamed_addr #0 align 2' here.


https://reviews.llvm.org/D34041