Subzero, MIPS32: Intrinsic call Ctlz for i32

src/IceTargetLoweringMIPS32.cpp

Index: src/IceTargetLoweringMIPS32.cpp
diff --git a/src/IceTargetLoweringMIPS32.cpp b/src/IceTargetLoweringMIPS32.cpp
index 6d6c6b9316f43f8e48f4e759d7493ea7a7eea595..e8ab8aa673e08dd3af60bc8edbe561823b952b68 100644
--- a/src/IceTargetLoweringMIPS32.cpp
+++ b/src/IceTargetLoweringMIPS32.cpp
@@ -2809,7 +2809,18 @@ void TargetMIPS32::lowerIntrinsicCall(const InstIntrinsicCall *Instr) {
     return;
   }
   case Intrinsics::Ctlz: {
-    UnimplementedLoweringError(this, Instr);
+    auto *Src = Instr->getArg(0);
+    Type SrcTy = Src->getType();

[Jim Stichnoth, 2016/09/22 13:48:27]

const Type

[obucinac, 2016/09/22 15:42:07]

Done.

+    if (isScalarIntegerType(SrcTy)) {

[Jim Stichnoth, 2016/09/22 13:48:27]

This code seems a bit unbalanced - it appears to do one thing if the type is one
of {i1,i8,i16,i32}, throw an error if the type is i64, and do nothing at all if
the type is float, double or one of the vector types.

The PNaCl bitcode spec
(https://developer.chrome.com/native-client/reference/pnacl-bitcode-abi#intrin...)
guarantees that SrcTy will be only i32 or i64.

So I think the code would be much clearer if you first assert that SrcTy is
either i32 or i64 (like x86 lowerCountZeros), and then do the two cases based on
whether SrcTy is i32 or i64.  Or maybe a switch statement:

  switch (Src->getType()) {
    case IceType_i32: { ... } break;
    case IceType_i64: UnimplementedLoweringError(this, Instr); break;
    default: llvm::report_fatal_error("..."); break;
  }

And then later reuse the same structure for cttz and ctpop.

[obucinac, 2016/09/22 15:42:07]

Done.

+      if (SrcTy == IceType_i64) {
+        UnimplementedLoweringError(this, Instr);
+      } else {
+        auto *DestR = legalizeToReg(Dest);

[Jim Stichnoth, 2016/09/22 13:48:27]

This doesn't seem right to me.  Dest shouldn't actually be live coming into this
high-level instruction, so legalizing it into a register should cause a liveness
error.  I would expect something more like this (though I haven't actually
tested it):

  auto *T = I32Reg();
  auto *SrcR = legalizeToReg(Src);
  _clz(T, SrcR);
  _mov(Dest, T);

This same comment would apply to the 3 other existing instances of
"legalizeToReg(Dest)".

[obucinac, 2016/09/22 15:42:07]

Done.

+        auto *SrcR = legalizeToReg(Src);
+        _clz(DestR, SrcR);
+        _mov(Dest, DestR);
+      }
+    }
     return;
   }
   case Intrinsics::Cttz: {