Bug 182633 – Add a GetIndexMask node and make it an input to GetByVal for array and typed array accesses in DFG SSA

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

[Saam Barati]

B3 isn't able to hoist loading the index mask out of loops where we logically should be able to hoist it out. The DFG would probably be able to preform such a transformation.

[Saam Barati]

Created attachment 333438 [details]
WIP

It's a start. Probably doesn't build yet.

[Saam Barati]

Created attachment 333440 [details]
WIP

[Saam Barati]

Created attachment 333441 [details]
WIP

[Saam Barati]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Saam Barati]

Created attachment 333470 [details]
WIP

[Saam Barati]

Created attachment 333508 [details]
WIP

[EWS Watchlist]

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.

[Saam Barati]

The data shows this is a 1% progression on JetStream. I'm gonna prepare a patch.

[Saam Barati]

Created attachment 333570 [details]
patch

[Saam Barati]

<rdar://problem/37441037>

[Keith Miller]

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?

[Saam Barati]

(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.

[Saam Barati]

Created attachment 333637 [details]
patch for landing

[WebKit Commit Bot]

Comment on attachment 333637 [details]
patch for landing

Clearing flags on attachment: 333637

Committed r228411: <https://trac.webkit.org/changeset/228411>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryan Haddad]

(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()

[Saam Barati]

(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.

[Saam Barati]

(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