809390 - IonMonkey: improve range analysis of bitand

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Hannes Verschore [:h4writer]]

In a bitand the upper range is only as big as the minimum of both upper ranges.
(We compute it by taking the maximum of both)

Comment 1

[Hannes Verschore [:h4writer]]

Attachment: Patch

Comment 2

[Hannes Verschore [:h4writer]]

Comment on attachment 679100 [details] [diff] [review]
Patch

Jandem pointed me to #799793. I'll create a better patch ;)

Comment 3

[Hannes Verschore [:h4writer]]

Attachment: Patch

This should do it. Now we compute the range of x & 5 being between 0 and 5 correctly. But still do the right thing for x & -1.

Comment 4

[Hannes Verschore [:h4writer]]

Attachment: Patch

Sorry, part of another patch was included. Now only the right stuff.

Comment 5

[Marty Rosenberg [:mjrosenb]]

Comment on attachment 679114 [details] [diff] [review]
Patch

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

::: js/src/ion/RangeAnalysis.cpp
@@ +407,5 @@
> +    // EXCEPT when upper bound of non negative number is max value,
> +    // because negative value can return the whole max value.
> +    // -1 & 5 = 5
> +    if (lhs->lower_ < 0)
> +       upper = rhs->upper_;

this and the next one should probably be Max(upper, rhs->upper_).

Comment 6

[Hannes Verschore [:h4writer]]

(In reply to Marty Rosenberg [:mjrosenb] from comment #5)
> Comment on attachment 679114 [details] [diff] [review]
> Patch
> 
> Review of attachment 679114 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/ion/RangeAnalysis.cpp
> @@ +407,5 @@
> > +    // EXCEPT when upper bound of non negative number is max value,
> > +    // because negative value can return the whole max value.
> > +    // -1 & 5 = 5
> > +    if (lhs->lower_ < 0)
> > +       upper = rhs->upper_;
> 
> this and the next one should probably be Max(upper, rhs->upper_).

Yes, but simplified that is "upper = rhs->upper_" ;).

Comment 7

[Hannes Verschore [:h4writer]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7fa5f6815e08

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7fa5f6815e08

Should this have a test?

Comment 9

[Hannes Verschore [:h4writer]]

(In reply to Ryan VanderMeulen from comment #8)
> https://hg.mozilla.org/mozilla-central/rev/7fa5f6815e08
> 
> Should this have a test?

No, this wasn't actually wrong. We now calculate better bounds for the range. I've updated the title to reflect the patch.