[PATCH] implement integer compare in mips fast-isel

[Daniel Sanders]

Thanks. Your update fixes the most important comments but you've missed a couple. I've replied to each comment indicating whether I think it's been done or not.

================
Comment at: lib/Target/Mips/MipsFastISel.cpp:531
@@ -529,1 +530,3 @@
 
+bool MipsFastISel::SelectCmp(const Instruction *I) {
+  const CmpInst *CI = cast<CmpInst>(I);
----------------
dsanders wrote:
> dsanders wrote:
> > Style nit: This function needs some vertical spacing.
> > What kind of vertical spacing? I'm using clang-format on this file.
> 
> I mean it needs some blank lines to break up the logical sections of the code. For example, a blank line between the LHS/RHS handling, and just before the switch would help readability.
This comment hasn't been handled by your update

================
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);
----------------
dsanders wrote:
> dsanders wrote:
> > 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?
> > What do you mean about duplicating tests inside of EmitIntExt?
> > Not sure what you are referring to.
> 
> At the moment, the caller is testing that LMVT is i8 or i16 and passes LMVT to EmitIntExt as the first argument. EmitIntExt also checks that first argument type is i8 or i16 (in its switch statement). It seems unnecessary to do it twice and have to maintain the list in both places. One possible solution is to call EmitIntExt unconditionally and have it simply return the input register when an extension isn't necessary (e.g. %1 = sext i32 %0 to i32).
> 
Reed and I have chatted about the duplicate type checks and we've agreed to follow up on this in a later patch. This is because it requires refactoring EmitIntExt() so it supports i32->i32 without creating a temporary register.

The remaining part of this comment hasn't been handled by the update though. The LHS/RHS code duplication I'm referring to is:
  if (LeftReg == 0)
    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, IsUnsigned))
      return false;
    LeftReg = TempReg;
  }
then following it is the same code with s/LeftReg/RightReg/ and s/LMVT/RMVT/. If it were only couple lines then I'd leave it but there's enough there that I'd prefer not to duplicate it.

================
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;
+  }
----------------
dsanders wrote:
> These aren't needed. We already have a 'default: return false;'
Done

================
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
----------------
dsanders wrote:
> Please check register numbers match between instructions using variables.
> 
> Likewise for the other tests below.
Done

================
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
+
----------------
dsanders wrote:
> 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
This comment hasn't been handled.

================
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
----------------
dsanders wrote:
> 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
Done

http://reviews.llvm.org/D5566