Subzero, MIPS32: Intrinsic call Ctlz for i32

src/IceTargetLoweringMIPS32.cpp

 //
 //                        The Subzero Code Generator
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 ///
 /// \file
 /// \brief Implements the TargetLoweringMIPS32 class, which consists almost
@@ skipping 2791 matching lines @@
   }
   case Intrinsics::Bswap: {
     UnimplementedLoweringError(this, Instr);
     return;
   }
   case Intrinsics::Ctpop: {
     UnimplementedLoweringError(this, 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: {
     UnimplementedLoweringError(this, Instr);
     return;
   }
   case Intrinsics::Fabs: {
     if (isScalarFloatingType(DestTy)) {
       Variable *T = makeReg(DestTy);
       if (DestTy == IceType_f32) {
@@ skipping 907 matching lines @@
   Str << "\t.set\t"
       << "nomips16\n";
 }
 
 SmallBitVector TargetMIPS32::TypeToRegisterSet[RCMIPS32_NUM];
 SmallBitVector TargetMIPS32::TypeToRegisterSetUnfiltered[RCMIPS32_NUM];
 SmallBitVector TargetMIPS32::RegisterAliases[RegMIPS32::Reg_NUM];
 
 } // end of namespace MIPS32
 } // end of namespace Ice