Bug 186178

Created attachment 356320 [details] WIP - Patch

Created attachment 357118 [details] Patch

Created attachment 357119 [details] Benchmarks These changes are perf neutral into x86_64, according this benchmark report.

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

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

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.

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.

Created attachment 357171 [details] Patch

Comment on attachment 357171 [details] Patch r=me

Comment on attachment 357171 [details] Patch Thank you very much for the review!

Comment on attachment 357171 [details] Patch Clearing flags on attachment: 357171 Committed r239158: <https://trac.webkit.org/changeset/239158>

All reviewed patches have been landed. Closing bug.

<rdar://problem/46694295>