995230 - Inline Math.clz32

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Tom Schuster [:evilpie]]

I already have a working patch for x86. There might be some problems with the MIR nodes, because I don't really remember how TypePolicies work. It would be nice if we could avoid the check for 0 most of the time.

Comment 1

[Tom Schuster [:evilpie]]

Attachment: clz32-jit

Hey Kannan. Would you mind taking a look at this patch. I think mostly the MClz implementation could need some improvements. Just for feedback, because ARM isn't even done yet.

Comment 2

[Kannan Vijayan [:djvj]]

Comment on attachment 8407030 [details] [diff] [review]
clz32-jit

Review of attachment 8407030 [details] [diff] [review]:
-----------------------------------------------------------------

Good start.  One comment about avoiding the zero-check on x86: you can try to eliminate a few of those cases by using the range analysis info for the input operation.  If the input operation's int32 range excludes zero, then you can skip zero-check on x86.

This should be possible during lowering.

::: js/src/jit/MCallOptimize.cpp
@@ +701,5 @@
> +        return InliningStatus_NotInlined;
> +
> +    callInfo.setImplicitlyUsedUnchecked();
> +
> +    MInstruction *input = MTruncateToInt32::New(alloc(), callInfo.getArg(0));

Adding MTruncateToInt32 can be made conditional if input is already MIRType_Int32.  I know this will probably get optimized out later on anyway, but feeding less to the optimizers is generally good when possible.

Comment 3

[Tom Schuster [:evilpie]]

Attachment: updated

Finally updated that patch. I am still not sure about what the right type policy is. And still not working on ARM. I hope Marty has some time to do that.

Comment 4

[Marty Rosenberg [:mjrosenb]]

Attachment: arm_clz32-r0.patch

passes the included jit test.

Comment 5

[Marty Rosenberg [:mjrosenb]]

Attachment: re-rebased-r0.patch

I noticed that the updated patch had some hunks that failed to apply, so I fixed those.

Comment 7

[Tom Schuster [:evilpie]]

Attachment: v1

Thanks Marty! Rebased again.

Comment 8

[Douglas Crosher [:dougc]]

Comment on attachment 8472566 [details] [diff] [review]
arm_clz32-r0.patch

Review of attachment 8472566 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.

::: js/src/jit/arm/Assembler-arm.cpp
@@ +1529,5 @@
>      return writeInst(0x0730f010 | c | RN(rd) | RM(rm) | rn.code());
>  }
>  
> +BufferOffset
> +Assembler::as_clz(Register dest, Register src, Condition c, Instruction *instdest)

Nit: There does appear to be any need for the instdest argument so could you remove it.

::: js/src/jit/arm/Assembler-arm.h
@@ +1187,5 @@
>      // instruction. In order to do this very after assembly buffers no longer
>      // exist, when calling with a third dest parameter, a this object is still
>      // needed. Dummy always happens to be null, but we shouldn't be looking at
>      // it in any case.
> +  public:

Nit: there is no need to make Dummy public so could you remove this change.

Comment 9

[Tom Schuster [:evilpie]]

We could optimize this further by using LZCNT. And this instruction could also reuse its input.

Comment 10

[Tom Schuster [:evilpie]]

Attachment: v1

Comment 11

[Jan de Mooij [:jandem]]

Comment on attachment 8473695 [details] [diff] [review]
v1

Review of attachment 8473695 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great, r=me with comments below addressed.

::: js/src/jit-test/tests/basic/testMathClz32.js
@@ +1,4 @@
> +function f() {
> +    var x = 0;
> +    for (var i = 1; i < 1e6; i++) {
> +        if (i > 0)

Nit: what's the point of this branch?

::: js/src/jit/MCallOptimize.cpp
@@ +807,5 @@
> +
> +    callInfo.setImplicitlyUsedUnchecked();
> +
> +    MInstruction *input = MTruncateToInt32::New(alloc(), callInfo.getArg(0));
> +    current->add(input);

Can you remove this and use BitwisePolicy instead, like MBitNot?

::: js/src/jit/MIR.cpp
@@ +3366,5 @@
> +    if (num()->isConstant()) {
> +        int32_t n = num()->toConstant()->value().toInt32();
> +        if (n == 0) {
> +            return MConstant::New(alloc, Int32Value(32));
> +        }

Nit: no {}

Comment 12

[Tom Schuster [:evilpie]]

Comment on attachment 8473695 [details] [diff] [review]
v1

Review of attachment 8473695 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit-test/tests/basic/testMathClz32.js
@@ +1,4 @@
> +function f() {
> +    var x = 0;
> +    for (var i = 1; i < 1e6; i++) {
> +        if (i > 0)

Maybe I wasn't careful enough, but I think I tried this without this branch and the range analysis wasn't able to pick on I never being 0.

Comment 13

[Tom Schuster [:evilpie]]

on *it* never being 0.

Comment 14

[Tom Schuster [:evilpie]]

I had to add specialization = MIRType_Int32 to the constructor to use the BitwisePolicy.
https://hg.mozilla.org/integration/mozilla-inbound/rev/c9ffa291cfe3
https://hg.mozilla.org/integration/mozilla-inbound/rev/9339c5e928e2

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c9ffa291cfe3
https://hg.mozilla.org/mozilla-central/rev/9339c5e928e2