Closed Bug 513063 Opened 10 years ago Closed 10 years ago

Avoid bit twiddling on double values


(Core :: JavaScript Engine, defect)

Not set



Tracking Status
status1.9.2 --- beta1-fixed


(Reporter: gal, Assigned: gal)


(Whiteboard: fixed-in-tracemonkey)


(2 files)

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.
Attached patch patchSplinter Review
Assignee: general → gal
Attachment #397094 - Flags: review?(brendan)
Comment on attachment 397094 [details] [diff] [review]

> static JSDHashNumber
> HashDouble(JSDHashTable *table, const void *key)
> {
>-    jsdouble d;
>-    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;
>                 d = -d;

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.

Attachment #397094 - Flags: review?(brendan) → review+
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.
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.
Whiteboard: fixed-in-tracemonkey
Win32 tinderboxes break on the patch as-landed. Attached patch fixes them.
Attachment #397134 - Flags: review?
Attachment #397134 - Flags: review? → review?(jorendorff)
Attachment #397134 - Flags: review?(jorendorff) → review+
Comment on attachment 397134 [details] [diff] [review]
fix windows breakage

OK, looks good, and anything to stop the fires.
Closed: 10 years ago
Resolution: --- → FIXED
Flags: wanted1.9.2+
You need to log in before you can comment on or make changes to this bug.