Bug 186175 – [BigInt] Add ValueMul into DFG

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

[Caio Lima]

...

[Caio Lima]

Created attachment 355348 [details]
WIP - Patch

[Caio Lima]

Created attachment 355948 [details]
WIP - Patch

[Caio Lima]

Created attachment 356002 [details]
WIP - Patch

[Caio Lima]

Created attachment 356551 [details]
WIP - Patch

[EWS Watchlist]

Comment on attachment 356551 [details]
WIP - Patch

Attachment 356551 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/10275208

New failing tests:
http/tests/misc/resource-timing-resolution.html

[EWS Watchlist]

Created attachment 356590 [details]
Archive of layout-test-results from ews100 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Caio Lima]

Created attachment 356725 [details]
WIP - Patch

[Caio Lima]

Created attachment 356873 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 356873 [details]
Patch

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

r=me with nits

> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:79
> +    void fixupMultiplication(Node* node, Edge& leftChild, Edge& rightChild)

I prefer the name `fixupArithMul`, since it is only used for ArithMul.

> Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp:344
> +                    mergePrediction(SpecInt32Only);

`change |= mergePrediction(SpecInt32Only)` is required.

[Caio Lima]

Comment on attachment 356873 [details]
Patch

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

Thank you very much for the review!

>> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:79
>> +    void fixupMultiplication(Node* node, Edge& leftChild, Edge& rightChild)
> 
> I prefer the name `fixupArithMul`, since it is only used for ArithMul.

Better name. Changed.

>> Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp:344
>> +                    mergePrediction(SpecInt32Only);
> 
> `change |= mergePrediction(SpecInt32Only)` is required.

Oops. Nice catch.

[Caio Lima]

Created attachment 356979 [details]
Benchmarks results

Benchmark is perf neutral into x86_64.

[Caio Lima]

Created attachment 356981 [details]
Patch

[Caio Lima]

Created attachment 356984 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 356984 [details]
Patch

Clearing flags on attachment: 356984

Committed r239045: <https://trac.webkit.org/changeset/239045>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/46605762>

[Saam Barati]

Comment on attachment 356984 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:858
> +        clobberWorld();

Why do we clobberWorld if we speculate BigInt?

> Source/JavaScriptCore/dfg/DFGClobberize.h:653
> +    case ValueMul:

I think we can define a CSE rule for this when speculating BigInt

[Caio Lima]

Comment on attachment 356984 [details]
Patch

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

>> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:858
>> +        clobberWorld();
> 
> Why do we clobberWorld if we speculate BigInt?

I implemented thinking in being conservative, but I think I was very pessimistic about SpecBigInt. I think we should support constant propagation/folding and CSE as well, so we can change such rules. IIRC, I'm following the same thing into all other ValueArith nodes.

>> Source/JavaScriptCore/dfg/DFGClobberize.h:653
>> +    case ValueMul:
> 
> I think we can define a CSE rule for this when speculating BigInt

I agree. Created the following bug: https://bugs.webkit.org/show_bug.cgi?id=192723