Bug 189682

Summary:

ArgumentsEliminationPhase should snip basic blocks after proven OSR exits

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, keith_miller, mark.lam, msaboff, rmorisset, ticaiolima, webkit-bug-importer, ysuzuki

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
patch	mark.lam: review+
patch for landing	none
patch	ews-watchlist: commit-queue-
patch	none

Saam Barati

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

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
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Saam Barati

Comment 1 2018-09-17 14:58:38 PDT

<rdar://problem/43557315>

Saam Barati

Comment 2 2018-09-17 15:05:56 PDT

Created attachment 349947 [details] patch

Mark Lam

Comment 3 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/.

Saam Barati

Comment 4 2018-09-17 15:16:35 PDT

Created attachment 349950 [details] patch for landing

Filip Pizlo

Comment 5 2018-09-17 15:35:47 PDT

Comment on attachment 349950 [details] patch for landing Why not make arguments elimination just emit good IR?

Saam Barati

Comment 6 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

Saam Barati

Comment 7 2018-09-17 17:29:20 PDT

Created attachment 349979 [details] patch

EWS Watchlist

Comment 8 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

Saam Barati

Comment 9 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.

Saam Barati

Comment 10 2018-09-20 17:51:31 PDT

I think my previous analysis is wrong. It looks like I just forgot to call invalidateCFG

Saam Barati

Comment 11 2018-09-20 18:33:48 PDT

Created attachment 350298 [details] patch

Mark Lam

Comment 12 2018-09-24 12:28:21 PDT

Comment on attachment 350298 [details] patch LGTM.

Saam Barati

Comment 13 2018-09-24 12:45:17 PDT

Comment on attachment 350298 [details] patch Thanks for reviewing.

WebKit Commit Bot

Comment 14 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>

WebKit Commit Bot

Comment 15 2018-09-24 13:12:36 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.