Bug 182633 - Add a GetIndexMask node and make it an input to GetByVal for array and typed array accesses in DFG SSA
Summary: Add a GetIndexMask node and make it an input to GetByVal for array and typed ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Saam Barati
URL:
Keywords: InRadar
Depends on:
Blocks: 182676
  Show dependency treegraph
 
Reported: 2018-02-08 17:39 PST by Saam Barati
Modified: 2018-02-13 15:03 PST (History)
17 users (show)

See Also:


Attachments
WIP (44.57 KB, patch)
2018-02-08 19:00 PST, Saam Barati
no flags Details | Formatted Diff | Diff
WIP (48.81 KB, patch)
2018-02-08 19:59 PST, Saam Barati
no flags Details | Formatted Diff | Diff
WIP (49.21 KB, patch)
2018-02-08 20:01 PST, Saam Barati
no flags Details | Formatted Diff | Diff
WIP (83.95 KB, patch)
2018-02-08 23:49 PST, Saam Barati
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-sierra (2.26 MB, application/zip)
2018-02-09 00:51 PST, Build Bot
no flags Details
Archive of layout-test-results from ews107 for mac-sierra-wk2 (2.66 MB, application/zip)
2018-02-09 00:55 PST, Build Bot
no flags Details
Archive of layout-test-results from ews125 for ios-simulator-wk2 (2.36 MB, application/zip)
2018-02-09 01:11 PST, Build Bot
no flags Details
Archive of layout-test-results from ews117 for mac-sierra (2.98 MB, application/zip)
2018-02-09 01:21 PST, Build Bot
no flags Details
WIP (84.86 KB, patch)
2018-02-09 01:56 PST, Saam Barati
no flags Details | Formatted Diff | Diff
WIP (86.38 KB, patch)
2018-02-09 12:04 PST, Saam Barati
no flags Details | Formatted Diff | Diff
patch (93.43 KB, patch)
2018-02-11 11:42 PST, Saam Barati
keith_miller: review+
Details | Formatted Diff | Diff
patch for landing (93.75 KB, patch)
2018-02-12 15:18 PST, Saam Barati
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Saam Barati 2018-02-08 17:39:20 PST
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.
Comment 1 Saam Barati 2018-02-08 19:00:49 PST
Created attachment 333438 [details]
WIP

It's a start. Probably doesn't build yet.
Comment 2 Saam Barati 2018-02-08 19:59:28 PST
Created attachment 333440 [details]
WIP
Comment 3 Saam Barati 2018-02-08 20:01:52 PST
Created attachment 333441 [details]
WIP
Comment 4 Saam Barati 2018-02-08 23:49:31 PST
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 5 Build Bot 2018-02-09 00:51:27 PST
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
Comment 6 Build Bot 2018-02-09 00:51:29 PST
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 7 Build Bot 2018-02-09 00:55:38 PST
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
Comment 8 Build Bot 2018-02-09 00:55:40 PST
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 9 Build Bot 2018-02-09 00:58:58 PST
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 10 Build Bot 2018-02-09 01:11:13 PST
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
Comment 11 Build Bot 2018-02-09 01:11:15 PST
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 12 Build Bot 2018-02-09 01:21:23 PST
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
Comment 13 Build Bot 2018-02-09 01:21:24 PST
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
Comment 14 Saam Barati 2018-02-09 01:56:06 PST
Created attachment 333470 [details]
WIP
Comment 15 Saam Barati 2018-02-09 12:04:31 PST
Created attachment 333508 [details]
WIP
Comment 16 Build Bot 2018-02-09 13:16:45 PST
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.
Comment 17 Saam Barati 2018-02-11 10:03:57 PST
The data shows this is a 1% progression on JetStream. I'm gonna prepare a patch.
Comment 18 Saam Barati 2018-02-11 11:42:01 PST
Created attachment 333570 [details]
patch
Comment 19 Saam Barati 2018-02-11 12:02:41 PST
<rdar://problem/37441037>
Comment 20 Keith Miller 2018-02-12 14:12:46 PST
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?
Comment 21 Saam Barati 2018-02-12 15:16:06 PST
(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.
Comment 22 Saam Barati 2018-02-12 15:18:30 PST
Created attachment 333637 [details]
patch for landing
Comment 23 WebKit Commit Bot 2018-02-12 17:12:34 PST
Comment on attachment 333637 [details]
patch for landing

Clearing flags on attachment: 333637

Committed r228411: <https://trac.webkit.org/changeset/228411>
Comment 24 WebKit Commit Bot 2018-02-12 17:12:36 PST
All reviewed patches have been landed.  Closing bug.
Comment 25 Ryan Haddad 2018-02-13 13:46:34 PST
(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()
Comment 26 Saam Barati 2018-02-13 14:33:13 PST
(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.
Comment 27 Saam Barati 2018-02-13 15:03:55 PST
(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