Bug 175610 - Teach DFGFixupPhase.cpp that the current scope is always a cell
Summary: Teach DFGFixupPhase.cpp that the current scope is always a cell
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Enhancement
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-08-15 17:38 PDT by Robin Morisset
Modified: 2017-08-17 17:20 PDT (History)
7 users (show)

See Also:


Attachments
Patch (1.93 KB, patch)
2017-08-16 10:34 PDT, Robin Morisset
no flags Details | Formatted Diff | Diff
Patch (8.12 KB, patch)
2017-08-17 11:33 PDT, Robin Morisset
no flags Details | Formatted Diff | Diff
Patch, with FTL fixed, now passes test locally (9.44 KB, patch)
2017-08-17 14:28 PDT, Robin Morisset
no flags Details | Formatted Diff | Diff
Patch, with nits fixed (9.56 KB, patch)
2017-08-17 14:54 PDT, Robin Morisset
no flags Details | Formatted Diff | Diff
Patch, reverting to KnownCellUse (9.57 KB, patch)
2017-08-17 14:56 PDT, Robin Morisset
no flags Details | Formatted Diff | Diff
Patch, reverting to KnownCellUse (9.66 KB, patch)
2017-08-17 15:02 PDT, Robin Morisset
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>