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.
To clarify, the goal of the simple_number.h changes was to allow a SimpleNumber to take advantage of a 64 bit memory space.
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?
Created attachment 3959 [details] Fix Oops. Previous patch had some garbage in it.
+ 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.
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. >
Created attachment 4006 [details] Fix
Comment on attachment 4006 [details] Fix Darin and Maciej reviewed this.
Created attachment 4007 [details] fast/js/integer-extremes.html Added a layout test.
Created attachment 4008 [details] fast/js/integer-extremes-expected.txt Added expected test results.
Performance testing with i-bench shows no regression. I'm landing this.