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)
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)
3.30 KB,
text/plain

Details  
5.27 KB,
patch

nbp
:
review+

Details  Diff  Splinter Review 
(function() { "use asm" function f(x) { x = +x x = 2.; (x > 1.5) ? 0 : 0 } })() asserts js debug shell on mc changeset 829d7bef8b0a with iongvn=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/tinderboxbuilds/mozillacentralmacosx64debug/1383099757/jsshellmac64.zip which I presume is a 64bit debug nondeterministic threadsafe build. Guessing this is a range analysisrelated regression, setting needinfo for Dan (:sunfish).
Flags: needinfo?(sunfish)
Assignee  
Comment 1•6 years ago


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)
Assignee  
Comment 2•6 years ago


Attachment #826363 
Flags: review?(nicolas.b.pierron)
Comment 3•6 years ago


Comment on attachment 826363 [details] [diff] [review] rangeemptyafterintersect.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 poweroftwo > + // boundary, the exponent may imply a tigher range. tighter
Comment 4•6 years ago


Comment on attachment 826363 [details] [diff] [review] rangeemptyafterintersect.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 nonempty intersection. So, I think we can just add this case as part of the previous condition, as the content of the previous ifblock should already handle this case. newFractional && newHasInt32LowerBound && newHasInt32UpperBound && newLower == newUpper
Attachment #826363 
Flags: review?(nicolas.b.pierron) → feedback+
Assignee  
Comment 5•6 years ago


(In reply to {N/A until Nov. 18} Nicolas B. Pierron [:nbp] from comment #4) > Comment on attachment 826363 [details] [diff] [review] > rangeemptyafterintersect.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 nonempty intersection. > > So, I think we can just add this case as part of the previous condition, as > the content of the previous ifblock 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•6 years ago


(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.
Reporter  
Updated•6 years ago

Component: JavaScript Engine → JavaScript Engine: JIT
Assignee  
Comment 7•6 years ago


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 8•6 years ago


Comment on attachment 830465 [details] [diff] [review] rangeemptyafterintersect.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+
Assignee  
Comment 9•6 years ago


(In reply to {N/A until Nov. 18} Nicolas B. Pierron [:nbp] from comment #8) > Comment on attachment 830465 [details] [diff] [review] > rangeemptyafterintersect.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/mozillainbound/rev/bf086d7163ff
Comment 10•6 years ago


https://hg.mozilla.org/mozillacentral/rev/bf086d7163ff
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: intestsuite+
Resolution:  → FIXED
Target Milestone:  → mozilla28
You need to log in
before you can comment on or make changes to this bug.
Description
•