Closed Bug 458472 Opened 16 years ago Closed 16 years ago

TM: NaN checking builds infinite branches

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: dvander, Assigned: dvander)

Details

Attachments

(1 file, 1 obsolete file)

Attached patch proposed fix (obsolete) — 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)
Attachment #341708 - Flags: review?(gal) → review-
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.
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!
Attachment #341708 - Flags: review- → review+
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.
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Backed out. Seems to fail jquery.

http://hg.mozilla.org/tracemonkey/rev/f8725b93953d
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch proposed fix, v2Splinter Review
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)
Attachment #341998 - Flags: review?(gal) → review+
Pushed as changeset http://hg.mozilla.org/tracemonkey/rev/2169e638daf4 (will pay attention to the tinderboxes this time!)
Status: ASSIGNED → RESOLVED
Closed: 16 years ago16 years ago
Resolution: --- → FIXED
Flags: in-testsuite-
Flags: in-litmus-
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: