Bug 186175

Summary: [BigInt] Add ValueMul 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
WIP - Patch
none
WIP - Patch
none
WIP - Patch
none
Archive of layout-test-results from ews100 for mac-sierra
none
WIP - Patch
none
Patch
none
Benchmarks results
none
Patch
none
Patch none

Caio Lima
Reported 2018-05-31 19:09:34 PDT
...
Attachments
WIP - Patch (25.92 KB, patch)
2018-11-20 09:37 PST, Caio Lima 		no flags
DetailsFormatted DiffDiff
WIP - Patch (30.30 KB, patch)
2018-11-28 17:25 PST, Caio Lima 		no flags
DetailsFormatted DiffDiff
WIP - Patch (30.32 KB, patch)
2018-11-29 04:20 PST, Caio Lima 		no flags
DetailsFormatted DiffDiff
WIP - Patch (30.76 KB, patch)
2018-12-04 16:31 PST, Caio Lima 		no flags
DetailsFormatted DiffDiff
Archive of layout-test-results from ews100 for mac-sierra (2.47 MB, application/zip)
2018-12-05 00:42 PST, EWS Watchlist 		no flags
Details
WIP - Patch (33.31 KB, patch)
2018-12-06 05:35 PST, Caio Lima 		no flags
DetailsFormatted DiffDiff
Patch (35.54 KB, patch)
2018-12-08 04:56 PST, Caio Lima 		no flags
DetailsFormatted DiffDiff
Benchmarks results (96.59 KB, text/plain)
2018-12-10 12:22 PST, Caio Lima 		no flags
Details
Patch (35.48 KB, patch)
2018-12-10 12:24 PST, Caio Lima 		no flags
DetailsFormatted DiffDiff
Patch (35.48 KB, patch)
2018-12-10 12:42 PST, Caio Lima 		no flags
DetailsFormatted DiffDiff
Show Obsolete (8) View All Add attachment proposed patch, testcase, etc.
Caio Lima
Comment 1 2018-11-20 09:37:07 PST
Created attachment 355348 [details] WIP - Patch
Caio Lima
Comment 2 2018-11-28 17:25:00 PST
Created attachment 355948 [details] WIP - Patch
Caio Lima
Comment 3 2018-11-29 04:20:43 PST
Created attachment 356002 [details] WIP - Patch
Caio Lima
Comment 4 2018-12-04 16:31:01 PST
Created attachment 356551 [details] WIP - Patch
EWS Watchlist
Comment 5 2018-12-05 00:42:13 PST
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
Comment 6 2018-12-05 00:42:15 PST
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
Comment 7 2018-12-06 05:35:24 PST
Created attachment 356725 [details] WIP - Patch
Caio Lima
Comment 8 2018-12-08 04:56:03 PST
Created attachment 356873 [details] Patch
Yusuke Suzuki
Comment 9 2018-12-10 02:09:08 PST
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 10 2018-12-10 12:03:37 PST
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
Comment 11 2018-12-10 12:22:26 PST
Created attachment 356979 [details] Benchmarks results Benchmark is perf neutral into x86_64.
Caio Lima
Comment 12 2018-12-10 12:24:54 PST
Created attachment 356981 [details] Patch
Caio Lima
Comment 13 2018-12-10 12:42:37 PST
Created attachment 356984 [details] Patch
WebKit Commit Bot
Comment 14 2018-12-10 13:11:52 PST
Comment on attachment 356984 [details] Patch Clearing flags on attachment: 356984 Committed r239045: <https://trac.webkit.org/changeset/239045>
WebKit Commit Bot
Comment 15 2018-12-10 13:11:53 PST
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 16 2018-12-10 13:12:29 PST
<rdar://problem/46605762>
Saam Barati
Comment 17 2018-12-14 09:34:39 PST
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 18 2018-12-14 16:16:25 PST
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
Note You need to log in before you can comment on or make changes to this bug.