465137 - TM: JIT thinks !NaN is false

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Jesse Ruderman]

js> for (var i=0;i<5;++i) print("NaN is " + (!(NaN) ? "falsy" : "truthy"));
NaN is falsy
NaN is falsy
NaN is truthy
NaN is truthy
NaN is truthy

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Patch

I'm still stepping lightly through this code -- which later stage of processing converts these instructions to non-FP instructions?  Will this != check interfere with what appears to be described as "demotion" in jstracer.cpp?  Alternately, is it potentially faster to short-circuit the computation (but introduce branches) and avoid not evaluating the entire thing every time?

This was more trouble than it should have been because I forgot that not is not not.  (LIR_not is not !, that is.  :-) )

Comment 2

[Andreas Gal :gal]

Comment on attachment 348468 [details] [diff] [review]
Patch

Yeah, always use LIR_f* for numbers in the frontend code generators. Filters do the rest.

Comment 3

[Andreas Gal :gal]

Additional comment: always try to avoid forking of new traces since that can be very costly (if you really take the other path and have to build a whole new trace). The basic guidance is:
- if the type changes, you need a new trace
- if the value is extremely unlikely to be hit, make it a mismatch exit (like out of memory conditions)
- if the type is the same, use or or conditional moves or whatever other magic you need. We should teach the backend to pickup these patterns and optimize them. x = !(a == x) for example is very inefficient atm and does first sete and setne instead of just setne.

Comment 4

[Brendan Eich [:brendan]]

This could get ugly for the beta. Look out for dups. Need to fix before final no matter what.

/be

Comment 5

[Brendan Eich [:brendan]]

(In reply to comment #3)
> - if the type is the same, use or or conditional moves or whatever other magic
> you need. We should teach the backend to pickup these patterns and optimize
> them. x = !(a == x) for example is very inefficient atm and does first sete and
> setne instead of just setne.

Bug on file?

/be

Comment 6

[Jeff Walden [:Waldo]]

Fixed in TM.

Comment 7

[Damon Sicore (:damons)]

reopening, marking blocking beta2, will close once landed on m-c.

Comment 8

[Mike Beltzner [:beltzner, not reading bugmail]]

Fixed in the merge pushed by vlad on Nov 18 14:11:14 2008 -0800:
http://hg.mozilla.org/mozilla-central/rev/e8ed5d4bf531

Comment 9

[Bob Clary [:bc] (inactive)]

testLogicalNotNaN already in js1_8_1/trace/trace-test.js

Comment 10

[Bob Clary [:bc] (inactive)]

test landed http://hg.mozilla.org/mozilla-central/rev/887f1597048e and cvs

Comment 11

[Bob Clary [:bc] (inactive)]

v 1.9.1, 1.9.2