942236 - Optimize unsigned div, mod, and ursh

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, enhancement)

Description

[Dan Gohman [:sunfish]]

Attachment: range-optimize-div-mod.patch

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.

Comment 1

[Nicolas B. Pierron [:nbp]]

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.

Comment 2

[Dan Gohman [:sunfish]]

(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

Comment 3

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e14cd09c843e