Closed Bug 942236 Opened 8 years ago Closed 8 years ago

Optimize unsigned div, mod, and ursh

Categories

(Core :: JavaScript Engine: JIT, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: sunfish, Assigned: sunfish)

References

Details

(Whiteboard: [qa-])

Attachments

(1 file)

Attached is a patch which builds on the fix for bug 941877 and adds range analysis optimizations to convert MDiv and MMod to unsigned in more cases, and to drop the bailout checks on MUrsh when possible.
Attachment #8336908 - Flags: review?(nicolas.b.pierron)
Comment on attachment 8336908 [details] [diff] [review]
range-optimize-div-mod.patch

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

This patch depends on Bug 941877's patch, but all the guards are here to prevent corner cases which are not clear  in Bug 941877's patch. => r+, even if the dependent patch does not have an r+ yet.

::: js/src/jit/RangeAnalysis.cpp
@@ +1137,5 @@
> +
> +    // If the most significant bit of our result is always going to be zero,
> +    // we can optimize by disabling bailout checks for enforcing an int32 range.
> +    if (left.lower() >= 0 || right.lower() > 0)
> +        bailoutsDisabled_ = true;

Nice, but this does not help at computing the range, this should be moved to the collectRangeInfo analysis.

@@ +1295,5 @@
>  
>      // Something simple for now: When dividing by a positive rhs, the result
>      // won't be further from zero than lhs.
>      if (rhs.lower() > 0 && lhs.lower() >= 0) {
>          setRange(new Range(0, lhs.upper(), true, lhs.exponent()));

note: I think we should make follow-up patches to replace "rhs.lower() > 0" by "rhs.lower() >= 1".  Even if I agree that comparing against zero is more efficient, the type of the bounds is hiding the logic here.
Attachment #8336908 - Flags: review?(nicolas.b.pierron) → review+
(In reply to Nicolas B. Pierron [:nbp] from comment #1)
> Comment on attachment 8336908 [details] [diff] [review]
> range-optimize-div-mod.patch
> 
> Review of attachment 8336908 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This patch depends on Bug 941877's patch, but all the guards are here to
> prevent corner cases which are not clear  in Bug 941877's patch. => r+, even
> if the dependent patch does not have an r+ yet.
> 
> ::: js/src/jit/RangeAnalysis.cpp
> @@ +1137,5 @@
> > +
> > +    // If the most significant bit of our result is always going to be zero,
> > +    // we can optimize by disabling bailout checks for enforcing an int32 range.
> > +    if (left.lower() >= 0 || right.lower() > 0)
> > +        bailoutsDisabled_ = true;
> 
> Nice, but this does not help at computing the range, this should be moved to
> the collectRangeInfo analysis.

Done (now renamed to collectRangeInfoPreTrunc).

> @@ +1295,5 @@
> >  
> >      // Something simple for now: When dividing by a positive rhs, the result
> >      // won't be further from zero than lhs.
> >      if (rhs.lower() > 0 && lhs.lower() >= 0) {
> >          setRange(new Range(0, lhs.upper(), true, lhs.exponent()));
> 
> note: I think we should make follow-up patches to replace "rhs.lower() > 0"
> by "rhs.lower() >= 1".  Even if I agree that comparing against zero is more
> efficient, the type of the bounds is hiding the logic here.

I fixed updated all the cases in this patch and the unsigned MMod/MDiv patch to do this.

https://hg.mozilla.org/integration/mozilla-inbound/rev/e14cd09c843e
https://hg.mozilla.org/mozilla-central/rev/e14cd09c843e
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.