Closed Bug 933104 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: JavaScript Engine: JIT, defect, critical)

x86_64
macOS
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: gkw, Assigned: sunfish)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, regression, testcase)

Attachments

(2 files, 1 obsolete file)

Attached file 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).
Flags: needinfo?(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.
Assignee: general → sunfish
Flags: needinfo?(sunfish)
Attachment #826363 - Flags: review?(nicolas.b.pierron)
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 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
Attachment #826363 - Flags: review?(nicolas.b.pierron) → feedback+
(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 ;-).
(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.
Component: JavaScript Engine → JavaScript Engine: JIT
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.
Attachment #826363 - Attachment is obsolete: true
Attachment #830465 - Flags: review?(nicolas.b.pierron)
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.
Attachment #830465 - Flags: review?(nicolas.b.pierron) → review+
(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
https://hg.mozilla.org/mozilla-central/rev/bf086d7163ff
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
You need to log in before you can comment on or make changes to this bug.