933104 - OdinMonkey: Assertion failure: max_exponent_ + canHaveFractionalPart_ >= mozilla::FloorLog2(mozilla::Abs(upper_)), at jit/RangeAnalysis.h

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: stack without symbols

(function() {
    "use asm"
    function f(x) {
        x = +x
        x = -2.;
        (x > -1.5) ? 0 : 0
    }
})()

asserts js debug shell on m-c changeset 829d7bef8b0a with --ion-gvn=off at Assertion failure: max_exponent_ + canHaveFractionalPart_ >= mozilla::FloorLog2(mozilla::Abs(upper_)), at jit/RangeAnalysis.h


Tested with:

https://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-macosx64-debug/1383099757/jsshell-mac64.zip

which I presume is a 64-bit debug non-deterministic threadsafe build.

Guessing this is a range analysis-related regression, setting needinfo for Dan (:sunfish).

Comment 1

[Dan Gohman [:sunfish]]

Confirmed. This is similar to bug 928450, but slightly different. Range::intersect is again computing an invalid range on an unreachable path.

"Alice laughed: "There's no use trying," she said; "one can't believe impossible things." "I daresay you haven't had much practice," said the Queen.

Comment 2

[Dan Gohman [:sunfish]]

Attachment: range-empty-after-intersect.patch

Comment 3

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 826363 [details] [diff] [review]
range-empty-after-intersect.patch

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

::: js/src/jit/RangeAnalysis.cpp
@@ +294,5 @@
> +   if (!r->canHaveFractionalPart())
> +       return false;
> +
> +   // Otherwise, if the bounds are conservatively rounded across a power-of-two
> +   // boundary, the exponent may imply a tigher range.

tighter

Comment 4

[Nicolas B. Pierron [:nbp]]

Comment on attachment 826363 [details] [diff] [review]
range-empty-after-intersect.patch

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

::: js/src/jit/RangeAnalysis.cpp
@@ +429,5 @@
> +        // When intersecting F[0,2] (< pow(2, 0+1)) with a range like F[2,4],
> +        // the naive intersection is F[2,2], but using the max exponent, we can
> +        // discover that the intersection is actually empty.
> +        if (newHasInt32LowerBound && newHasInt32UpperBound &&
> +            newLower == newUpper &&

Note that independently of the exponent, this conditions implies that the Range would be an Integer range.
Then, if we can refine the newLower & newUpper based on the exponent, and check that we still have a non-empty intersection.

So, I think we can just add this case as part of the previous condition, as the content of the previous if-block should already handle this case.

    newFractional && newHasInt32LowerBound && newHasInt32UpperBound && newLower == newUpper

Comment 5

[Dan Gohman [:sunfish]]

(In reply to {N/A until Nov. 18} Nicolas B. Pierron [:nbp] from comment #4)
> Comment on attachment 826363 [details] [diff] [review]
> range-empty-after-intersect.patch
> 
> Review of attachment 826363 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jit/RangeAnalysis.cpp
> @@ +429,5 @@
> > +        // When intersecting F[0,2] (< pow(2, 0+1)) with a range like F[2,4],
> > +        // the naive intersection is F[2,2], but using the max exponent, we can
> > +        // discover that the intersection is actually empty.
> > +        if (newHasInt32LowerBound && newHasInt32UpperBound &&
> > +            newLower == newUpper &&
> 
> Note that independently of the exponent, this conditions implies that the
> Range would be an Integer range.
> Then, if we can refine the newLower & newUpper based on the exponent, and
> check that we still have a non-empty intersection.
> 
> So, I think we can just add this case as part of the previous condition, as
> the content of the previous if-block should already handle this case.
> 
>     newFractional && newHasInt32LowerBound && newHasInt32UpperBound &&
> newLower == newUpper

Unfortunately, rounding needs to be handled differently between these two cases. refineInt32BoundsByExponent does rounding based on the assumption that its output range is known in advance to be integer. So if newExponent is 0, it caps newUpper at 1, because that's the greatest integer that has an exponent of 0. However, if neither incoming range is integer, we don't necessarily know that the output will be integer, so rounding newUpper down like that would be incorrect.

This also makes changing Range to use doubles increasingly intriguing ;-).

Comment 6

[Nicolas B. Pierron [:nbp]]

(In reply to Dan Gohman [:sunfish] from comment #5)
> However, if neither incoming range is integer, we don't
> necessarily know that the output will be integer, so rounding newUpper down
> like that would be incorrect.

If newLower == newUpper, then the range is necessary an integer, right?

And the only case where the rectify might do anything is when both operands have a fractional part.  In such case, If the exponent of the intersection is more precise, then either newLower or newUpper would be shifted by 1, which would lead to an empty set.

Comment 7

[Dan Gohman [:sunfish]]

Attachment: range-empty-after-intersect.patch

Ok, I see what you're saying now. This patch handles both conditions together. It's more complex in some ways, but simpler in others.

Comment 8

[Nicolas B. Pierron [:nbp]]

Comment on attachment 830465 [details] [diff] [review]
range-empty-after-intersect.patch

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

::: js/src/jit/RangeAnalysis.cpp
@@ +422,5 @@
> +    // upper bound needs to be adjusted to 1.
> +    //
> +    // When intersecting F[0,2] (< pow(2, 0+1)) with a range like F[2,4],
> +    // the naive intersection is F[2,2], but using the max exponent, we can
> +    // discover that the intersection is actually empty.

nit: …, we can discover that the intersection is actually >an integer and that it might be< empty.

Comment 9

[Dan Gohman [:sunfish]]

(In reply to {N/A until Nov. 18} Nicolas B. Pierron [:nbp] from comment #8)
> Comment on attachment 830465 [details] [diff] [review]
> range-empty-after-intersect.patch
> 
> Review of attachment 830465 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jit/RangeAnalysis.cpp
> @@ +422,5 @@
> > +    // upper bound needs to be adjusted to 1.
> > +    //
> > +    // When intersecting F[0,2] (< pow(2, 0+1)) with a range like F[2,4],
> > +    // the naive intersection is F[2,2], but using the max exponent, we can
> > +    // discover that the intersection is actually empty.
> 
> nit: …, we can discover that the intersection is actually >an integer and
> that it might be< empty.

I reworded the comment to clarify this.

https://hg.mozilla.org/integration/mozilla-inbound/rev/bf086d7163ff

Comment 10

[Ryan VanderMeulen [:RyanVM]]

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