Bug 175610 – Teach DFGFixupPhase.cpp that the current scope is always a cell

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

[Robin Morisset]

This applies to
- CreateScopedArgument
- CreateActivation
- PushWithScope
- NewFunction
- NewGeneratorFunction
- NewAsyncFunction

Their first argument is currently CellUse, but we can make it KnownCellUse.

In the case of PushWithScope we can also speculate that the second argument is likely to be an object (since toObject is called on it).

[Robin Morisset]

Created attachment 318271 [details]
Patch

[Build Bot]

Comment on attachment 318271 [details]
Patch

Attachment 318271 [details] did not pass jsc-ews (mac):
Output: http://webkit-queues.webkit.org/results/4325240

New failing tests:
stress/eval-func-decl-in-eval-within-with-scope.js.default
stress/get-from-scope-dynamic-onto-proxy.js.dfg-eager
stress/constant-closure-var-with-dynamic-invalidation.js.ftl-eager
stress/es6-default-parameters.js.ftl-eager-no-cjit
stress/global-lexical-variable-with-statement.js.dfg-eager-no-cjit-validate
stress/eval-func-decl-in-eval-within-with-scope.js.dfg-eager
jsc-layout-tests.yaml/js/script-tests/exception-propagate-from-dfg-to-llint.js.layout-ftl-eager-no-cjit
stress/eval-func-decl-in-eval-within-with-scope.js.ftl-eager-no-cjit-b3o1
stress/with.js.ftl-no-cjit-validate-sampling-profiler
stress/eval-func-decl-in-eval-within-with-scope.js.ftl-no-cjit-small-pool
stress/constant-closure-var-with-dynamic-invalidation.js.ftl-no-cjit-no-inline-validate
stress/lexical-let-and-with-statement.js.ftl-eager-no-cjit-b3o1
stress/constant-closure-var-with-dynamic-invalidation.js.ftl-eager-no-cjit
stress/constant-closure-var-with-dynamic-invalidation.js.ftl-eager-no-cjit-b3o1
stress/eval-func-decl-in-eval-within-with-scope.js.dfg-maximal-flush-validate-no-cjit
stress/eval-func-decl-in-eval-within-with-scope.js.ftl-no-cjit-b3o1
stress/const-and-with-statement.js.ftl-eager
stress/eval-func-decl-in-eval-within-with-scope.js.ftl-no-cjit-no-put-stack-validate
jsc-layout-tests.yaml/js/script-tests/eval-and-with.js.layout-dfg-eager-no-cjit
stress/eval-func-decl-in-eval-within-with-scope.js.ftl-no-cjit-validate-sampling-profiler
stress/eval-func-decl-in-eval-within-with-scope.js.no-llint
stress/constant-closure-var-with-dynamic-invalidation.js.ftl-no-cjit-b3o1
stress/constant-closure-var-with-dynamic-invalidation.js.ftl-no-cjit-small-pool
stress/constant-closure-var-with-dynamic-invalidation.js.ftl-no-cjit-no-put-stack-validate
stress/get-from-scope-dynamic-onto-proxy.js.dfg-eager-no-cjit-validate
stress/with.js.ftl-eager-no-cjit-b3o1
mozilla-tests.yaml/js1_5/Scope/regress-208496-002.js.mozilla-ftl-eager-no-cjit-validate-phases
stress/const-and-with-statement.js.ftl-eager-no-cjit-b3o1
stress/eval-func-decl-in-eval-within-with-scope.js.no-ftl
stress/with.js.dfg-eager-no-cjit-validate
stress/with.js.no-cjit-validate-phases
stress/eval-func-decl-in-eval-within-with-scope.js.ftl-eager-no-cjit
stress/with.js.ftl-no-cjit-no-inline-validate
stress/const-and-with-statement.js.ftl-eager-no-cjit
stress/constant-closure-var-with-dynamic-invalidation.js.ftl-no-cjit-validate-sampling-profiler
stress/eval-func-decl-in-eval-within-with-scope.js.no-cjit-collect-continuously
stress/global-lexical-variable-with-statement.js.dfg-eager
stress/es6-default-parameters.js.ftl-eager
stress/global-lexical-variable-with-statement.js.ftl-eager-no-cjit-b3o1
stress/es6-default-parameters.js.ftl-eager-no-cjit-b3o1
stress/eval-func-decl-in-eval-within-with-scope.js.no-cjit-validate-phases
jsc-layout-tests.yaml/js/script-tests/eval-and-with.js.layout-ftl-eager-no-cjit
stress/eval-func-decl-in-eval-within-with-scope.js.dfg-eager-no-cjit-validate
stress/get-from-scope-dynamic-onto-proxy.js.ftl-eager-no-cjit-b3o1
stress/get-from-scope-dynamic-onto-proxy.js.ftl-eager
stress/with.js.dfg-maximal-flush-validate-no-cjit
mozilla-tests.yaml/js1_5/Scope/scope-004.js.mozilla-dfg-eager-no-cjit-validate-phases
stress/with.js.ftl-no-cjit-no-put-stack-validate
stress/with.js.ftl-eager-no-cjit
stress/lexical-let-and-with-statement.js.ftl-eager
stress/global-lexical-variable-with-statement.js.ftl-eager-no-cjit
stress/with.js.ftl-no-cjit-b3o1
stress/constant-closure-var-with-dynamic-invalidation.js.default
stress/eval-func-decl-in-eval-within-with-scope.js.ftl-no-cjit-no-inline-validate
stress/get-from-scope-dynamic-onto-proxy.js.ftl-eager-no-cjit
stress/eval-func-decl-in-eval-within-with-scope.js.ftl-eager
mozilla-tests.yaml/js1_5/Scope/scope-004.js.mozilla-ftl-eager-no-cjit-validate-phases
stress/lexical-let-and-with-statement.js.ftl-eager-no-cjit

[Robin Morisset]

Created attachment 318391 [details]
Patch

[Build Bot]

Attachment 318391 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/jit/JITOperations.h:424:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/jit/JITOperations.h:424:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 318391 [details]
Patch

Attachment 318391 [details] did not pass jsc-ews (mac):
Output: http://webkit-queues.webkit.org/results/4331719

New failing tests:
stress/eval-func-decl-in-eval-within-with-scope.js.default
stress/constant-closure-var-with-dynamic-invalidation.js.ftl-eager
stress/es6-default-parameters.js.ftl-eager-no-cjit
stress/eval-func-decl-in-eval-within-with-scope.js.ftl-eager-no-cjit-b3o1
stress/eval-func-decl-in-eval-within-with-scope.js.ftl-no-cjit-small-pool
stress/constant-closure-var-with-dynamic-invalidation.js.ftl-no-cjit-no-inline-validate
stress/lexical-let-and-with-statement.js.ftl-eager-no-cjit-b3o1
stress/constant-closure-var-with-dynamic-invalidation.js.ftl-eager-no-cjit
stress/constant-closure-var-with-dynamic-invalidation.js.ftl-eager-no-cjit-b3o1
stress/eval-func-decl-in-eval-within-with-scope.js.ftl-no-cjit-b3o1
stress/const-and-with-statement.js.ftl-eager
stress/eval-func-decl-in-eval-within-with-scope.js.ftl-no-cjit-no-put-stack-validate
stress/eval-func-decl-in-eval-within-with-scope.js.ftl-no-cjit-validate-sampling-profiler
stress/constant-closure-var-with-dynamic-invalidation.js.ftl-no-cjit-b3o1
stress/constant-closure-var-with-dynamic-invalidation.js.ftl-no-cjit-no-put-stack-validate
stress/get-from-scope-dynamic-onto-proxy.js.ftl-eager
mozilla-tests.yaml/js1_5/Scope/regress-208496-002.js.mozilla-ftl-eager-no-cjit-validate-phases
stress/const-and-with-statement.js.ftl-eager-no-cjit-b3o1
stress/eval-func-decl-in-eval-within-with-scope.js.ftl-eager-no-cjit
stress/constant-closure-var-with-dynamic-invalidation.js.ftl-no-cjit-small-pool
stress/const-and-with-statement.js.ftl-eager-no-cjit
stress/constant-closure-var-with-dynamic-invalidation.js.ftl-no-cjit-validate-sampling-profiler
stress/es6-default-parameters.js.ftl-eager
stress/global-lexical-variable-with-statement.js.ftl-eager-no-cjit-b3o1
stress/es6-default-parameters.js.ftl-eager-no-cjit-b3o1
stress/lexical-let-and-with-statement.js.ftl-eager-no-cjit
jsc-layout-tests.yaml/js/script-tests/eval-and-with.js.layout-ftl-eager-no-cjit
jsc-layout-tests.yaml/js/script-tests/exception-propagate-from-dfg-to-llint.js.layout-ftl-eager-no-cjit
stress/get-from-scope-dynamic-onto-proxy.js.ftl-eager-no-cjit-b3o1
stress/lexical-let-and-with-statement.js.ftl-eager
stress/global-lexical-variable-with-statement.js.ftl-eager-no-cjit
stress/constant-closure-var-with-dynamic-invalidation.js.default
stress/eval-func-decl-in-eval-within-with-scope.js.ftl-no-cjit-no-inline-validate
stress/get-from-scope-dynamic-onto-proxy.js.ftl-eager-no-cjit
stress/eval-func-decl-in-eval-within-with-scope.js.ftl-eager

[Saam Barati]

Comment on attachment 318391 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:1136
> +    if (objectEdge.useKind() == ObjectUse) {

You need this logic in the FTL too

[Robin Morisset]

Created attachment 318421 [details]
Patch, with FTL fixed, now passes test locally

[Build Bot]

Attachment 318421 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/jit/JITOperations.h:424:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/jit/JITOperations.h:424:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Keith Miller]

Comment on attachment 318421 [details]
Patch, with FTL fixed, now passes test locally

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

r=me with some minor changes.

> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:1717
> +            fixEdge<KnownCellUse>(node->child1());

I would change this to ObjectUse and make a FIXME to add KnownObjectUse.

> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:1723
> +            fixEdge<KnownCellUse>(node->child1());

Ditto.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:1146
> +        JSValueOperand object(this, node->child2());

Nit: this could be objectEdge.

[Keith Miller]

Comment on attachment 318421 [details]
Patch, with FTL fixed, now passes test locally

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

>> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:1717
>> +            fixEdge<KnownCellUse>(node->child1());
> 
> I would change this to ObjectUse and make a FIXME to add KnownObjectUse.

Nvm. ignore this. Just file a FIXME for the KnownObjectUse.

>> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:1723
>> +            fixEdge<KnownCellUse>(node->child1());
> 
> Ditto.

Ditto.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:4279
> +            LValue object = lowJSValue(m_node->child2());

Nit: this could also be objectEdge.

[Robin Morisset]

Created attachment 318425 [details]
Patch, with nits fixed

[Robin Morisset]

Created attachment 318426 [details]
Patch, reverting to KnownCellUse

[Build Bot]

Attachment 318426 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/jit/JITOperations.h:424:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/jit/JITOperations.h:424:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Robin Morisset]

Created attachment 318427 [details]
Patch, reverting to KnownCellUse

[Build Bot]

Attachment 318427 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/jit/JITOperations.h:424:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/jit/JITOperations.h:424:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 318427 [details]
Patch, reverting to KnownCellUse

Clearing flags on attachment: 318427

Committed r220890: <http://trac.webkit.org/changeset/220890>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/33953899>