513063 - Avoid bit twiddling on double values

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Andreas Gal :gal]

The attached patch performs DOUBLE_IS_INT and friends directly on floating point registers using compiler intrinsics to determine whether a number is nan or finite, instead of forcing it into memory and meddle with the bit representation.

Comment 1

[Andreas Gal :gal]

Attachment: patch

Comment 2

[Brendan Eich [:brendan]]

Comment on attachment 397094 [details] [diff] [review]
patch

> static JSDHashNumber
> HashDouble(JSDHashTable *table, const void *key)
> {
>-    jsdouble d;
>-
>     JS_ASSERT(IS_DOUBLE_TABLE(table));
>-    d = *(jsdouble *)key;
>-    return JSDOUBLE_HI32(d) ^ JSDOUBLE_LO32(d);
>+    return JS_HASH_INT64(key);

Is JS_HASH_INT64 strict-aliasing safe?

>-#ifdef HPUX
>-                /*
>-                 * Negation of a zero doesn't produce a negative
>-                 * zero on HPUX. Perform the operation by bit
>-                 * twiddling.
>-                 */
>-                JSDOUBLE_HI32(d) ^= JSDOUBLE_HI32_SIGNBIT;
>-#else
>                 d = -d;
>-#endif

There are three such old HPUX ifdefs:

$ grep HPUX *.cpp
jsnum.cpp:#ifdef HPUX
jsnum.cpp:             * here on HPUX. Force a negative zero instead.
jsops.cpp:#ifdef HPUX
jsops.cpp:                 * zero on HPUX. Perform the operation by bit
jsparse.cpp:#ifdef HPUX
jsparse.cpp:                 * zero on HPUX. Perform the operation by bit

Remove them all, or leave them all -- no 1/3rd measures :-P.

r=me with these addressed.

/be

Comment 3

[Andreas Gal :gal]

void* should be exempt from alias analysis since its not a proper type. On top of that, HashDouble is called via a virtual dispatch. So if there is an alias problem here, its the double going into the void* pointer. Whether we convert the void* on the other side of the virtual call into a double* doesn't really matter. It would be too late (if this was a problem, which it isn't).

I will remove the other HPUX stuff too. Thanks.

Comment 4

[Andreas Gal :gal]

After some additional discussion with jimb we decided to not rely on the virtual dispatch as knowledge boundary. I will add a proper union conversion.

Comment 5

[Andreas Gal :gal]

http://hg.mozilla.org/tracemonkey/rev/2db233807725

Comment 6

[Graydon Hoare :graydon]

Attachment: fix windows breakage

Win32 tinderboxes break on the patch as-landed. Attached patch fixes them.

Comment 7

[Jason Orendorff [:jorendorff]]

Comment on attachment 397134 [details] [diff] [review]
fix windows breakage

OK, looks good, and anything to stop the fires.

Comment 8

[Graydon Hoare :graydon]

http://hg.mozilla.org/tracemonkey/rev/74e62444e779

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/mozilla-central/rev/2db233807725

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/mozilla-central/rev/74e62444e779

Comment 11

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/7ab1462f8344