Closed Bug 995230 Opened 6 years ago Closed 5 years ago

Inline Math.clz32

Categories

(Core :: JavaScript Engine: JIT, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: evilpie, Assigned: evilpie)

References

Details

Attachments

(2 files, 4 obsolete files)

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.
Attached patch clz32-jit (obsolete) — Splinter Review
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.
Assignee: nobody → evilpies
Status: NEW → ASSIGNED
Attachment #8407030 - Flags: feedback?(kvijayan)
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.
Attachment #8407030 - Flags: feedback?(kvijayan) → feedback+
Attached patch updated (obsolete) — Splinter Review
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.
Flags: needinfo?(mrosenberg)
passes the included jit test.
Attachment #8472566 - Flags: review?(dtc-moz)
Flags: needinfo?(mrosenberg)
Attached patch re-rebased-r0.patch (obsolete) — Splinter Review
I noticed that the updated patch had some hunks that failed to apply, so I fixed those.
Duplicate of this bug: 1053989
Blocks: 1053989
Attached patch v1 (obsolete) — Splinter Review
Thanks Marty! Rebased again.
Attachment #8407030 - Attachment is obsolete: true
Attachment #8464065 - Attachment is obsolete: true
Attachment #8472569 - Attachment is obsolete: true
Attachment #8473681 - Flags: review?(jdemooij)
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.
Attachment #8472566 - Flags: review?(dtc-moz) → review+
We could optimize this further by using LZCNT. And this instruction could also reuse its input.
Attached patch v1Splinter Review
Attachment #8473681 - Attachment is obsolete: true
Attachment #8473681 - Flags: review?(jdemooij)
Attachment #8473695 - Flags: review?(jdemooij)
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 {}
Attachment #8473695 - Flags: review?(jdemooij) → review+
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.
on *it* never being 0.
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
https://hg.mozilla.org/mozilla-central/rev/c9ffa291cfe3
https://hg.mozilla.org/mozilla-central/rev/9339c5e928e2
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in before you can comment on or make changes to this bug.