Bug 184327 – [ESNext][BigInt] Implement support for "%" operation

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Caio Lima]

...

[Caio Lima]

Created attachment 337577 [details]
WIP - Patch

It starts.

[Caio Lima]

Created attachment 341057 [details]
Patch

[Caio Lima]

Created attachment 341487 [details]
Patch

[Yusuke Suzuki]

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]

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]

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 on attachment 341544 [details]
Patch for landing

r=me

[Yusuke Suzuki]

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]

Created attachment 341566 [details]
Patch for landing

[Caio Lima]

(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 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]

Created attachment 341567 [details]
Patch for landing

[Caio Lima]

Created attachment 341568 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 341568 [details]
Patch for landing

Clearing flags on attachment: 341568

Committed r232295: <https://trac.webkit.org/changeset/232295>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/40652656>

[Yusuke Suzuki]

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