Bug 189682 - ArgumentsEliminationPhase should snip basic blocks after proven OSR exits
Summary: ArgumentsEliminationPhase should snip basic blocks after proven OSR exits
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:
 
Reported: 2018-09-17 14:57 PDT by Saam Barati
Modified: 2018-09-24 13:12 PDT (History)
13 users (show)

See Also:


Attachments
patch (8.38 KB, patch)
2018-09-17 15:05 PDT, Saam Barati
mark.lam: review+
Details | Formatted Diff | Diff
patch for landing (8.34 KB, patch)
2018-09-17 15:16 PDT, Saam Barati
no flags Details | Formatted Diff | Diff
patch (4.46 KB, patch)
2018-09-17 17:29 PDT, Saam Barati
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
patch (4.32 KB, patch)
2018-09-20 18:33 PDT, 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-09-17 14:57:53 PDT
The arguments elimination phase will generate code that is safe because it queries isPseudoTerminal. It will then generate code that will fail validation.
Comment 1 Saam Barati 2018-09-17 14:58:38 PDT
<rdar://problem/43557315>
Comment 2 Saam Barati 2018-09-17 15:05:56 PDT
Created attachment 349947 [details]
patch
Comment 3 Mark Lam 2018-09-17 15:13:14 PDT
Comment on attachment 349947 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=349947&action=review

LGTM.

> Source/JavaScriptCore/ChangeLog:11
> +        This is because the arguments elimination phase doesn't stops doing

/stops/stop/.

> Source/JavaScriptCore/ChangeLog:15
> +        This patch makes it so that validation is does not fail on code like

/is does/does/.
Comment 4 Saam Barati 2018-09-17 15:16:35 PDT
Created attachment 349950 [details]
patch for landing
Comment 5 Filip Pizlo 2018-09-17 15:35:47 PDT
Comment on attachment 349950 [details]
patch for landing

Why not make arguments elimination just emit good IR?
Comment 6 Saam Barati 2018-09-17 17:19:38 PDT
Comment on attachment 349950 [details]
patch for landing

Phil suggested just snipping the basic block after the pseudo terminal
Comment 7 Saam Barati 2018-09-17 17:29:20 PDT
Created attachment 349979 [details]
patch
Comment 8 EWS Watchlist 2018-09-17 19:02:25 PDT
Comment on attachment 349979 [details]
patch

Attachment 349979 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/9250743

New failing tests:
stress/spread-calling.js.ftl-eager-no-cjit-b3o1
stress/array-concat-with-slow-indexingtypes.js.ftl-no-cjit-b3o1
stress/async-await-module-reserved-word.js.ftl-eager-no-cjit
stress/spread-capture-rest.js.ftl-no-cjit-no-put-stack-validate
stress/modules-syntax.js.ftl-eager
stress/array-concat-with-slow-indexingtypes.js.ftl-eager-no-cjit-b3o1
stress/spread-multi-layers.js.ftl-eager-no-cjit-b3o1
stress/spread-multi-layers.js.ftl-no-cjit-validate-sampling-profiler
stress/reserved-word-with-escape.js.ftl-eager-no-cjit
stress/spread-multi-layers.js.ftl-no-cjit-b3o1
stress/array-concat-with-slow-indexingtypes.js.ftl-eager-no-cjit
stress/reserved-word-with-escape.js.ftl-eager
stress/modules-syntax-error-with-names.js.ftl-eager-no-cjit-b3o1
stress/spread-calling.js.ftl-eager-no-cjit
stress/modules-syntax-error.js.ftl-eager-no-cjit-b3o1
stress/regress-187060.js.ftl-eager-no-cjit-b3o1
stress/for-in-invalidate-context-weird-assignments.js.ftl-eager-no-cjit-b3o1
stress/array-symbol-species-lazy-watchpoints.js.ftl-eager-no-cjit-b3o1
stress/for-in-invalidate-context-weird-assignments.js.ftl-eager-no-cjit
jsc-layout-tests.yaml/js/script-tests/stack-trace.js.layout-ftl-eager-no-cjit
stress/async-await-module-reserved-word.js.ftl-eager-no-cjit-b3o1
stress/spread-capture-rest.js.ftl-eager-no-cjit-b3o1
stress/spread-multi-layers.js.ftl-eager
stress/reserved-word-with-escape.js.ftl-eager-no-cjit-b3o1
stress/spread-multi-layers.js.ftl-eager-no-cjit
stress/spread-capture-rest.js.ftl-eager
stress/regress-187060.js.ftl-eager-no-cjit
stress/spread-calling.js.ftl-eager
stress/array-symbol-species-lazy-watchpoints.js.ftl-eager-no-cjit
stress/modules-syntax-error-with-names.js.ftl-eager
stress/array-concat-with-slow-indexingtypes.js.ftl-no-cjit-no-put-stack-validate
ChakraCore.yaml/ChakraCore/test/typedarray/dataview.js.default
stress/spread-non-varargs.js.ftl-no-cjit-validate-sampling-profiler
stress/spread-non-varargs.js.ftl-eager-no-cjit
stress/modules-syntax-error.js.ftl-eager-no-cjit
stress/rest-elements.js.ftl-eager-no-cjit-b3o1
stress/spread-non-varargs.js.default
stress/spread-capture-rest.js.ftl-eager-no-cjit
stress/spread-non-varargs.js.ftl-eager
stress/spread-multi-layers.js.ftl-no-cjit-no-put-stack-validate
stress/spread-capture-rest.js.ftl-no-cjit-small-pool
stress/rest-elements.js.ftl-eager
stress/regress-187060.js.ftl-eager
stress/array-symbol-species-lazy-watchpoints.js.ftl-eager
stress/spread-non-varargs.js.ftl-eager-no-cjit-b3o1
stress/spread-non-varargs.js.ftl-no-cjit-small-pool
stress/array-concat-with-slow-indexingtypes.js.ftl-eager
stress/spread-capture-rest.js.ftl-no-cjit-validate-sampling-profiler
ChakraCore.yaml/ChakraCore/test/ControlFlow/enumeration_adddelete.js.default
stress/array-concat-with-slow-indexingtypes.js.ftl-no-cjit-validate-sampling-profiler
stress/spread-capture-rest.js.default
stress/rest-elements.js.ftl-eager-no-cjit
microbenchmarks/deltablue-varargs.js.ftl-eager-no-cjit
stress/spread-non-varargs.js.ftl-no-cjit-b3o1
stress/spread-capture-rest.js.ftl-no-cjit-b3o1
stress/array-concat-with-slow-indexingtypes.js.ftl-no-cjit-no-inline-validate
stress/array-concat-with-slow-indexingtypes.js.default
stress/trailing-comma-in-patterns.js.ftl-eager-no-cjit-b3o1
stress/spread-multi-layers.js.default
stress/modules-syntax.js.ftl-eager-no-cjit
stress/modules-syntax-error-with-names.js.ftl-eager-no-cjit
stress/arguments-elimination-will-generate-edge-without-result.js.default
stress/spread-non-varargs.js.ftl-no-cjit-no-put-stack-validate
stress/array-concat-with-slow-indexingtypes.js.ftl-no-cjit-small-pool
stress/modules-syntax.js.ftl-eager-no-cjit-b3o1
apiTests
Comment 9 Saam Barati 2018-09-19 22:13:32 PDT
(In reply to Build Bot from comment #8)
> Comment on attachment 349979 [details]
> patch
> 
> Attachment 349979 [details] did not pass jsc-ews (mac):
> Output: https://webkit-queues.webkit.org/results/9250743
> 
> New failing tests:
> stress/spread-calling.js.ftl-eager-no-cjit-b3o1
> stress/array-concat-with-slow-indexingtypes.js.ftl-no-cjit-b3o1
> stress/async-await-module-reserved-word.js.ftl-eager-no-cjit
> stress/spread-capture-rest.js.ftl-no-cjit-no-put-stack-validate
> stress/modules-syntax.js.ftl-eager
> stress/array-concat-with-slow-indexingtypes.js.ftl-eager-no-cjit-b3o1
> stress/spread-multi-layers.js.ftl-eager-no-cjit-b3o1
> stress/spread-multi-layers.js.ftl-no-cjit-validate-sampling-profiler
> stress/reserved-word-with-escape.js.ftl-eager-no-cjit
> stress/spread-multi-layers.js.ftl-no-cjit-b3o1
> stress/array-concat-with-slow-indexingtypes.js.ftl-eager-no-cjit
> stress/reserved-word-with-escape.js.ftl-eager
> stress/modules-syntax-error-with-names.js.ftl-eager-no-cjit-b3o1
> stress/spread-calling.js.ftl-eager-no-cjit
> stress/modules-syntax-error.js.ftl-eager-no-cjit-b3o1
> stress/regress-187060.js.ftl-eager-no-cjit-b3o1
> stress/for-in-invalidate-context-weird-assignments.js.ftl-eager-no-cjit-b3o1
> stress/array-symbol-species-lazy-watchpoints.js.ftl-eager-no-cjit-b3o1
> stress/for-in-invalidate-context-weird-assignments.js.ftl-eager-no-cjit
> jsc-layout-tests.yaml/js/script-tests/stack-trace.js.layout-ftl-eager-no-cjit
> stress/async-await-module-reserved-word.js.ftl-eager-no-cjit-b3o1
> stress/spread-capture-rest.js.ftl-eager-no-cjit-b3o1
> stress/spread-multi-layers.js.ftl-eager
> stress/reserved-word-with-escape.js.ftl-eager-no-cjit-b3o1
> stress/spread-multi-layers.js.ftl-eager-no-cjit
> stress/spread-capture-rest.js.ftl-eager
> stress/regress-187060.js.ftl-eager-no-cjit
> stress/spread-calling.js.ftl-eager
> stress/array-symbol-species-lazy-watchpoints.js.ftl-eager-no-cjit
> stress/modules-syntax-error-with-names.js.ftl-eager
> stress/array-concat-with-slow-indexingtypes.js.ftl-no-cjit-no-put-stack-
> validate
> ChakraCore.yaml/ChakraCore/test/typedarray/dataview.js.default
> stress/spread-non-varargs.js.ftl-no-cjit-validate-sampling-profiler
> stress/spread-non-varargs.js.ftl-eager-no-cjit
> stress/modules-syntax-error.js.ftl-eager-no-cjit
> stress/rest-elements.js.ftl-eager-no-cjit-b3o1
> stress/spread-non-varargs.js.default
> stress/spread-capture-rest.js.ftl-eager-no-cjit
> stress/spread-non-varargs.js.ftl-eager
> stress/spread-multi-layers.js.ftl-no-cjit-no-put-stack-validate
> stress/spread-capture-rest.js.ftl-no-cjit-small-pool
> stress/rest-elements.js.ftl-eager
> stress/regress-187060.js.ftl-eager
> stress/array-symbol-species-lazy-watchpoints.js.ftl-eager
> stress/spread-non-varargs.js.ftl-eager-no-cjit-b3o1
> stress/spread-non-varargs.js.ftl-no-cjit-small-pool
> stress/array-concat-with-slow-indexingtypes.js.ftl-eager
> stress/spread-capture-rest.js.ftl-no-cjit-validate-sampling-profiler
> ChakraCore.yaml/ChakraCore/test/ControlFlow/enumeration_adddelete.js.default
> stress/array-concat-with-slow-indexingtypes.js.ftl-no-cjit-validate-sampling-
> profiler
> stress/spread-capture-rest.js.default
> stress/rest-elements.js.ftl-eager-no-cjit
> microbenchmarks/deltablue-varargs.js.ftl-eager-no-cjit
> stress/spread-non-varargs.js.ftl-no-cjit-b3o1
> stress/spread-capture-rest.js.ftl-no-cjit-b3o1
> stress/array-concat-with-slow-indexingtypes.js.ftl-no-cjit-no-inline-validate
> stress/array-concat-with-slow-indexingtypes.js.default
> stress/trailing-comma-in-patterns.js.ftl-eager-no-cjit-b3o1
> stress/spread-multi-layers.js.default
> stress/modules-syntax.js.ftl-eager-no-cjit
> stress/modules-syntax-error-with-names.js.ftl-eager-no-cjit
> stress/arguments-elimination-will-generate-edge-without-result.js.default
> stress/spread-non-varargs.js.ftl-no-cjit-no-put-stack-validate
> stress/array-concat-with-slow-indexingtypes.js.ftl-no-cjit-small-pool
> stress/modules-syntax.js.ftl-eager-no-cjit-b3o1
> apiTests

My guess here is somehow the GetStack turned out to be meaningful in OSR exit. However, that's just a guess. I haven't yet had time to look into these failures.
Comment 10 Saam Barati 2018-09-20 17:51:31 PDT
I think my previous analysis is wrong. It looks like I just forgot to call invalidateCFG
Comment 11 Saam Barati 2018-09-20 18:33:48 PDT
Created attachment 350298 [details]
patch
Comment 12 Mark Lam 2018-09-24 12:28:21 PDT
Comment on attachment 350298 [details]
patch

LGTM.
Comment 13 Saam Barati 2018-09-24 12:45:17 PDT
Comment on attachment 350298 [details]
patch

Thanks for reviewing.
Comment 14 WebKit Commit Bot 2018-09-24 13:12:34 PDT
Comment on attachment 350298 [details]
patch

Clearing flags on attachment: 350298

Committed r236421: <https://trac.webkit.org/changeset/236421>
Comment 15 WebKit Commit Bot 2018-09-24 13:12:36 PDT
All reviewed patches have been landed.  Closing bug.