Bug 190799 - [ESNext][BigInt] Implement support for "**"
Summary: [ESNext][BigInt] Implement support for "**"
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:
Blocks: 179001
  Show dependency treegraph
 
Reported: 2018-10-22 11:48 PDT by Caio Lima
Modified: 2019-06-03 11:44 PDT (History)
7 users (show)

See Also:


Attachments
WIP - Patch (43.94 KB, patch)
2019-01-07 11:05 PST, Caio Lima
no flags Details | Formatted Diff | Diff
WIP - Patch (43.93 KB, patch)
2019-01-07 13:07 PST, Caio Lima
no flags Details | Formatted Diff | Diff
Benchmarks (98.11 KB, text/plain)
2019-01-07 13:14 PST, Caio Lima
no flags Details
Patch (43.97 KB, patch)
2019-01-24 16:52 PST, Caio Lima
no flags Details | Formatted Diff | Diff
Patch (43.99 KB, patch)
2019-01-25 04:08 PST, Caio Lima
no flags Details | Formatted Diff | Diff
Benchmarks (97.02 KB, text/plain)
2019-01-25 04:32 PST, Caio Lima
no flags Details
Patch (44.83 KB, patch)
2019-02-17 15:56 PST, Caio Lima
no flags Details | Formatted Diff | Diff
Patch (48.63 KB, patch)
2019-03-11 05:56 PDT, Caio Lima
no flags Details | Formatted Diff | Diff
Patch (48.58 KB, patch)
2019-05-06 06:34 PDT, Caio Lima
no flags Details | Formatted Diff | Diff
Patch (48.26 KB, patch)
2019-05-26 14:15 PDT, Caio Lima
no flags Details | Formatted Diff | Diff
Patch (48.23 KB, patch)
2019-05-27 13:34 PDT, Caio Lima
no flags Details | Formatted Diff | Diff
Patch (48.23 KB, patch)
2019-06-03 10:52 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-10-22 11:48:31 PDT
...
Comment 1 Caio Lima 2019-01-07 11:05:33 PST
Created attachment 358509 [details]
WIP - Patch
Comment 2 Caio Lima 2019-01-07 13:07:31 PST
Created attachment 358518 [details]
WIP - Patch
Comment 3 Caio Lima 2019-01-07 13:14:02 PST
Created attachment 358520 [details]
Benchmarks

This Patch is perf neutral in x86_64
Comment 4 Caio Lima 2019-01-24 16:52:34 PST
Created attachment 360053 [details]
Patch
Comment 5 Caio Lima 2019-01-25 04:08:31 PST
Created attachment 360103 [details]
Patch
Comment 6 Caio Lima 2019-01-25 04:32:06 PST
Created attachment 360105 [details]
Benchmarks

The patch is perf neutral.
Comment 7 Caio Lima 2019-02-17 15:56:14 PST
Created attachment 362260 [details]
Patch
Comment 8 Caio Lima 2019-03-11 05:56:44 PDT
Created attachment 364248 [details]
Patch
Comment 9 Caio Lima 2019-05-06 06:34:10 PDT
Created attachment 369121 [details]
Patch
Comment 10 Saam Barati 2019-05-06 13:14:44 PDT
Comment on attachment 369121 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=369121&action=review

LGTM overall. Some bugs I believe that are similar to ValueMod patch

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:862
> +        if (childX && childY && childX.isNumber() && childY.isNumber()) {
> +            if (!node->isBinaryUseKind(BigIntUse))
> +                didFoldClobberWorld();
> +            setConstant(node, jsDoubleNumber(operationMathPow(childX.asNumber(), childY.asNumber())));
> +            break;
> +        }

same comment as I had in ValueMod patch. I believe you need to ensure that the types of X and Y are valid given BigInt.

> Source/JavaScriptCore/dfg/DFGOperations.cpp:545
> +    if (WTF::holds_alternative<JSBigInt*>(leftNumeric) || WTF::holds_alternative<JSBigInt*>(rightNumeric)) {
> +        if (WTF::holds_alternative<JSBigInt*>(leftNumeric) && WTF::holds_alternative<JSBigInt*>(rightNumeric))
> +            RELEASE_AND_RETURN(scope, JSValue::encode(JSBigInt::exponentiate(exec, WTF::get<JSBigInt*>(leftNumeric), WTF::get<JSBigInt*>(rightNumeric))));

Not related to this patch, but maybe we can make this type of pattern easier to read since it's so common in all the operations. Maybe this can be a followup. But if we had something like:
bool doBigIntMath(variant a, variant b, doMath lambda, throw lambda or throw message)

It returns false if both are *not* BigInt. Returns true otherwise. Throws an exception if 1 of 2 are big int, otherwise, returns lambda
Comment 11 Caio Lima 2019-05-26 14:15:39 PDT
Created attachment 370657 [details]
Patch
Comment 12 Caio Lima 2019-05-27 13:34:30 PDT
Created attachment 370705 [details]
Patch
Comment 13 Saam Barati 2019-05-31 18:47:47 PDT
Comment on attachment 370705 [details]
Patch

r=me
Comment 14 Caio Lima 2019-06-03 10:52:38 PDT
Created attachment 371195 [details]
Patch
Comment 15 Caio Lima 2019-06-03 11:03:34 PDT
(In reply to Saam Barati from comment #13)
> Comment on attachment 370705 [details]
> Patch
> 
> r=me

Thx for the review!
Comment 16 WebKit Commit Bot 2019-06-03 11:42:41 PDT
Comment on attachment 371195 [details]
Patch

Clearing flags on attachment: 371195

Committed r246041: <https://trac.webkit.org/changeset/246041>
Comment 17 WebKit Commit Bot 2019-06-03 11:42:42 PDT
All reviewed patches have been landed.  Closing bug.
Comment 18 Radar WebKit Bug Importer 2019-06-03 11:44:45 PDT
<rdar://problem/51361986>