Subzero, MIPS32: Intrinsic call Ctlz for i32

Subzero, MIPS32: Intrinsic call Ctlz for i32

Implements intrinsic call llvm.ctlz for i32

R=stichnot@chromium.org

Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a=commit;h=d27ce3d930109e53fadd82fe9bd05a1323d77298

[obucinac, 2016-09-21 15:53:38]

Description was changed from

==========
Subzero, MIPS32: Intrinsic call Ctlz for i32

Implements intrinsic call llvm.ctlz for i32
==========

to

==========
Subzero, MIPS32: Intrinsic call Ctlz for i32

Implements intrinsic call llvm.ctlz for i32
==========

[obucinac, 2016-09-21 15:53:38]

obucinac@gmail.com changed reviewers:
+ eholk@chromium.org, jaydeep.patil@imgtec.com, jpp@chromium.org,
kschimpf@google.com, sagar.thakur@imgtec.com, stichnot@chromium.org

[Jim Stichnoth, 2016-09-22 13:48:28]

https://codereview.chromium.org/2354293002/diff/1/src/IceTargetLoweringMIPS32...
File src/IceTargetLoweringMIPS32.cpp (right):

https://codereview.chromium.org/2354293002/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.cpp:2813: Type SrcTy = Src->getType();
const Type

https://codereview.chromium.org/2354293002/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.cpp:2814: if (isScalarIntegerType(SrcTy)) {
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.

https://codereview.chromium.org/2354293002/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.cpp:2818: auto *DestR = legalizeToReg(Dest);
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]

https://codereview.chromium.org/2354293002/diff/1/src/IceTargetLoweringMIPS32...
File src/IceTargetLoweringMIPS32.cpp (right):

https://codereview.chromium.org/2354293002/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.cpp:2813: Type SrcTy = Src->getType();
On 2016/09/22 13:48:27, stichnot wrote:
> const Type

Done.

https://codereview.chromium.org/2354293002/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.cpp:2814: if (isScalarIntegerType(SrcTy)) {
On 2016/09/22 13:48:27, stichnot wrote:
> 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.

Done.

https://codereview.chromium.org/2354293002/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.cpp:2818: auto *DestR = legalizeToReg(Dest);
On 2016/09/22 13:48:27, stichnot wrote:
> 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)".

Done.

[Jim Stichnoth, 2016-09-22 18:18:24]

https://codereview.chromium.org/2354293002/diff/20001/src/IceTargetLoweringMI...
File src/IceTargetLoweringMIPS32.cpp (right):

https://codereview.chromium.org/2354293002/diff/20001/src/IceTargetLoweringMI...
src/IceTargetLoweringMIPS32.cpp:2850: const Type SrcTy = Src->getType();
This gives an error in a MINIMAL build (make -f Makefile.standalone MINIMAL=1)
because SrcTy is only used inside an assert.

That said, I think the assert should just be removed because it is handled more
strongly in the "default:" case below.

[obucinac, 2016-09-22 18:58:18]

https://codereview.chromium.org/2354293002/diff/20001/src/IceTargetLoweringMI...
File src/IceTargetLoweringMIPS32.cpp (right):

https://codereview.chromium.org/2354293002/diff/20001/src/IceTargetLoweringMI...
src/IceTargetLoweringMIPS32.cpp:2850: const Type SrcTy = Src->getType();
On 2016/09/22 18:18:24, stichnot wrote:
> This gives an error in a MINIMAL build (make -f Makefile.standalone MINIMAL=1)
> because SrcTy is only used inside an assert.
> 
> That said, I think the assert should just be removed because it is handled
more
> strongly in the "default:" case below.

Maybe this would do...

[Jim Stichnoth, 2016-09-22 19:56:04]

lgtm

[Jim Stichnoth, 2016-09-22 19:56:15]

Description was changed from

==========
Subzero, MIPS32: Intrinsic call Ctlz for i32

Implements intrinsic call llvm.ctlz for i32
==========

to

==========
Subzero, MIPS32: Intrinsic call Ctlz for i32

Implements intrinsic call llvm.ctlz for i32

R=stichnot@chromium.org

Committed:
https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a...
==========

[Jim Stichnoth, 2016-09-22 19:56:17]

Committed patchset #3 (id:40001) manually as
d27ce3d930109e53fadd82fe9bd05a1323d77298 (presubmit successful).