Bug 186178 – [BigInt] Add ValueDiv into DFG

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Caio Lima]

...

[Caio Lima]

Created attachment 356320 [details]
WIP - Patch

[Caio Lima]

Created attachment 357118 [details]
Patch

[Caio Lima]

Created attachment 357119 [details]
Benchmarks

These changes are perf neutral into x86_64, according this benchmark report.

[EWS Watchlist]

Comment on attachment 357118 [details]
Patch

Attachment 357118 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/10367185

New failing tests:
imported/w3c/web-platform-tests/service-workers/service-worker/register-closed-window.https.html

[EWS Watchlist]

Created attachment 357128 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Yusuke Suzuki]

Comment on attachment 357118 [details]
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.

[Caio Lima]

Comment on attachment 357118 [details]
Patch

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

Thx for the review and for catching wrong prediction propagation rules. I fixed them up.

>> 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.

No. I forgot to update this code.

>> 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?

Ditto.

[Caio Lima]

Created attachment 357171 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 357171 [details]
Patch

r=me

[Caio Lima]

Comment on attachment 357171 [details]
Patch

Thank you very much for the review!

[WebKit Commit Bot]

Comment on attachment 357171 [details]
Patch

Clearing flags on attachment: 357171

Committed r239158: <https://trac.webkit.org/changeset/239158>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/46694295>