[PATCH] implement integer compare in mips fast-isel

[Daniel Sanders]

LGTM with the redundant instruction comments added to the test.

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

================
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:
> > 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.
Done. When you do follow up on the duplicate type checks in a later patch, I'm expecting most of getRegEnsuringSimpleIntegerWidening() to end up folded into EmitIntExt(). Please follow up on this fairly soon.

================
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:
> 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.
I think you've misunderstood me. The comment you've added to the implementation is good but I actually intended for a comment to be left on the redundant instructions of these tests. As in:
  ; FIXME: This instruction is redundant. The sltiu can only produce 0 and 1.
  ; CHECK: andi ${{[0-9]+}}, $[[REG2]], 1

The aim is to clearly mark parts of the test that are suboptimal or undesirable but we've temporarily compromised on. This helps us to remember issues we need to come back to and also helps anyone who accidentally affects this test in some way.

http://reviews.llvm.org/D5566