Summary: | [BigInt] Add ValueDiv into DFG | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Caio Lima <ticaiolima> | ||||||||||||
Component: | JavaScriptCore | Assignee: | Caio Lima <ticaiolima> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | cdumez, commit-queue, ews-watchlist, keith_miller, mark.lam, msaboff, rniwa, saam, webkit-bug-importer, ysuzuki | ||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Bug Depends on: | |||||||||||||||
Bug Blocks: | 186173 | ||||||||||||||
Attachments: |
|
Description
Caio Lima
2018-05-31 19:14:20 PDT
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. |