[webkit-reviews] review denied: [Bug 186228] [ESNext][BigInt] Implement support for "&" : [Attachment 350532] 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 186228: [ESNext][BigInt] Implement support for "&"
https://bugs.webkit.org/show_bug.cgi?id=186228

Attachment 350532: Patch

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




--- Comment #34 from Yusuke Suzuki <yusukesuzuki at slowstart.org> ---
Comment on attachment 350532
  --> https://bugs.webkit.org/attachment.cgi?id=350532
Patch

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

Looks nice. I would like to have one more round of reviews.

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:4848
> +	       if (getPredictionWithoutOSRExit() != SpecBigInt)

If `type = SpecBigInt | SpecInt32Only`, it goes to `ArithBitAnd`. It is not
desirable result.
We should use `isInt32Speculation(getPredictionWithoutOSRExit())` =>
ArithBitAnd.

> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:103
> +	       if (Node::shouldSpeculateBigInt(node->child1().node(),
node->child1().node())) {

Both operands use `child1()`.

> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:121
> +	       if (Node::shouldSpeculateUntypedForBitOps(node->child1().node(),
node->child2().node())) {
> +		   fixEdge<UntypedUse>(node->child1());
> +		   fixEdge<UntypedUse>(node->child2());
> +		   node->setOp(ValueBitAnd);
> +		   node->setResult(NodeResultJS);
> +		   break;
> +	       }

If the type is `ValueBitAnd`, it should be `NodeMustBeGenerate`. You need to
mark it as `NodeMustBeGenerate`. You can use
`setOpAndDefaultFlags(ValueBitAnd)` instead of setOp(ValueBitAnd) and
setResult(NodeResultJS);.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3613
> +    GPRTemporary result(this);

Remove this.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3626
> +    silentSpillAllRegisters(resultGPR);
> +    callOperation(operationBitAndBigInt, resultGPR, leftGPR, rightGPR);
> +    silentFillAllRegisters(); 
> +    m_jit.exceptionCheck();
> +
> +    cellResult(resultGPR, node);

In such case, you can use,

flushRegisters();
GPRFlushedCallResult result(this);
GPRReg resultGPR = result.gpr();
callOperation(operationBitAndBigInt, resultGPR, leftGPR, rightGPR);
m_jit.exceptionCheck();
cellResult(resultGPR, node);

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:655
> +	   case DFG::ArithBitAnd:

Can we just use `ArithBitAnd` here? (b/c it does not conflict with B3 BitAnd).

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:2821
> +	   

Remove this line.

> Source/JavaScriptCore/runtime/JSBigInt.cpp:98
> +    bigInt->setSign(false);

This should be done in JSBigInt::JSBigInt constructor.

, m_sign(false)

> Source/JavaScriptCore/runtime/JSCJSValueInlines.h:756
> +    if (isInt32())
> +	   return asInt32();

What happens if the `this` is double and it is in range of int32? (e.g. 1.0).
I think we can have `if (isDouble())` path too. `canBeInt32` function can be
used for this case.