Bug 184327

Created attachment 337577 [details] WIP - Patch It starts.

Created attachment 341057 [details] Patch

Created attachment 341487 [details] Patch

Comment on attachment 341487 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=341487&action=review r=me with comments. > Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:572 > + JSValue result(JSBigInt::remainder(exec, WTF::get<JSBigInt*>(leftNumeric), WTF::get<JSBigInt*>(rightNumeric))); Keep the narrower type (JSBigInt rather than JSValue). JSBigInt* result = JSBigInt::remainder(exec, WTF::get<JSBigInt*>(leftNumeric), WTF::get<JSBigInt*>(rightNumeric))); > Source/JavaScriptCore/runtime/JSBigInt.cpp:1018 > if (isZero()) > return this; Typically, we call `setSign()` and `rightTrim()`. But what prevents us from creating `negative zero`? Please review the use of rightTrim(), and if we have a bug, could you fix it with a test? > JSTests/ChangeLog:14 > + * stress/big-int-mod-memory-stress.js: Added. > + * stress/big-int-mod-to-primitive-precedence.js: Added. > + * stress/big-int-mod-to-primitive.js: Added. > + * stress/big-int-mod-type-error.js: Added. > + * stress/big-int-mod-wrapped-value.js: Added. > + * stress/big-int-mod.js: Added. Add tests involving JIT tiers. Especially, prevent constant folding by using `noInline` and arguments.

Thank you! (In reply to Yusuke Suzuki from comment #4) > Comment on attachment 341487 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=341487&action=review > > r=me with comments. > > > Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:572 > > + JSValue result(JSBigInt::remainder(exec, WTF::get<JSBigInt*>(leftNumeric), WTF::get<JSBigInt*>(rightNumeric))); > > Keep the narrower type (JSBigInt rather than JSValue). Sounds good. > JSBigInt* result = JSBigInt::remainder(exec, > WTF::get<JSBigInt*>(leftNumeric), WTF::get<JSBigInt*>(rightNumeric))); > > > Source/JavaScriptCore/runtime/JSBigInt.cpp:1018 > > if (isZero()) > > return this; > > Typically, we call `setSign()` and `rightTrim()`. But what prevents us from > creating `negative zero`? > Please review the use of rightTrim(), and if we have a bug, could you fix it > with a test? The test below will check leading zero digits. If the number is zero, then we will fallback into ```if (nonZeroIndex < 0)``` that will return the expected zero representation. This is the first operation that actually can result in negative zero before rightTrim, since all other previous operation have fast patches when some of the operands are zero. I will add a test cases for that, because I can probably missing some edge case. > > JSTests/ChangeLog:14 > > + * stress/big-int-mod-memory-stress.js: Added. > > + * stress/big-int-mod-to-primitive-precedence.js: Added. > > + * stress/big-int-mod-to-primitive.js: Added. > > + * stress/big-int-mod-type-error.js: Added. > > + * stress/big-int-mod-wrapped-value.js: Added. > > + * stress/big-int-mod.js: Added. > > Add tests involving JIT tiers. Especially, prevent constant folding by using > `noInline` and arguments. Oops. I forgot them. Thx!

Created attachment 341544 [details] Patch for landing @Yusuke, could you confirm if you think this JIT test case is what you are thinking about? I also added test to verify the rightTrim changes.

Comment on attachment 341544 [details] Patch for landing r=me

Comment on attachment 341544 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=341544&action=review > Source/JavaScriptCore/runtime/JSBigInt.cpp:1034 > if (isZero()) > return this; Add `ASSERT(!sign());` in this if-clause.

Created attachment 341566 [details] Patch for landing

(In reply to Yusuke Suzuki from comment #8) > Comment on attachment 341544 [details] > Patch for landing > > View in context: > https://bugs.webkit.org/attachment.cgi?id=341544&action=review > > > Source/JavaScriptCore/runtime/JSBigInt.cpp:1034 > > if (isZero()) > > return this; > > Add `ASSERT(!sign());` in this if-clause. Done.

Comment on attachment 341566 [details] Patch for landing Rejecting attachment 341566 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'validate-changelog', '--check-oops', '--non-interactive', 341566, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit ChangeLog entry in JSTests/ChangeLog contains OOPS!. Full output: http://webkit-queues.webkit.org/results/7865356

Created attachment 341567 [details] Patch for landing

Created attachment 341568 [details] Patch for landing

Comment on attachment 341568 [details] Patch for landing Clearing flags on attachment: 341568 Committed r232295: <https://trac.webkit.org/changeset/232295>

All reviewed patches have been landed. Closing bug.

<rdar://problem/40652656>

Remember DFG ArithMod only returns Number. We need to extend this to ValueMod approach too.