[webkit-reviews] review denied: [Bug 182214] [ESNext][BigInt] Implement "+" and "-" unary operation : [Attachment 340263] Patch

[bugzilla-daemon at webkit.org]

Yusuke Suzuki <utatane.tea at gmail.com> has denied Caio Lima
<ticaiolima at gmail.com>'s request for review:
Bug 182214: [ESNext][BigInt] Implement "+" and "-" unary operation
https://bugs.webkit.org/show_bug.cgi?id=182214

Attachment 340263: Patch

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




--- Comment #40 from Yusuke Suzuki <utatane.tea at gmail.com> ---
Comment on attachment 340263
  --> https://bugs.webkit.org/attachment.cgi?id=340263
Patch

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

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:766
> +	   forNode(node).setType(m_graph, SpecBytecodeNumber | SpecBigInt);

Use setTypeForNode(node, SpecBytecodeNumber | SpecBigInt);

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:770
>      case ArithNegate: {

Should not have ArithNegate(Untyped) case. It is incorrect if the input is
BigInt. It should be handled by ValueAdd. Let's add RELEASE_ASSERT_NOT_REACHED
for "default" clause.

> Source/JavaScriptCore/dfg/DFGBackwardsPropagationPhase.cpp:310
> -	       
> +

Remove this change

> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:163
> +	       if (node->child1()->shouldSpeculateInt32OrBoolean() &&
node->canSpeculateInt32(FixupPass)) {
> +		   node->setOp(ArithNegate);
> +		   fixIntOrBooleanEdge(node->child1());
> +		   if (bytecodeCanTruncateInteger(node->arithNodeFlags()))
> +		       node->setArithMode(Arith::Unchecked);
> +		   else if
(bytecodeCanIgnoreNegativeZero(node->arithNodeFlags()))
> +		       node->setArithMode(Arith::CheckOverflow);
> +		   else
> +		       node->setArithMode(Arith::CheckOverflowAndNegativeZero);
> +		   node->setResult(NodeResultInt32);
> +		   node->clearFlags(NodeMustGenerate);
> +		   break;
> +	       }

Nice.

> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:186
> +	   }

And let's fix ArithNegate case too. ArithNegate can produce
ArithNegate(Untyped). But it is wrong... ArithNegate(BigInt) would produce
BigInt, but ArithNegate node type claims that it always produces numbers.
So, we should remove ArithNegate(Untyped) case, and this should be handled
ValueNegate(Untyped). This is the same strategy to ArithAdd and ValueAdd.
We should fixup edges with some number filters in ArithNegate. If int fixup
(Int32, Int52 etc.) is failed in ArithNegate, we should always call
fixDoubleOrBooleanEdge(node->child1()) to emit edge filter.
This is OK since bytecode parser emits ArithNegate only for operands which
result is number. Original ArithNegate(Untyped) thing should be done by
ValueNegate.

> Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp:258
>		   else {

We should have BigInt speculation type, edge filter, and speculation check
here. Let's add FIXME and url.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:4533
> +    return;

This "return" is unnecessary.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:4536
>  void SpeculativeJIT::compileArithNegate(Node* node)

Why does ArithNegate have ArithNegate(Untyped)? Can we use ValueNegate for that
case?
Since ArithNegate's result type should be number, we cannot take BigInt for
that. So, current ArithNegate(Untyped) is wrong with BigInt.

ArithAdd does not have ArithAdd(Untyped, Untyped). Instead it uses
ValueAdd(Untyped, Untyped) for that. Since ArithNegate is emitted in bytecode
parser if the operand has NumberResult, and we only convert ValueAdd to
ArithAdd with number related edge filtering, we can remove ArithNegate(Untyped)
case if we carefully change fixup etc.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:2720
>      void compileArithNegate()

Ditto. Remove ArithNegate(Untyped) case. And please ensure ArithAdd(Untyped) is
not produced.

> Source/JavaScriptCore/runtime/JSBigInt.cpp:253
> +JSBigInt* JSBigInt::copy(ExecState* state, JSBigInt* x)

Take VM& instead of ExecState*.

> Source/JavaScriptCore/runtime/JSBigInt.cpp:264
> +JSBigInt* JSBigInt::unaryMinus(ExecState* state, JSBigInt* x)

Take VM& instead of ExecState*.

> Source/JavaScriptCore/runtime/JSBigInt.h:81
> +    enum ParseIntMode { DisallowEmptyString, AllowEmptyString };
> +    enum ParseIntSign { Unsigned, Signed };

Use enum class.