Bug 186178

Summary: [BigInt] Add ValueDiv into DFG
Product: WebKit Reporter: Caio Lima <ticaiolima>
Component: JavaScriptCoreAssignee: 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 Flags
WIP - Patch
none
Patch
none
Benchmarks
none
Archive of layout-test-results from ews124 for ios-simulator-wk2
none
Patch none

Description Caio Lima 2018-05-31 19:14:20 PDT
...
Comment 1 Caio Lima 2018-12-01 13:08:41 PST
Created attachment 356320 [details]
WIP - Patch
Comment 2 Caio Lima 2018-12-12 05:57:21 PST
Created attachment 357118 [details]
Patch
Comment 3 Caio Lima 2018-12-12 06:02:55 PST
Created attachment 357119 [details]
Benchmarks

These changes are perf neutral into x86_64, according this benchmark report.
Comment 4 EWS Watchlist 2018-12-12 07:49:13 PST
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
Comment 5 EWS Watchlist 2018-12-12 07:49:15 PST
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 6 Yusuke Suzuki 2018-12-12 08:12:24 PST
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 7 Caio Lima 2018-12-12 14:56:46 PST
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.
Comment 8 Caio Lima 2018-12-12 15:03:54 PST
Created attachment 357171 [details]
Patch
Comment 9 Yusuke Suzuki 2018-12-12 17:56:16 PST
Comment on attachment 357171 [details]
Patch

r=me
Comment 10 Caio Lima 2018-12-13 04:00:49 PST
Comment on attachment 357171 [details]
Patch

Thank you very much for the review!
Comment 11 WebKit Commit Bot 2018-12-13 04:26:09 PST
Comment on attachment 357171 [details]
Patch

Clearing flags on attachment: 357171

Committed r239158: <https://trac.webkit.org/changeset/239158>
Comment 12 WebKit Commit Bot 2018-12-13 04:26:11 PST
All reviewed patches have been landed.  Closing bug.
Comment 13 Radar WebKit Bug Importer 2018-12-13 04:27:31 PST
<rdar://problem/46694295>