[webkit-reviews] review denied: [Bug 186177] [BigInt] Add support to BigInt into ValueAdd : [Attachment 351908] Patch

[bugzilla-daemon at webkit.org]

Yusuke Suzuki <yusukesuzuki at slowstart.org> has denied Caio Lima
<ticaiolima at gmail.com>'s request for review:
Bug 186177: [BigInt] Add support to BigInt into ValueAdd
https://bugs.webkit.org/show_bug.cgi?id=186177

Attachment 351908: Patch

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




--- Comment #10 from Yusuke Suzuki <yusukesuzuki at slowstart.org> ---
Comment on attachment 351908
  --> https://bugs.webkit.org/attachment.cgi?id=351908
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.