Bug 175610

Summary: Teach DFGFixupPhase.cpp that the current scope is always a cell
Product: WebKit Reporter: Robin Morisset <rmorisset>
Component: JavaScriptCoreAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Enhancement CC: buildbot, commit-queue, keith_miller, mark.lam, msaboff, saam, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch, with FTL fixed, now passes test locally
none
Patch, with nits fixed
none
Patch, reverting to KnownCellUse
none
Patch, reverting to KnownCellUse none

Description Robin Morisset 2017-08-15 17:38:46 PDT 
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).
Comment 1 Robin Morisset 2017-08-16 10:34:53 PDT 
Created attachment 318271 [details]
Patch
Comment 2 Build Bot 2017-08-16 11:26:08 PDT 
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
Comment 3 Robin Morisset 2017-08-17 11:33:05 PDT 
Created attachment 318391 [details]
Patch
Comment 4 Build Bot 2017-08-17 11:34:47 PDT 
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.
Comment 5 Build Bot 2017-08-17 12:30:31 PDT 
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
Comment 6 Saam Barati 2017-08-17 13:07:19 PDT 
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
Comment 7 Robin Morisset 2017-08-17 14:28:42 PDT 
Created attachment 318421 [details]
Patch, with FTL fixed, now passes test locally
Comment 8 Build Bot 2017-08-17 14:30:16 PDT 
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.
Comment 9 Keith Miller 2017-08-17 14:36:26 PDT 
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.
Comment 10 Keith Miller 2017-08-17 14:42:49 PDT 
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.
Comment 11 Robin Morisset 2017-08-17 14:54:32 PDT 
Created attachment 318425 [details]
Patch, with nits fixed
Comment 12 Robin Morisset 2017-08-17 14:56:48 PDT 
Created attachment 318426 [details]
Patch, reverting to KnownCellUse
Comment 13 Build Bot 2017-08-17 14:59:51 PDT 
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.
Comment 14 Robin Morisset 2017-08-17 15:02:36 PDT 
Created attachment 318427 [details]
Patch, reverting to KnownCellUse
Comment 15 Build Bot 2017-08-17 15:05:22 PDT 
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.
Comment 16 WebKit Commit Bot 2017-08-17 17:19:43 PDT 
Comment on attachment 318427 [details]
Patch, reverting to KnownCellUse

Clearing flags on attachment: 318427

Committed r220890: <http://trac.webkit.org/changeset/220890>
Comment 17 WebKit Commit Bot 2017-08-17 17:19:45 PDT 
All reviewed patches have been landed.  Closing bug.
Comment 18 Radar WebKit Bug Importer 2017-08-17 17:20:53 PDT 
<rdar://problem/33953899>