[webkit-reviews] review denied: [Bug 186178] [BigInt] Add ValueDiv into DFG : [Attachment 357118] 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 186178: [BigInt] Add ValueDiv into DFG
https://bugs.webkit.org/show_bug.cgi?id=186178

Attachment 357118: Patch

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




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

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

Looks nice, but I think prediction propagation phase needs to be revised.

> Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp:371
> +		   } else if (isBigIntSpeculation(left) &&
isBigIntSpeculation(right))

While ValueMul/ArithMul checks `op == ValueMul`, we don't do that in ValueDiv.
Any reason?
If there is no reason, I think this should be consistent.

> Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp:375
> +		       if (node->mayHaveBigIntResult())

Why is this code different from ValueMul's one?

349349			  if ((op == ValueMul && node->mayHaveBigIntResult())
350350			      || (left & SpecBigInt)
351351			      || (right & SpecBigInt))
352352			      changed |= mergePrediction(SpecBigInt);


I think we should have consistent prediction rule.
ArithDiv cannot return SpecBigInt. And we do not convert ArithDiv to ValueDiv
in the later phases. So, the prediction should not include SpecBigInt. Is it
correct?

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:1881
> +	       LValue result = vmCall(Int64,
m_out.operation(operationAddBigInt), m_callFrame, left, right);

Ditto.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:1900
> +	       LValue result = vmCall(Int64,
m_out.operation(operationSubBigInt), m_callFrame, left, right);

Ditto.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:2292
> +	       LValue result = vmCall(Int64,
m_out.operation(operationDivBigInt), m_callFrame, left, right);

Ditto.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:-2870
> -	       LValue result = vmCall(pointerType(),
m_out.operation(operationValueBitNot), m_callFrame, operand);

pointerType() is better since operationValueBitNot returns pointer.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:-2884
> -	       LValue result = vmCall(pointerType(),
m_out.operation(operationBitAndBigInt), m_callFrame, left, right);

Ditto.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:-2903
> -	       LValue result = vmCall(pointerType(),
m_out.operation(operationBitOrBigInt), m_callFrame, left, right);

Ditto.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:-2922
> -	       LValue result = vmCall(pointerType(),
m_out.operation(operationBitXorBigInt), m_callFrame, left, right);

Ditto.