Bug 184327 - [ESNext][BigInt] Implement support for "%" operation
Summary: [ESNext][BigInt] Implement support for "%" operation
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Caio Lima
URL:
Keywords: InRadar
Depends on: 183996
Blocks: 179001
  Show dependency treegraph
 
Reported: 2018-04-04 23:55 PDT by Caio Lima
Modified: 2018-05-31 18:51 PDT (History)
11 users (show)

See Also:


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
yusukesuzuki: review+
Details | Formatted Diff | Diff
Patch for landing (29.21 KB, patch)
2018-05-29 17:41 PDT, Caio Lima
yusukesuzuki: 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

Note You need to log in before you can comment on or make changes to this bug.
Description Caio Lima 2018-04-04 23:55:20 PDT
...
Comment 1 Caio Lima 2018-04-09 19:46:10 PDT
Created attachment 337577 [details]
WIP - Patch

It starts.
Comment 2 Caio Lima 2018-05-22 18:59:53 PDT
Created attachment 341057 [details]
Patch
Comment 3 Caio Lima 2018-05-29 07:31:28 PDT
Created attachment 341487 [details]
Patch
Comment 4 Yusuke Suzuki 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.
Comment 5 Caio Lima 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!
Comment 6 Caio Lima 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.
Comment 7 Yusuke Suzuki 2018-05-30 03:18:06 PDT
Comment on attachment 341544 [details]
Patch for landing

r=me
Comment 8 Yusuke Suzuki 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.
Comment 9 Caio Lima 2018-05-30 07:17:12 PDT
Created attachment 341566 [details]
Patch for landing
Comment 10 Caio Lima 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.
Comment 11 WebKit Commit Bot 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
Comment 12 Caio Lima 2018-05-30 07:21:07 PDT
Created attachment 341567 [details]
Patch for landing
Comment 13 Caio Lima 2018-05-30 07:27:49 PDT
Created attachment 341568 [details]
Patch for landing
Comment 14 WebKit Commit Bot 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>
Comment 15 WebKit Commit Bot 2018-05-30 09:40:25 PDT
All reviewed patches have been landed.  Closing bug.
Comment 16 Radar WebKit Bug Importer 2018-05-30 09:44:01 PDT
<rdar://problem/40652656>
Comment 17 Yusuke Suzuki 2018-05-31 18:51:21 PDT
Remember DFG ArithMod only returns Number. We need to extend this to ValueMod approach too.