| Summary: | [BigInt] Add ValueMul 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:09:34 PDT
Created attachment 355348 [details]
WIP - Patch
Created attachment 355948 [details]
WIP - Patch
Created attachment 356002 [details]
WIP - Patch
Created attachment 356551 [details]
WIP - Patch
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 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
Created attachment 356725 [details]
WIP - Patch
Created attachment 356873 [details]
Patch
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. 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. Created attachment 356979 [details]
Benchmarks results
Benchmark is perf neutral into x86_64.
Created attachment 356981 [details]
Patch
Created attachment 356984 [details]
Patch
Comment on attachment 356984 [details] Patch Clearing flags on attachment: 356984 Committed r239045: <https://trac.webkit.org/changeset/239045> All reviewed patches have been landed. Closing bug. 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 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 |