Closed Bug 1356207 Opened 5 years ago Closed 5 years ago

Optimize ToLength better with Ion

Categories

(Core :: JavaScript Engine: JIT, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: evilpie, Assigned: evilpie)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

The self-hosted ToLength method has this branch: `if (v <= 0) return 0;`. We currently don't optimize away this branch, even when we know that v must be in [0, inf). We can also only do this if v is an int32, because otherwise v = -0, would result in -0 and not 0.

To solve this issue I am replacing this branch with Math.max(v, 0), which should be equivalent, even under the -0 or -Infinity case. NaN can't appear, because v is the result of ToInteger.

I didn't want to try and muddle around with the more complex range analysis and instead I just added a targeted optimization for this, based on the ArrayLength case.
Attached patch WIP (obsolete) — Splinter Review
Comment on attachment 8857908 [details] [diff] [review]
WIP

André does the self-hosting part look correct to you?
Attachment #8857908 - Flags: feedback?(andrebargull)
Comment on attachment 8857908 [details] [diff] [review]
WIP

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

Yes, that looks correct to me.
Attachment #8857908 - Flags: feedback?(andrebargull) → feedback+
Thank you André!
Attachment #8857908 - Attachment is obsolete: true
Attachment #8857911 - Flags: review?(hv1989)
Comment on attachment 8857911 [details] [diff] [review]
Optimize ToLength better with Ion

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

Thanks for the extensive tests!

::: js/src/jit/MIR.cpp
@@ +3373,5 @@
> +        // max(array.length, 0) = array.length
> +        // ArrayLength is always >= 0, so just return it.
> +        if (isMax() && constant->toInt32() <= 0) {
> +            return operand;
> +        }

Nit: No need for {}
Attachment #8857911 - Flags: review?(hv1989) → review+
Pushed by evilpies@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/89375918b96e
Optimize self-hosted ToLength better with Ion. r=h4writer
Weird, seems like this didn't help on AWFY at all.
https://hg.mozilla.org/mozilla-central/rev/89375918b96e
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.