[PATCH] implement integer compare in mips fast-isel

[Daniel Sanders]

LGTM with a few style and code duplication nits fixed along with an improvement to the testcase.

================
Comment at: lib/Target/Mips/MipsFastISel.cpp:531
@@ -529,1 +530,3 @@
 
+bool MipsFastISel::SelectCmp(const Instruction *I) {
+  const CmpInst *CI = cast<CmpInst>(I);
----------------
Style nit: This function needs some vertical spacing.

================
Comment at: lib/Target/Mips/MipsFastISel.cpp:537-553
@@ +536,19 @@
+    return false;
+  MVT LMVT = TLI.getValueType(Left->getType(), true).getSimpleVT();
+  if ((LMVT == MVT::i8) || (LMVT == MVT::i16)) {
+    unsigned TempReg = createResultReg(&Mips::GPR32RegClass);
+    if (!EmitIntExt(LMVT, LeftReg, MVT::i32, TempReg, CI->isUnsigned()))
+      return false;
+    LeftReg = TempReg;
+  }
+  unsigned RightReg = getRegForValue(Right);
+  if (RightReg == 0)
+    return false;
+  MVT RMVT = TLI.getValueType(Right->getType(), true).getSimpleVT();
+  if ((RMVT == MVT::i8) || (RMVT == MVT::i16)) {
+    unsigned TempReg = createResultReg(&Mips::GPR32RegClass);
+    if (!EmitIntExt(LMVT, RightReg, MVT::i32, TempReg, CI->isUnsigned()))
+      return false;
+    RightReg = TempReg;
+  }
+  unsigned ResultReg = createResultReg(&Mips::GPR32RegClass);
----------------
We've got some code duplication here. Both the LHS and the RHS are sign/zero extended in the same way, and we also duplicate the type checking that's inside EmitIntExt.

Can we factor it out?

================
Comment at: lib/Target/Mips/MipsFastISel.cpp:611-626
@@ +610,18 @@
+
+  case CmpInst::FCMP_OEQ:
+  case CmpInst::FCMP_ONE:
+  case CmpInst::FCMP_OGT:
+  case CmpInst::FCMP_OLT:
+  case CmpInst::FCMP_OLE:
+  case CmpInst::FCMP_UEQ:
+  case CmpInst::FCMP_UNE:
+  case CmpInst::FCMP_UGT:
+  case CmpInst::FCMP_ULT:
+  case CmpInst::FCMP_UGE:
+  case CmpInst::FCMP_ULE:
+  case CmpInst::FCMP_ORD:
+  case CmpInst::FCMP_UNO:
+  case CmpInst::FCMP_TRUE:
+  case CmpInst::FCMP_FALSE:
+    return false;
+  }
----------------
These aren't needed. We already have a 'default: return false;'

================
Comment at: test/CodeGen/Mips/Fast-ISel/icmpa.ll:21-22
@@ +20,4 @@
+  %conv = zext i1 %cmp to i32
+; CHECK:  xor  ${{[0-9]+}}, ${{[0-9]+}}, ${{[0-9]+}}
+; CHECK:  sltiu  ${{[0-9]+}}, ${{[0-9]+}}, 1
+; CHECK:  andi  ${{[0-9]+}}, ${{[0-9]+}}, 1
----------------
Please check register numbers match between instructions using variables.

Likewise for the other tests below.

================
Comment at: test/CodeGen/Mips/Fast-ISel/icmpa.ll:23
@@ +22,3 @@
+; CHECK:  sltiu  ${{[0-9]+}}, ${{[0-9]+}}, 1
+; CHECK:  andi  ${{[0-9]+}}, ${{[0-9]+}}, 1
+
----------------
This instruction is redundant but we don't want to fix that in this patch. We might not want to fix it at all if it proves costly for compilation time. We should leave a comment on redundant instructions though.

Likewise for the others below

================
Comment at: test/CodeGen/Mips/Fast-ISel/icmpa.ll:53
@@ +52,3 @@
+  %conv = zext i1 %cmp to i32
+; CHECK:  sltu  ${{[0-9]+}}, ${{[0-9]+}}, ${{[0-9]+}}
+; CHECK:  andi  ${{[0-9]+}}, ${{[0-9]+}}, 1
----------------
We ought to check that the operands appear in the right order. We can do this with variables by matching the loads first.

Likewise for the tests below

http://reviews.llvm.org/D5566