720094 - Only have one implementation of JSDOUBLE_IS_NaN, not two-via-ifdefs

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

Attachment: Patch

JSDOUBLE_IS_NaN contains two separate implementations, one for MIPS, and one for everything else.  I don't see why there can't just be one.  The attached patch implements this.

(Tangentially, all this double-bit-manipulation/frobbing stuff needs to move out of the JS engine.  The same functionality is also coded up in content/ in different form, but there's no good reason to have to implement it all twice.  But that should be another bug.)

Comment 1

[Jeff Walden [:Waldo]]

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

Review of attachment 590441 [details] [diff] [review]:
-----------------------------------------------------------------

Er, scratch that, I was looking at the wrong part of IEEE-754.  The right patch in a sec.

Comment 2

[Jeff Walden [:Waldo]]

Attachment: Real patch

Comment 3

[Jeff Walden [:Waldo]]

Comment on attachment 590442 [details] [diff] [review]
Real patch

glandium pointed me at the trail of tears in bug 640494 and bug 653056 -- namely, that MSVC+PGO miscompiles the MIPS bitwise algorithm here.  So this version's a no-go.  I'll do some experimenting and figure out an approach that'll appease all the compiler gods, then re-request review once I've done that.

Comment 4

[Jeff Walden [:Waldo]]

Attachment: Patch that definitely passes try, including PGO builds

Okay, this should really do it.  I did a try push that also enabled PGO, and while there seems to be the usual level of orangeness to it, there doesn't seem to be any PGO-specific orange.

https://tbpl.mozilla.org/?tree=Try&rev=3b0d90538d2b

And despite that not mentioning PGO explicitly as a listed "Win pgo" item, PGO definitely happened.  Ed Morley says the string to search for in the logs is "linker max virtual size", and that's definitely present in them.  So I think this is good to go.

Comment 5

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0049666222de

To be safe, this probably shouldn't be closed until we've had a round of PGO builds on m-c pass with it.

Comment 6

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/0049666222de

Is not PGO on inbound enough to close this?

Comment 7

[Jeff Walden [:Waldo]]

Inbound doesn't do PGO builds, I thought.  But, looking back, I see it does them "sometimes", with no rhyme or reason I can see.  As long as those passed, we should be good.

Comment 8

[Marco Bonardo [:mak]]

pgo builds are made every 3 hours, on all trees. Nightlies are also PGO. Merges from inbound to central happen only on pushes that got a green pgo build+tests.

Comment 9

[Jeff Walden [:Waldo]]

In the interest of developer sanity at having to think about different implementations of on trunk and branches, it seemed a good idea to get this in branches as well.  I talked it over with akeybl, and he was persuaded to take it in aurora.

https://hg.mozilla.org/releases/mozilla-aurora/rev/e7312c8640ff

Comment 10

[Jeff Walden [:Waldo]]

...and beta as well:

https://hg.mozilla.org/releases/mozilla-beta/rev/217edd58f5e5