[Subzero][MIPS] Implement conditional branches and integer comparisons

src/IceTargetLoweringMIPS32.cpp

Index: src/IceTargetLoweringMIPS32.cpp
diff --git a/src/IceTargetLoweringMIPS32.cpp b/src/IceTargetLoweringMIPS32.cpp
index 238b7bb9d8eb52dddae9b0685600022b5c0d1935..2d42d0f9fa09fff54ed367fbbb20aaac81a5f50c 100644
--- a/src/IceTargetLoweringMIPS32.cpp
+++ b/src/IceTargetLoweringMIPS32.cpp
@@ -956,7 +956,100 @@ void TargetMIPS32::lowerFcmp(const InstFcmp *Instr) {
   UnimplementedLoweringError(this, Instr);
 }
 
+void TargetMIPS32::lower64Icmp(const InstIcmp *Instr) {
+  UnimplementedLoweringError(this, Instr);
+  return;
+}
+
 void TargetMIPS32::lowerIcmp(const InstIcmp *Instr) {

[Jim Stichnoth, 2016/04/19 16:44:01]

This CL is currently lacking tests.  There should be a basic lit test for each
icmp condition.

Unfortunately, the tests_lit/llvm2ice_tests/ directory doesn't seem to have a
suitable test file to add MIPS tests to, so I think you need to create a new
one.  You might start with vector-icmp.ll as a template.

"make -f Makefile.standalone check-lit" to run the tests.

[sagar.thakur, 2016/04/25 09:14:32]

Yes, the test for icmp eq could be something like this :

define internal i32 @test_icmp_eq(i32 %a, i32 %b) {
entry:
  %icmp = icmp eq i32 %a, %b
  %res = zext i1 %icmp to i32
  ret i32 %res
}

But, with this test subzero throws an unimplemented error message from the
lowering code for casts. We also get unimplemented message from
TargetMIPS32::emitVariable(). Therefore, I think we should first implement these
missing bits before we can add these tests. What do you think ?

[Jim Stichnoth, 2016/04/25 15:26:53]

Ah, you're right.  It looks like none of the lowering is implemented yet that
would consume an i1 operand.  So you could have a test that does an icmp and
doesn't use the result, but then liveness analysis with -O2 would remove the
icmp instruction.  So I guess we can wait for conditional branches, sext, or
zext to add icmp tests.

+  auto Src0 = Instr->getSrc(0);
+  auto Src1 = Instr->getSrc(1);
+  if (Src0->getType() == IceType_i64 || Src1->getType() == IceType_i64) {

[Jim Stichnoth, 2016/04/19 16:44:01]

Typically we just check the type of getDest() or getSrc(0) when dispatching on
type - and of course it can't be getDest() in this case.  It would be fine to
assert in lower64Icmp() that Src1 has type i64.

[sagar.thakur, 2016/04/25 09:14:32]

Done.

+      lower64Icmp(Instr);

[Jim Stichnoth, 2016/04/19 16:44:02]

LLVM coding style uses 2-space indents.  If you run "make -f Makefile.standalone
format", it will take care of that for you.

[sagar.thakur, 2016/04/25 09:14:32]

Done.

+      return;
+  }
+  Variable *Dest = Instr->getDest();
+  if (isVectorType(Dest->getType())) {
+    UnimplementedLoweringError(this, Instr);
+    return;
+  }
+  InstIcmp::ICond Cond = Instr->getCondition();
+  auto Src0R = legalizeToReg(Src0);

[Jim Stichnoth, 2016/04/19 16:44:02]

auto *
for these two

[sagar.thakur, 2016/04/25 09:14:32]

Done.

+  auto Src1R = legalizeToReg(Src1);
+  switch(Cond) {
+  case InstIcmp::Eq: {
+    auto DestT = I32Reg(), T = I32Reg();

[Jim Stichnoth, 2016/04/19 16:44:01]

We usually do variable initializations as separate statements:
  auto *DestT = I32Reg();
  auto *T = I32Reg();

Also, all of these "auto"s should be "auto *".

[sagar.thakur, 2016/04/25 09:14:32]

Done.

+    _xor(T, Src0R, Src1R);
+    _sltiu(DestT, T, 1);
+    _mov(Dest, DestT);
+    return;
+  }
+  case InstIcmp::Ne: {
+    auto DestT = I32Reg(), T = I32Reg(), Zero = getZero();

[Jim Stichnoth, 2016/04/19 16:44:01]

The following doesn't need to be addressed in this CL, but it's time to start
thinking about it.

How do you plan to deal with comparisons and other arithmetic on i8 and i16
operands?  (and perhaps i1 as well?)

For x86, we use native 8-bit and 16-bit operations on i8 and i16 operands.  We
have to deal with the complexities of extra restrictions on some i8 operations,
which makes the lowering code a bit more complex.  We also have to deal with
register classes and register aliasing, but that is handled pretty cleanly for
x86 and ARM, and will presumably work just as well for MIPS.

Another approach is to make an invariant that all i8 and i16 source operands are
already sign-extended or zero-extended to i32.  Naively done, this might result
in a lot of redundant sign- or zero-extensions that would need separate analysis
to clean up.

Yet another approach might be to assume that any i8 or i16 source operand
resides in a 32-bit slot (stack location or register), and simply has garbage in
the upper bits.  Some operations, like add, may not care about the upper bits,
which other operations, like icmp, may need to first sign- or zero-extend the
source operands first.  (This may actually be a very reasonable approach, and
I'm sorry I didn't consider it for the x86 lowering...)

[sagar.thakur, 2016/04/25 09:14:31]

I think the third approach would be a good one. With this approach we wouldn't
need to worry about redundant sign- or zero-extensions.

+    _xor(T, Src0R, Src1R);
+    _sltu(DestT, Zero, T);
+    _mov(Dest, DestT);
+    return;
+  }
+  case InstIcmp::Ugt: {
+    auto DestT = I32Reg();
+    _sltu(DestT, Src1R, Src0R);
+    _mov(Dest, DestT);
+    return;
+  }
+  case InstIcmp::Uge: {
+    auto DestT = I32Reg(), T = I32Reg();
+    _sltu(T, Src0R, Src1R);
+    _xori(DestT, T, 1);
+    _mov(Dest, DestT);
+    return;
+  }
+  case InstIcmp::Ult: {
+    auto DestT = I32Reg();
+    _sltu(DestT, Src0R, Src1R);
+    _mov(Dest, DestT);
+    return;
+  }
+  case InstIcmp::Ule:{
+    auto DestT = I32Reg(), T = I32Reg();
+    _sltu(T, Src1R, Src0R);
+    _xori(DestT, T, 1);
+    _mov(Dest, DestT);
+    return;
+  }
+  case InstIcmp::Sgt: {
+    auto DestT = I32Reg();
+    _slt(DestT, Src1R, Src0R);
+   _mov(Dest, DestT);
+   return;
+  }
+  case InstIcmp::Sge: {
+    auto DestT = I32Reg(), T = I32Reg();
+    _slt(T, Src1R, Src0R);
+    _xori(DestT, T, 1);
+    _mov(Dest, DestT);
+    return;
+  }
+  case InstIcmp::Slt: {
+    auto DestT = I32Reg();
+    _slt(DestT, Src0R, Src1R);
+    _mov(Dest, DestT);
+    return;
+  }
+  case InstIcmp::Sle:{
+    auto DestT = I32Reg(), T = I32Reg();
+    _slt(T, Src1R, Src0R);
+    _xori(DestT, T, 1);
+    _mov(Dest, DestT);
+    return;
+  }
+  default:
+    llvm_unreachable("Invalid ICmp operator");
+    return;
+  }
+  (void)Src0;

[Jim Stichnoth, 2016/04/19 16:44:02]

remove these

[sagar.thakur, 2016/04/25 09:14:31]

Done.

+  (void)Src1;
+  (void)Cond;
   UnimplementedLoweringError(this, Instr);

[Jim Stichnoth, 2016/04/19 16:44:01]

I think this can be removed, given the return in the default clause.

[sagar.thakur, 2016/04/25 09:14:32]

Done.

 }