...
This looks like a spec change from the original one. https://tc39.es/ecma262/#sec-number-constructor-number-value
DFG/FTL support is also required since NumberConstructor is supported in DFG. Maybe, we should introduce CallNumberConstructor DFG node.
Created attachment 397656 [details] Patch
Created attachment 397657 [details] Patch
Created attachment 397658 [details] Patch
Created attachment 397660 [details] Patch
Comment on attachment 397660 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=397660&action=review Some comments while I continue reviewing. If you haven't already done so, can you also run the added tests against Chrome and Firefox as sanity check that there's no unexpected difference in behavior? > Source/JavaScriptCore/runtime/JSBigInt.cpp:2256 > + mantissa >>= 12; // (12 = 64 - 52) (double's mantissa bit count). 12 is the non-mantissa bit count, i.e. exponent and sign bits. 52 is the mantissa it count. > Source/JavaScriptCore/runtime/JSBigInt.cpp:2257 > + int32_t mantissaBitsUnset = shiftAmount - 12; This equation was unclear to me at first, but after it working it out, I see that it's correct. Can you put the following in a comment her to explain how we derived this equation? unsetBits = 64 - setBits - 12 // 12 for non-mantissa bits setBits = 64 - (msdLeadingZeros + 1 + bitsNotAvailableDueToDigitSize); // 1 for hidden mantissa bit. = 64 - (msdLeadingZeros + 1 + (64 - digitBits)) = 64 - shiftAmount Hence, unsetBits = 64 - (64 - shiftAmount) - 12 = shiftAmount - 12 > Source/JavaScriptCore/runtime/JSBigInt.cpp:2266 > + if (mantissaBitsUnset >= static_cast<int32_t>(digitBits) && digitIndex > 0) { > + digitIndex--; > + currentDigit = static_cast<uint64_t>(bigInt->digit(digitIndex)); > + mantissa |= (currentDigit << (mantissaBitsUnset - digitBits)); > + mantissaBitsUnset -= digitBits; > + } Can you wrap this in a "if constexpr (digitBits < 64)"? This code should be a no-op for 64 bit targets. > Source/JavaScriptCore/runtime/JSBigInt.cpp:2292 > + uint64_t doubleBits = signBit | exponent | mantissa; Can you add ASSERTs here to confirm that we didn't have a bug that results in any of the 3 components exceeding their bit boundaries? ASSERT(doubleBits & (1 << 63) == signBit); ASSERT(doubleBits & (0x7ff << 52) == exponent); ASSERT(doubleBits & ((1 << 52) - 1) == mantissa); > Source/JavaScriptCore/runtime/JSBigInt.h:280 > + static constexpr unsigned doublePhysicalSignificandSize = 52; You're using a mix of "Significand" and "mantissa" in this patch. Can you stick with just one? Maybe call this doublePhysicalMantissaSize instead? > Source/JavaScriptCore/runtime/NumberConstructor.cpp:100 > + numeric = JSBigInt::toNumber(numeric); Can you ASSERT(numeric.isBigInt()) at the top here?
Comment on attachment 397660 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=397660&action=review > JSTests/stress/number-constructor-bigint.js:29 > +// Around Infinity. Can you also add some cases for mantissa rounding both near infinity (may rounD to infinity) and not near infinity?
Comment on attachment 397660 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=397660&action=review r=me > Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:3102 > #if USE(BIGINT32) Let's move this #if up around the "if (node->op() == CallNumberConstructor) {". > Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:3109 > #endif Move below } below.
Comment on attachment 397660 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=397660&action=review >> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:3102 >> #if USE(BIGINT32) > > Let's move this #if up around the "if (node->op() == CallNumberConstructor) {". Done. >> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:3109 >> #endif > > Move below } below. Done >> Source/JavaScriptCore/runtime/JSBigInt.cpp:2256 >> + mantissa >>= 12; // (12 = 64 - 52) (double's mantissa bit count). > > 12 is the non-mantissa bit count, i.e. exponent and sign bits. 52 is the mantissa it count. Fixed. >> Source/JavaScriptCore/runtime/JSBigInt.cpp:2266 >> + } > > Can you wrap this in a "if constexpr (digitBits < 64)"? This code should be a no-op for 64 bit targets. Sounds nice! Fixed. >> Source/JavaScriptCore/runtime/JSBigInt.cpp:2292 >> + uint64_t doubleBits = signBit | exponent | mantissa; > > Can you add ASSERTs here to confirm that we didn't have a bug that results in any of the 3 components exceeding their bit boundaries? > ASSERT(doubleBits & (1 << 63) == signBit); > ASSERT(doubleBits & (0x7ff << 52) == exponent); > ASSERT(doubleBits & ((1 << 52) - 1) == mantissa); Sounds nice, fixed. >> Source/JavaScriptCore/runtime/JSBigInt.h:280 >> + static constexpr unsigned doublePhysicalSignificandSize = 52; > > You're using a mix of "Significand" and "mantissa" in this patch. Can you stick with just one? Maybe call this doublePhysicalMantissaSize instead? Fixed, nice. >> Source/JavaScriptCore/runtime/NumberConstructor.cpp:100 >> + numeric = JSBigInt::toNumber(numeric); > > Can you ASSERT(numeric.isBigInt()) at the top here? Sounds good. Fixed. >> JSTests/stress/number-constructor-bigint.js:29 >> +// Around Infinity. > > Can you also add some cases for mantissa rounding both near infinity (may rounD to infinity) and not near infinity? near infinity thing is already added (`shouldBe(Number(0x3fffffffffffffn << 969n), 8.98846567431158e+307);`), I'll add non-near infinity case.
Comment on attachment 397660 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=397660&action=review >> Source/JavaScriptCore/runtime/JSBigInt.cpp:2257 >> + int32_t mantissaBitsUnset = shiftAmount - 12; > > This equation was unclear to me at first, but after it working it out, I see that it's correct. Can you put the following in a comment her to explain how we derived this equation? > > unsetBits = 64 - setBits - 12 // 12 for non-mantissa bits > setBits = 64 - (msdLeadingZeros + 1 + bitsNotAvailableDueToDigitSize); // 1 for hidden mantissa bit. > = 64 - (msdLeadingZeros + 1 + (64 - digitBits)) > = 64 - shiftAmount > Hence, unsetBits = 64 - (64 - shiftAmount) - 12 = shiftAmount - 12 Added, thanks!
Created attachment 397805 [details] Patch for landing
Committed r260834: <https://trac.webkit.org/changeset/260834>
<rdar://problem/62531125>