Closed
Bug 458472
Opened 16 years ago
Closed 16 years ago
TM: NaN checking builds infinite branches
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: dvander, Assigned: dvander)
Details
Attachments
(1 file, 1 obsolete file)
941 bytes,
patch
|
gal
:
review+
|
Details | Diff | Splinter Review |
Right now we guard (d == 0 || JSDOUBLE_IS_NaN(d)) in ifop. That NaN check was mine and it looks nonsense in retrospect. By guarding that NaN is always true, we always bail back into the interpreter and always build a new (similarly faulty) branch. For the two orNaNTests in trace-test.js we compile 16 traces because of this. The solution seems to flip the expected value and add an extra NaN-check guard if we expect a NaN. Using the proposed patch I pass the correctness tests and don't have a crazy number of exits/traces.
Attachment #341708 -
Flags: review?(gal)
Updated•16 years ago
|
Attachment #341708 -
Flags: review?(gal) → review-
Comment 1•16 years ago
|
||
Comment on attachment 341708 [details] [diff] [review] proposed fix I think this is a bad idea since it needlessly fans out the tree. How about merging the two feq using ins_chose? (in nj2 we could also use a branch) Or even a builtin. Anything but 2 branches essentially.
Comment 2•16 years ago
|
||
Ok, r+ but file a bug to change it to LIR_j later. We don't want to artificially increase the number of guards. Guard on type, not value!
Assignee | ||
Updated•16 years ago
|
Attachment #341708 -
Flags: review- → review+
Assignee | ||
Comment 3•16 years ago
|
||
Comment on attachment 341708 [details] [diff] [review] proposed fix Yup, agree, want to get this fixed in the interim though -- one extra guard is better than infinity guards. Filed new bug 458474.
Assignee | ||
Updated•16 years ago
|
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Comment 4•16 years ago
|
||
Backed out. Seems to fail jquery. http://hg.mozilla.org/tracemonkey/rev/f8725b93953d
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 5•16 years ago
|
||
Original patch was flaky and set the tree on fire. New patch 1) doesn't guard twice, since a NaN-biased tree will never take the second guard and 2) fixes the second guard which shouldn't be checking NaN at all. Will keep the NJ2 version of this bug open because there's still an extra guard - NaN tree will build a new branch for non-NaN and then the d == 0 guard can trigger a third.
Assignee: general → danderson
Attachment #341708 -
Attachment is obsolete: true
Status: REOPENED → ASSIGNED
Attachment #341998 -
Flags: review?(gal)
Updated•16 years ago
|
Attachment #341998 -
Flags: review?(gal) → review+
Assignee | ||
Comment 6•16 years ago
|
||
Pushed as changeset http://hg.mozilla.org/tracemonkey/rev/2169e638daf4 (will pay attention to the tinderboxes this time!)
Status: ASSIGNED → RESOLVED
Closed: 16 years ago → 16 years ago
Resolution: --- → FIXED
Updated•16 years ago
|
Flags: in-testsuite-
Flags: in-litmus-
You need to log in
before you can comment on or make changes to this bug.
Description
•