Bug 5053 – Need to restore int/long changes to simple_number.h

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Geoffrey Garen]

The fix to 5028 was to roll them out. Once all the JavaScriptCore tests are passing again, we should figure 
out what was wrong with them and restore them.

[Geoffrey Garen]

To clarify, the goal of the simple_number.h changes was to allow a SimpleNumber to take advantage of a 
64 bit memory space.

[Geoffrey Garen]

Created attachment 3958 [details]
Fix

This is a substantial reworking of the original code, after talking with
Maciej. It passes all the JavaScriptCore tests (I didn't run the date tests)
and the layout tests. 

I have two outstanding questions about this: 

(1) What about the previous code would not have worked in 64 bits?
(2) Are all these casts necessary?

[Geoffrey Garen]

Created attachment 3959 [details]
Fix

Oops. Previous patch had some garbage in it.

[Maciej Stachowiak]

+            maxValue  = (signMask >> 2) - 1,
+            minValue  = signMask >> 2

Should use tagBits instead of 2 here.

+        static inline long value(const ValueImp *imp) { return ((unsigned long)imp >> tagBits) | (((long)
imp & signMask) ? ~0 : 0); }

This doeslike look right to me - ~0 is -1, which has all bits on, so | with it will always give all ones, i.e. 
-1 in all cases. It seems like this would turn all negative numbers that fit in SimpleNumber into -1. (~0 
<< valueBits) might do.

+        static inline bool fits(int i)                { return !(i > static_cast<long>(maxValue) || i < 
static_cast<long>(minValue)); }

Does minValue really come out negative when casted to long? if the enum is unsigned, then minValue 
will be positive, if it is signed, then it will depend on >> doing an arithmetic shift. I think it might be 
clearer to replace the enum with static const long / static const unsigned long declarations, so it's clear 
which of the constants are supposed to be signed and which are supposed to be unsigned. If minValue 
is coming out positive then this patch is probably preventing SimpleNumber from being used at all, 
thus masking any other problems.

Given these concerns, I suggest making more tests and doing performance testing on the next shot at 
this.

[Geoffrey Garen]

Yeah, that | ~0 business was just a brain fart.

I like the static const idea.

I'll post a new patch with a layout test soon. (Pop quiz: how do you test the number machinery without relying on 
numbers? I think I have an interesting answer...)

(In reply to comment #4)
> +            maxValue  = (signMask >> 2) - 1,
> +            minValue  = signMask >> 2
> 
> Should use tagBits instead of 2 here.
> 
> +        static inline long value(const ValueImp *imp) { return ((unsigned long)imp >> tagBits) | (((long)
> imp & signMask) ? ~0 : 0); }
> 
> This doeslike look right to me - ~0 is -1, which has all bits on, so | with it will always give all ones, i.e. 
> -1 in all cases. It seems like this would turn all negative numbers that fit in SimpleNumber into -1. (~0 
> << valueBits) might do.
> 
> +        static inline bool fits(int i)                { return !(i > static_cast<long>(maxValue) || i < 
> static_cast<long>(minValue)); }
> 
> Does minValue really come out negative when casted to long? if the enum is unsigned, then minValue 
> will be positive, if it is signed, then it will depend on >> doing an arithmetic shift. I think it might be 
> clearer to replace the enum with static const long / static const unsigned long declarations, so it's clear 
> which of the constants are supposed to be signed and which are supposed to be unsigned. If minValue 
> is coming out positive then this patch is probably preventing SimpleNumber from being used at all, 
> thus masking any other problems.
> 
> Given these concerns, I suggest making more tests and doing performance testing on the next shot at 
> this.
> 

[Geoffrey Garen]

Created attachment 4006 [details]
Fix

[Geoffrey Garen]

Comment on attachment 4006 [details]
Fix

Darin and Maciej reviewed this.

[Geoffrey Garen]

Created attachment 4007 [details]
fast/js/integer-extremes.html

Added a layout test.

[Geoffrey Garen]

Created attachment 4008 [details]
fast/js/integer-extremes-expected.txt

Added expected test results.

[Geoffrey Garen]

Performance testing with i-bench shows no regression. I'm landing this.