Bug 189682 – ArgumentsEliminationPhase should snip basic blocks after proven OSR exits

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

[Saam Barati]

The arguments elimination phase will generate code that is safe because it queries isPseudoTerminal. It will then generate code that will fail validation.

[Saam Barati]

<rdar://problem/43557315>

[Saam Barati]

Created attachment 349947 [details]
patch

[Mark Lam]

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]

Created attachment 349950 [details]
patch for landing

[Filip Pizlo]

Comment on attachment 349950 [details]
patch for landing

Why not make arguments elimination just emit good IR?

[Saam Barati]

Comment on attachment 349950 [details]
patch for landing

Phil suggested just snipping the basic block after the pseudo terminal

[Saam Barati]

Created attachment 349979 [details]
patch

[EWS Watchlist]

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]

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

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

[Saam Barati]

Created attachment 350298 [details]
patch

[Mark Lam]

Comment on attachment 350298 [details]
patch

LGTM.

[Saam Barati]

Comment on attachment 350298 [details]
patch

Thanks for reviewing.

[WebKit Commit Bot]

Comment on attachment 350298 [details]
patch

Clearing flags on attachment: 350298

Committed r236421: <https://trac.webkit.org/changeset/236421>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.