[BigInt] Add support to BigInt into ValueAdd

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Caio Lima]

...

[Caio Lima]

Created attachment 351418 [details]
WIP - RFC Patch

[Caio Lima]

Comment on attachment 351418 [details]
WIP - RFC Patch

Patch not ready yet.

[Caio Lima]

@dinfuehr, @caitp, @xan and @guijemont could you take a look in this Patch before sending to review?

[Caio Lima]

Created attachment 351721 [details]
WIP - RFC Patch

[Caio Lima]

Created attachment 351908 [details]
Patch

[Caio Lima]

Created attachment 351921 [details]
Benchmarks

These changes are perf neutral.

[Caio Lima]

Ping Review

[Caio Lima]

Ping Review

[Caio Lima]

Ping Review

[Yusuke Suzuki]

Comment on attachment 351908 [details]
Patch

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

I think our ArithProfile for `+` needs additional changes.

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:594
> +        ASSERT(node->binaryUseKind() == UntypedUse || node->binaryUseKind() == BigIntUse);

Use DFG_ASSERT instead.

> Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp:203
>                      changed |= mergePrediction(SpecInt32Only);
>                      if (node->mayHaveDoubleResult())
>                          changed |= mergePrediction(SpecBytecodeDouble);

Let's consider the polymorphic `+` site. It takes Number and BigInt. It can be like, `function add(a, b) { return a + b; }` and it is used in various places as a callback. Some functions pass Number, and some pass BigInt.
In this case, ArithProfile says "Result includes non number". And this prediction says "String | Number". But it is not correct in this case. It should say "Number | BigInt".
So, at least for `+`, I think ArithProfile needs to record additional result type information.

[Caio Lima]

Comment on attachment 351908 [details]
Patch

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

Thx for the review!

>> Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp:203
>>                          changed |= mergePrediction(SpecBytecodeDouble);
> 
> Let's consider the polymorphic `+` site. It takes Number and BigInt. It can be like, `function add(a, b) { return a + b; }` and it is used in various places as a callback. Some functions pass Number, and some pass BigInt.
> In this case, ArithProfile says "Result includes non number". And this prediction says "String | Number". But it is not correct in this case. It should say "Number | BigInt".
> So, at least for `+`, I think ArithProfile needs to record additional result type information.

Oops. Sorry I missed that. Nice Catch.

[Caio Lima]

Created attachment 352640 [details]
Patch

[Caio Lima]

Created attachment 352648 [details]
Patch

[EWS Watchlist]

Comment on attachment 352648 [details]
Patch

Attachment 352648 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/9645190

New failing tests:
stress/big-int-negate-jit.js.big-int-enabled
apiTests

[Yusuke Suzuki]

Comment on attachment 352648 [details]
Patch

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

Looks nice. But since I have found several issues, I would like to have another round of review.

> Source/JavaScriptCore/bytecode/ArithProfile.cpp:41
>      CCallHelpers::Jump isInt32 = jit.branchIfInt32(regs, mode);

`done.append(jit.branchIfInt32(regs, mode))` seems simple.

> Source/JavaScriptCore/bytecode/ArithProfile.cpp:47
> +    CCallHelpers::Jump notBigInt = jit.branchIfNotBigInt(regs.payloadGPR());

It assumes the regs are JSCell, but it seems flaky. Putting branchIfNotCell before this is better.
At that time, please pass `mode` to branchIfNotCell since we may not have tag register.

> Source/JavaScriptCore/bytecode/ArithProfile.h:274
>      void emitSetNonNumber(CCallHelpers&) const;

I think renaming this term "NonNumber" in ArithProfile, DFG::Node flags etc. to "NonNumeric" is better since Numeric can include BigInt.

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:926
> +                        node->mergeFlags(NodeMayHaveBigIntResult);

I think ValueNegate and ArithNegate needs this flag too.

> Source/JavaScriptCore/dfg/DFGNodeFlags.h:75
> +#define NodeMayHaveBigIntResult         0x100000

I think this flag should not be sparse: this should be cooperating with the other flags like NodeMayHaveNonNumberResult, NodeMayHaveDoubleResult etc.
The value of this flag should be close to NodeMayHaveNonNumberResult. And NodeMayHaveNonIntResult should be updated since BigInt is NonInt result.

> Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp:274
>                          // FIXME: We should add support to BigInt into speculatio
>                          // https://bugs.webkit.org/show_bug.cgi?id=182470

This can be removed.

[Caio Lima]

Created attachment 352873 [details]
Patch

[Caio Lima]

Comment on attachment 352648 [details]
Patch

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

Thx for the review!

>> Source/JavaScriptCore/bytecode/ArithProfile.cpp:47
>> +    CCallHelpers::Jump notBigInt = jit.branchIfNotBigInt(regs.payloadGPR());
> 
> It assumes the regs are JSCell, but it seems flaky. Putting branchIfNotCell before this is better.
> At that time, please pass `mode` to branchIfNotCell since we may not have tag register.

Done. BTW, I'm trying to create a test for such case, but I'm stuck to find a case where this code is emitted. Do you know when we emit such code? I tried OSR code generated due BadType and Overflow and neither one triggers the generation of this code.

>> Source/JavaScriptCore/bytecode/ArithProfile.h:274
>>      void emitSetNonNumber(CCallHelpers&) const;
> 
> I think renaming this term "NonNumber" in ArithProfile, DFG::Node flags etc. to "NonNumeric" is better since Numeric can include BigInt.

I agree. Done.

>> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:926
>> +                        node->mergeFlags(NodeMayHaveBigIntResult);
> 
> I think ValueNegate and ArithNegate needs this flag too.

Done.

>> Source/JavaScriptCore/dfg/DFGNodeFlags.h:75
>> +#define NodeMayHaveBigIntResult         0x100000
> 
> I think this flag should not be sparse: this should be cooperating with the other flags like NodeMayHaveNonNumberResult, NodeMayHaveDoubleResult etc.
> The value of this flag should be close to NodeMayHaveNonNumberResult. And NodeMayHaveNonIntResult should be updated since BigInt is NonInt result.

Makes sense. I updated it.

[Caio Lima]

Created attachment 352874 [details]
Benchmarks

The performance is perf neutral according to results.

[Caio Lima]

Created attachment 353619 [details]
Patch

[Keith Miller]

Comment on attachment 353619 [details]
Patch

r=me.

[Caio Lima]

@Yusuke and  @Keith thank you very much for the review!

[WebKit Commit Bot]

Comment on attachment 353619 [details]
Patch

Clearing flags on attachment: 353619

Committed r237972: <https://trac.webkit.org/changeset/237972>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/45896441>