| Summary: | Add a GetIndexMask node and make it an input to GetByVal for array and typed array accesses in DFG SSA | ||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Saam Barati <saam> | ||||||||||||||||||||||||||
| Component: | JavaScriptCore | Assignee: | Saam Barati <saam> | ||||||||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||
| Severity: | Normal | CC: | benjamin, commit-queue, ews-watchlist, fpizlo, ggaren, gskachkov, jfbastien, jlewis3, keith_miller, mark.lam, msaboff, rmorisset, rniwa, ryanhaddad, ticaiolima, webkit-bug-importer, ysuzuki | ||||||||||||||||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||||||||||||
| Bug Depends on: | |||||||||||||||||||||||||||||
| Bug Blocks: | 182676 | ||||||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||||||
|
Description
Saam Barati
2018-02-08 17:39:20 PST
Created attachment 333438 [details]
WIP
It's a start. Probably doesn't build yet.
Created attachment 333440 [details]
WIP
Created attachment 333441 [details]
WIP
Created attachment 333450 [details]
WIP
It turns out having a node that participates in CSE and is varargs is a first. So I needed to add CheckVarargs to support this.
Comment on attachment 333450 [details] WIP Attachment 333450 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/6427525 New failing tests: js/dom/dfg-arguments-alias-activation.html Created attachment 333457 [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
Comment on attachment 333450 [details] WIP Attachment 333450 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/6427536 New failing tests: js/dom/dfg-arguments-alias-activation.html Created attachment 333458 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Comment on attachment 333450 [details] WIP Attachment 333450 [details] did not pass jsc-ews (mac): Output: http://webkit-queues.webkit.org/results/6427489 New failing tests: microbenchmarks/scoped-arguments-possibly-overridden-length.js.no-cjit-validate-phases stress/scoped-then-direct-arguments-get-by-val-in-baseline.js.ftl-no-cjit-no-inline-validate microbenchmarks/try-catch-get-by-val-scoped-arguments.js.dfg-maximal-flush-validate-no-cjit microbenchmarks/try-catch-get-by-val-scoped-arguments.js.ftl-no-cjit-no-put-stack-validate microbenchmarks/scoped-arguments-length.js.default stress/captured-arguments-variable.js.no-ftl stress/captured-arguments-variable.js.default stress/scoped-then-direct-arguments-get-by-val-in-baseline.js.ftl-no-cjit-b3o1 microbenchmarks/try-catch-get-by-val-scoped-arguments.js.ftl-no-cjit-small-pool microbenchmarks/try-catch-get-by-val-scoped-arguments.js.ftl-no-cjit-validate-sampling-profiler stress/have-a-bad-time-with-arguments.js.misc-ftl-no-cjit microbenchmarks/scoped-arguments-overridden-length.js.no-llint microbenchmarks/scoped-arguments-overridden-length.js.no-cjit-validate-phases microbenchmarks/scoped-arguments-length.js.no-llint microbenchmarks/scoped-arguments-possibly-overridden-length.js.no-cjit-collect-continuously microbenchmarks/scoped-arguments-overridden-length.js.ftl-eager-no-cjit microbenchmarks/scoped-arguments-possibly-overridden-length.js.ftl-no-cjit-b3o1 microbenchmarks/scoped-arguments-possibly-overridden-length.js.ftl-no-cjit-no-inline-validate microbenchmarks/scoped-arguments-possibly-overridden-length.js.ftl-eager-no-cjit microbenchmarks/scoped-arguments-overridden-length.js.ftl-eager microbenchmarks/scoped-arguments-length.js.dfg-eager microbenchmarks/scoped-arguments-overridden-length.js.ftl-no-cjit-b3o1 microbenchmarks/scoped-arguments-length.js.no-ftl microbenchmarks/try-catch-get-by-val-scoped-arguments.js.ftl-eager-no-cjit-b3o1 stress/scoped-then-direct-arguments-get-by-val-in-baseline.js.ftl-eager-no-cjit-b3o1 stress/scoped-then-direct-arguments-get-by-val-in-baseline.js.no-cjit-collect-continuously microbenchmarks/scoped-arguments-possibly-overridden-length.js.ftl-no-cjit-no-put-stack-validate microbenchmarks/scoped-arguments-possibly-overridden-length.js.no-llint stress/scoped-then-direct-arguments-get-by-val-in-baseline.js.dfg-maximal-flush-validate-no-cjit microbenchmarks/scoped-arguments-overridden-length.js.ftl-no-cjit-validate-sampling-profiler microbenchmarks/try-catch-get-by-val-scoped-arguments.js.no-cjit-validate-phases stress/captured-arguments-variable.js.dfg-maximal-flush-validate-no-cjit stress/scoped-arguments-array-length.js.ftl-no-cjit-small-pool stress/captured-arguments-variable.js.dfg-eager-no-cjit-validate stress/scoped-arguments-array-length.js.ftl-no-cjit-b3o1 stress/scoped-then-direct-arguments-get-by-val-in-baseline.js.ftl-no-cjit-validate-sampling-profiler stress/scoped-arguments-array-length.js.no-llint stress/scoped-then-direct-arguments-get-by-val-in-baseline.js.dfg-eager-no-cjit-validate microbenchmarks/scoped-arguments-overridden-length.js.no-cjit-collect-continuously stress/captured-arguments-variable.js.ftl-eager-no-cjit stress/scoped-arguments-array-length.js.dfg-eager microbenchmarks/scoped-arguments-possibly-overridden-length.js.ftl-no-cjit-small-pool microbenchmarks/scoped-arguments-length.js.ftl-no-cjit-small-pool stress/captured-arguments-variable.js.no-cjit-collect-continuously microbenchmarks/scoped-arguments-overridden-length.js.ftl-no-cjit-no-put-stack-validate stress/scoped-arguments-array-length.js.dfg-maximal-flush-validate-no-cjit stress/captured-arguments-variable.js.no-cjit-validate-phases microbenchmarks/scoped-arguments-possibly-overridden-length.js.dfg-maximal-flush-validate-no-cjit microbenchmarks/scoped-arguments-overridden-length.js.ftl-no-cjit-no-inline-validate stress/captured-arguments-variable.js.ftl-no-cjit-validate-sampling-profiler microbenchmarks/scoped-arguments-overridden-length.js.dfg-eager microbenchmarks/scoped-arguments-length.js.ftl-no-cjit-b3o1 stress/scoped-arguments-array-length.js.ftl-no-cjit-no-put-stack-validate microbenchmarks/scoped-arguments-possibly-overridden-length.js.no-ftl microbenchmarks/scoped-arguments-possibly-overridden-length.js.ftl-eager-no-cjit-b3o1 microbenchmarks/scoped-arguments-possibly-overridden-length.js.default stress/scoped-arguments-array-length.js.ftl-eager-no-cjit microbenchmarks/try-catch-get-by-val-scoped-arguments.js.ftl-eager-no-cjit microbenchmarks/scoped-arguments-length.js.no-cjit-collect-continuously microbenchmarks/scoped-arguments-overridden-length.js.dfg-eager-no-cjit-validate microbenchmarks/try-catch-get-by-val-scoped-arguments.js.no-ftl microbenchmarks/scoped-arguments-possibly-overridden-length.js.ftl-eager microbenchmarks/scoped-arguments-length.js.dfg-eager-no-cjit-validate microbenchmarks/scoped-arguments-length.js.ftl-eager microbenchmarks/scoped-arguments-possibly-overridden-length.js.dfg-eager-no-cjit-validate microbenchmarks/scoped-arguments-length.js.no-cjit-validate-phases stress/captured-arguments-variable.js.ftl-no-cjit-no-inline-validate stress/broken-have-a-bad-time-with-arguments-for-gc-testing.js.misc-ftl-no-cjit stress/captured-arguments-variable.js.ftl-eager-no-cjit-b3o1 microbenchmarks/scoped-arguments-possibly-overridden-length.js.ftl-no-cjit-validate-sampling-profiler stress/scoped-arguments-array-length.js.default stress/scoped-then-direct-arguments-get-by-val-in-baseline.js.ftl-no-cjit-no-put-stack-validate stress/scoped-then-direct-arguments-get-by-val-in-baseline.js.ftl-eager-no-cjit stress/scoped-arguments-array-length.js.ftl-no-cjit-no-inline-validate microbenchmarks/scoped-arguments-length.js.ftl-eager-no-cjit stress/captured-arguments-variable.js.dfg-eager microbenchmarks/scoped-arguments-length.js.ftl-no-cjit-no-inline-validate stress/scoped-then-direct-arguments-get-by-val-in-baseline.js.ftl-eager microbenchmarks/scoped-arguments-overridden-length.js.ftl-no-cjit-small-pool stress/captured-arguments-variable.js.ftl-no-cjit-b3o1 microbenchmarks/scoped-arguments-possibly-overridden-length.js.dfg-eager stress/scoped-arguments-array-length.js.no-cjit-validate-phases microbenchmarks/try-catch-get-by-val-scoped-arguments.js.ftl-eager microbenchmarks/try-catch-get-by-val-scoped-arguments.js.ftl-no-cjit-b3o1 microbenchmarks/try-catch-get-by-val-scoped-arguments.js.dfg-eager stress/scoped-arguments-array-length.js.no-ftl microbenchmarks/scoped-arguments-length.js.dfg-maximal-flush-validate-no-cjit microbenchmarks/scoped-arguments-overridden-length.js.dfg-maximal-flush-validate-no-cjit stress/scoped-arguments-array-length.js.ftl-eager microbenchmarks/scoped-arguments-overridden-length.js.ftl-eager-no-cjit-b3o1 stress/scoped-arguments-array-length.js.dfg-eager-no-cjit-validate stress/captured-arguments-variable.js.no-llint microbenchmarks/scoped-arguments-overridden-length.js.no-ftl stress/captured-arguments-variable.js.ftl-eager stress/scoped-arguments-array-length.js.no-cjit-collect-continuously stress/captured-arguments-variable.js.ftl-no-cjit-small-pool microbenchmarks/scoped-arguments-length.js.ftl-no-cjit-no-put-stack-validate microbenchmarks/try-catch-get-by-val-scoped-arguments.js.dfg-eager-no-cjit-validate stress/scoped-arguments-array-length.js.ftl-no-cjit-validate-sampling-profiler microbenchmarks/scoped-arguments-length.js.ftl-no-cjit-validate-sampling-profiler microbenchmarks/try-catch-get-by-val-scoped-arguments.js.ftl-no-cjit-no-inline-validate microbenchmarks/try-catch-get-by-val-scoped-arguments.js.default microbenchmarks/scoped-arguments-length.js.ftl-eager-no-cjit-b3o1 stress/scoped-then-direct-arguments-get-by-val-in-baseline.js.no-cjit-validate-phases stress/captured-arguments-variable.js.ftl-no-cjit-no-put-stack-validate microbenchmarks/try-catch-get-by-val-scoped-arguments.js.no-llint stress/scoped-arguments-array-length.js.ftl-eager-no-cjit-b3o1 microbenchmarks/try-catch-get-by-val-scoped-arguments.js.no-cjit-collect-continuously microbenchmarks/scoped-arguments-overridden-length.js.default Comment on attachment 333450 [details] WIP Attachment 333450 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/6427496 New failing tests: js/dom/dfg-arguments-alias-activation.html Created attachment 333461 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.6
Comment on attachment 333450 [details] WIP Attachment 333450 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/6427657 New failing tests: js/dom/dfg-arguments-alias-activation.html Created attachment 333464 [details]
Archive of layout-test-results from ews117 for mac-sierra
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117 Port: mac-sierra Platform: Mac OS X 10.12.6
Created attachment 333470 [details]
WIP
Created attachment 333508 [details]
WIP
Attachment 333508 [details] did not pass style-queue:
ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:11215: Should have a space between // and comment [whitespace/comments] [4]
Total errors found: 1 in 43 files
If any of these errors are false positives, please file a bug against check-webkit-style.
The data shows this is a 1% progression on JetStream. I'm gonna prepare a patch. Created attachment 333570 [details]
patch
Comment on attachment 333570 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=333570&action=review r=me. It seems like we could do some amount of optimization in just the DFG if we always had an index mask. If a basic block had multiple GetByVals then we could CSE the index masks. It's probably not the most important thing in the world though. > Source/JavaScriptCore/dfg/DFGSSALoweringPhase.cpp:101 > + default: Can you add a FIXME: to also do this for arguments objects? (In reply to Keith Miller from comment #20) > Comment on attachment 333570 [details] > patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=333570&action=review > > r=me. > > It seems like we could do some amount of optimization in just the DFG if we > always had an index mask. If a basic block had multiple GetByVals then we > could CSE the index masks. It's probably not the most important thing in the > world though. > > > Source/JavaScriptCore/dfg/DFGSSALoweringPhase.cpp:101 > > + default: > > Can you add a FIXME: to also do this for arguments objects? Added both FIXMEs. Created attachment 333637 [details]
patch for landing
Comment on attachment 333637 [details] patch for landing Clearing flags on attachment: 333637 Committed r228411: <https://trac.webkit.org/changeset/228411> All reviewed patches have been landed. Closing bug. (In reply to WebKit Commit Bot from comment #23) > Comment on attachment 333637 [details] > patch for landing > > Clearing flags on attachment: 333637 > > Committed r228411: <https://trac.webkit.org/changeset/228411> This change appears to have caused 96 tests to fail an assert on the 32-bit JSC bot: https://build.webkit.org/builders/Apple%20High%20Sierra%2032-bit%20JSC%20%28BuildAndTest%29/builds/1179 stress/array-from-with-iterator.js.dfg-eager: ASSERTION FAILED: !(m_flags & NodeHasVarArgs) stress/array-from-with-iterator.js.dfg-eager: ./dfg/DFGNode.h(2136) : JSC::DFG::Edge &JSC::DFG::Node::child2() (In reply to Ryan Haddad from comment #25) > (In reply to WebKit Commit Bot from comment #23) > > Comment on attachment 333637 [details] > > patch for landing > > > > Clearing flags on attachment: 333637 > > > > Committed r228411: <https://trac.webkit.org/changeset/228411> > This change appears to have caused 96 tests to fail an assert on the 32-bit > JSC bot: > https://build.webkit.org/builders/Apple%20High%20Sierra%2032- > bit%20JSC%20%28BuildAndTest%29/builds/1179 > > stress/array-from-with-iterator.js.dfg-eager: ASSERTION FAILED: !(m_flags & > NodeHasVarArgs) > stress/array-from-with-iterator.js.dfg-eager: ./dfg/DFGNode.h(2136) : > JSC::DFG::Edge &JSC::DFG::Node::child2() looking into this now. (In reply to Saam Barati from comment #26) > (In reply to Ryan Haddad from comment #25) > > (In reply to WebKit Commit Bot from comment #23) > > > Comment on attachment 333637 [details] > > > patch for landing > > > > > > Clearing flags on attachment: 333637 > > > > > > Committed r228411: <https://trac.webkit.org/changeset/228411> > > This change appears to have caused 96 tests to fail an assert on the 32-bit > > JSC bot: > > https://build.webkit.org/builders/Apple%20High%20Sierra%2032- > > bit%20JSC%20%28BuildAndTest%29/builds/1179 > > > > stress/array-from-with-iterator.js.dfg-eager: ASSERTION FAILED: !(m_flags & > > NodeHasVarArgs) > > stress/array-from-with-iterator.js.dfg-eager: ./dfg/DFGNode.h(2136) : > > JSC::DFG::Edge &JSC::DFG::Node::child2() > > looking into this now. Should be fixed as of: https://trac.webkit.org/changeset/228438/webkit |