WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
184327
[ESNext][BigInt] Implement support for "%" operation
https://bugs.webkit.org/show_bug.cgi?id=184327
Summary
[ESNext][BigInt] Implement support for "%" operation
Caio Lima
Reported
2018-04-04 23:55:20 PDT
...
Attachments
WIP - Patch
(22.84 MB, patch)
2018-04-09 19:46 PDT
,
Caio Lima
no flags
Details
Formatted Diff
Diff
Patch
(27.20 KB, patch)
2018-05-22 18:59 PDT
,
Caio Lima
no flags
Details
Formatted Diff
Diff
Patch
(28.62 KB, patch)
2018-05-29 07:31 PDT
,
Caio Lima
ysuzuki
: review+
Details
Formatted Diff
Diff
Patch for landing
(29.21 KB, patch)
2018-05-29 17:41 PDT
,
Caio Lima
ysuzuki
: review+
Details
Formatted Diff
Diff
Patch for landing
(29.35 KB, patch)
2018-05-30 07:17 PDT
,
Caio Lima
commit-queue
: commit-queue-
Details
Formatted Diff
Diff
Patch for landing
(29.35 KB, patch)
2018-05-30 07:21 PDT
,
Caio Lima
no flags
Details
Formatted Diff
Diff
Patch for landing
(29.41 KB, patch)
2018-05-30 07:27 PDT
,
Caio Lima
no flags
Details
Formatted Diff
Diff
Show Obsolete
(6)
View All
Add attachment
proposed patch, testcase, etc.
Caio Lima
Comment 1
2018-04-09 19:46:10 PDT
Created
attachment 337577
[details]
WIP - Patch It starts.
Caio Lima
Comment 2
2018-05-22 18:59:53 PDT
Created
attachment 341057
[details]
Patch
Caio Lima
Comment 3
2018-05-29 07:31:28 PDT
Created
attachment 341487
[details]
Patch
Yusuke Suzuki
Comment 4
2018-05-29 11:41:30 PDT
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.
Caio Lima
Comment 5
2018-05-29 14:03:00 PDT
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!
Caio Lima
Comment 6
2018-05-29 17:41:43 PDT
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.
Yusuke Suzuki
Comment 7
2018-05-30 03:18:06 PDT
Comment on
attachment 341544
[details]
Patch for landing r=me
Yusuke Suzuki
Comment 8
2018-05-30 04:54:13 PDT
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.
Caio Lima
Comment 9
2018-05-30 07:17:12 PDT
Created
attachment 341566
[details]
Patch for landing
Caio Lima
Comment 10
2018-05-30 07:17:30 PDT
(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.
WebKit Commit Bot
Comment 11
2018-05-30 07:18:45 PDT
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
Caio Lima
Comment 12
2018-05-30 07:21:07 PDT
Created
attachment 341567
[details]
Patch for landing
Caio Lima
Comment 13
2018-05-30 07:27:49 PDT
Created
attachment 341568
[details]
Patch for landing
WebKit Commit Bot
Comment 14
2018-05-30 09:40:23 PDT
Comment on
attachment 341568
[details]
Patch for landing Clearing flags on attachment: 341568 Committed
r232295
: <
https://trac.webkit.org/changeset/232295
>
WebKit Commit Bot
Comment 15
2018-05-30 09:40:25 PDT
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 16
2018-05-30 09:44:01 PDT
<
rdar://problem/40652656
>
Yusuke Suzuki
Comment 17
2018-05-31 18:51:21 PDT
Remember DFG ArithMod only returns Number. We need to extend this to ValueMod approach too.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug