Remove support for "loads and stores to global vars through property cell shortcuts inst…

Remove support for "loads and stores to global vars through property cell shortcuts installed into parent script context" from all compilers.

The plan is to implement the same idea using vector IC machinery.
Stubs implementations and scopes modifications are left untouched for now.

Committed: https://crrev.com/14b31970e7a1b6363e72e1b7649c853d9cfe42f6
Cr-Commit-Position: refs/heads/master@{#31458}

[Igor Sheludko, 2015-10-21 15:31:23]

Description was changed from

==========
Remove compilers support for "loads and stores to global vars through property
cell shortcuts installed into parent script context".

The plan is to implement the same idea using vector IC machinery.
Stubs implementations are left untouched for now.
==========

to

==========
Remove compilers' support for "loads and stores to global vars through property
cell shortcuts installed into parent script context".

The plan is to implement the same idea using vector IC machinery.
Stubs implementations and scopes modifications are left untouched for now.
==========

[Igor Sheludko, 2015-10-21 15:52:54]

Patchset #1 (id:1) has been deleted

[Igor Sheludko, 2015-10-21 16:16:36]

ishell@chromium.org changed reviewers:
+ bmeurer@chromium.org, rmcilroy@chromium.org

[Igor Sheludko, 2015-10-21 16:16:36]

Benedikt, PTAL.
Ross, FYI.

[rmcilroy, 2015-10-21 16:52:15]

https://codereview.chromium.org/1419823003/diff/20001/test/cctest/interpreter...
File test/cctest/interpreter/test-bytecode-generator.cc (right):

https://codereview.chromium.org/1419823003/diff/20001/test/cctest/interpreter...
test/cctest/interpreter/test-bytecode-generator.cc:1044: B(Return)              
                              //
This is really a lot more verbose than the LdaGlobal bytecode. What is the plan
going forward for loading via the feedback vector? Will we be able to have a
LdaGlobal type bytecode again which looks up a lot on the feedback vector or
will we have to do LoadIC bytecodes with the global name?

[Igor Sheludko, 2015-10-21 17:08:55]

https://codereview.chromium.org/1419823003/diff/20001/test/cctest/interpreter...
File test/cctest/interpreter/test-bytecode-generator.cc (right):

https://codereview.chromium.org/1419823003/diff/20001/test/cctest/interpreter...
test/cctest/interpreter/test-bytecode-generator.cc:1044: B(Return)              
                              //
On 2015/10/21 16:52:15, rmcilroy wrote:
> This is really a lot more verbose than the LdaGlobal bytecode. What is the
plan
> going forward for loading via the feedback vector? Will we be able to have a
> LdaGlobal type bytecode again which looks up a lot on the feedback vector or
> will we have to do LoadIC bytecodes with the global name?

I totally agree! The plan is to make new kinds of ICs: (Load|Store)GlobalIC
which will receive only two/three parameters: slot, vector and value. In the
fast case the feedback vector will contain a right PropertyCell to read/write
from, and in case of IC miss is will load global object from the context and
name the type feedback metadata (which is a shareable part of the feedback
vector). This approach will work for both declared globals and unallocated free
variables.

Unfortunately, we will proceed only next quarter.

[rmcilroy, 2015-10-21 19:32:39]

On 2015/10/21 17:08:55, Igor Sheludko wrote:
>
https://codereview.chromium.org/1419823003/diff/20001/test/cctest/interpreter...
> File test/cctest/interpreter/test-bytecode-generator.cc (right):
> 
>
https://codereview.chromium.org/1419823003/diff/20001/test/cctest/interpreter...
> test/cctest/interpreter/test-bytecode-generator.cc:1044: B(Return)            
 
>                               //
> On 2015/10/21 16:52:15, rmcilroy wrote:
> > This is really a lot more verbose than the LdaGlobal bytecode. What is the
> plan
> > going forward for loading via the feedback vector? Will we be able to have a
> > LdaGlobal type bytecode again which looks up a lot on the feedback vector or
> > will we have to do LoadIC bytecodes with the global name?
> 
> I totally agree! The plan is to make new kinds of ICs: (Load|Store)GlobalIC
> which will receive only two/three parameters: slot, vector and value. In the
> fast case the feedback vector will contain a right PropertyCell to read/write
> from, and in case of IC miss is will load global object from the context and
> name the type feedback metadata (which is a shareable part of the feedback
> vector). This approach will work for both declared globals and unallocated
free
> variables.
> 
> Unfortunately, we will proceed only next quarter.

Ok, good to know the plan. Given that this is going to be around for a while I
think we need a specific globals bytecode. It should be possible to add
something which loads the global object and calls the load/storeIC in a single
bytecode. I wanted to tweak how named loadic bytecodes work to make them less
verbose, so I can do this at the same time. I'll cook up a CL tomorrow morning
which does this and unifies global and unallocated variable loads / stores and
cc you on it.

[rmcilroy, 2015-10-21 19:32:41]

On 2015/10/21 17:08:55, Igor Sheludko wrote:
>
https://codereview.chromium.org/1419823003/diff/20001/test/cctest/interpreter...
> File test/cctest/interpreter/test-bytecode-generator.cc (right):
> 
>
https://codereview.chromium.org/1419823003/diff/20001/test/cctest/interpreter...
> test/cctest/interpreter/test-bytecode-generator.cc:1044: B(Return)            
 
>                               //
> On 2015/10/21 16:52:15, rmcilroy wrote:
> > This is really a lot more verbose than the LdaGlobal bytecode. What is the
> plan
> > going forward for loading via the feedback vector? Will we be able to have a
> > LdaGlobal type bytecode again which looks up a lot on the feedback vector or
> > will we have to do LoadIC bytecodes with the global name?
> 
> I totally agree! The plan is to make new kinds of ICs: (Load|Store)GlobalIC
> which will receive only two/three parameters: slot, vector and value. In the
> fast case the feedback vector will contain a right PropertyCell to read/write
> from, and in case of IC miss is will load global object from the context and
> name the type feedback metadata (which is a shareable part of the feedback
> vector). This approach will work for both declared globals and unallocated
free
> variables.
> 
> Unfortunately, we will proceed only next quarter.

Ok, good to know the plan. Given that this is going to be around for a while I
think we need a specific globals bytecode. It should be possible to add
something which loads the global object and calls the load/storeIC in a single
bytecode. I wanted to tweak how named loadic bytecodes work to make them less
verbose, so I can do this at the same time. I'll cook up a CL tomorrow morning
which does this and unifies global and unallocated variable loads / stores and
cc you on it.

[Benedikt Meurer, 2015-10-22 05:03:52]

LGTM once comments are addressed.

https://codereview.chromium.org/1419823003/diff/20001/src/compiler/ast-graph-...
File src/compiler/ast-graph-builder.h (right):

https://codereview.chromium.org/1419823003/diff/20001/src/compiler/ast-graph-...
src/compiler/ast-graph-builder.h:304: Node* BuildGlobalLoad(Node* global,
Handle<Name> name,
Don't explicitly pass the global object here, but instead load it from the
context (every JS operator has a context) in generic lowering.

https://codereview.chromium.org/1419823003/diff/20001/src/compiler/ast-graph-...
src/compiler/ast-graph-builder.h:306: Node* BuildGlobalStore(Node* global,
Handle<Name> name, Node* value,
Don't explicitly pass the global object here, but instead load it from the
context (every JS operator has a context) in generic lowering.

https://codereview.chromium.org/1419823003/diff/20001/src/compiler/js-generic...
File src/compiler/js-generic-lowering.cc (right):

https://codereview.chromium.org/1419823003/diff/20001/src/compiler/js-generic...
src/compiler/js-generic-lowering.cc:315: ReplaceWithStubCall(node, callable,
flags);
Insert the global object here, loading it from the context of the node.

https://codereview.chromium.org/1419823003/diff/20001/src/compiler/js-generic...
src/compiler/js-generic-lowering.cc:358: node->InsertInput(zone(), 1,
jsgraph()->HeapConstant(p.name()));
Insert the global object here, loading it from the context of the node.

[Igor Sheludko, 2015-10-22 08:47:57]

Description was changed from

==========
Remove compilers' support for "loads and stores to global vars through property
cell shortcuts installed into parent script context".

The plan is to implement the same idea using vector IC machinery.
Stubs implementations and scopes modifications are left untouched for now.
==========

to

==========
Remove support for "loads and stores to global vars through property cell
shortcuts installed into parent script context" from all compilers.

The plan is to implement the same idea using vector IC machinery.
Stubs implementations and scopes modifications are left untouched for now.
==========

[Igor Sheludko, 2015-10-22 08:48:05]

Patchset #2 (id:40001) has been deleted

[Igor Sheludko, 2015-10-22 08:49:18]

The CQ bit was checked by ishell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-10-22 08:49:31]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1419823003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1419823003/60001

[Igor Sheludko, 2015-10-22 08:49:52]

Thanks!

https://codereview.chromium.org/1419823003/diff/20001/src/compiler/ast-graph-...
File src/compiler/ast-graph-builder.h (right):

https://codereview.chromium.org/1419823003/diff/20001/src/compiler/ast-graph-...
src/compiler/ast-graph-builder.h:304: Node* BuildGlobalLoad(Node* global,
Handle<Name> name,
On 2015/10/22 05:03:52, Benedikt Meurer wrote:
> Don't explicitly pass the global object here, but instead load it from the
> context (every JS operator has a context) in generic lowering.

Done.

https://codereview.chromium.org/1419823003/diff/20001/src/compiler/ast-graph-...
src/compiler/ast-graph-builder.h:306: Node* BuildGlobalStore(Node* global,
Handle<Name> name, Node* value,
On 2015/10/22 05:03:52, Benedikt Meurer wrote:
> Don't explicitly pass the global object here, but instead load it from the
> context (every JS operator has a context) in generic lowering.

Done.

https://codereview.chromium.org/1419823003/diff/20001/src/compiler/js-generic...
File src/compiler/js-generic-lowering.cc (right):

https://codereview.chromium.org/1419823003/diff/20001/src/compiler/js-generic...
src/compiler/js-generic-lowering.cc:315: ReplaceWithStubCall(node, callable,
flags);
On 2015/10/22 05:03:52, Benedikt Meurer wrote:
> Insert the global object here, loading it from the context of the node.

Done.

https://codereview.chromium.org/1419823003/diff/20001/src/compiler/js-generic...
src/compiler/js-generic-lowering.cc:358: node->InsertInput(zone(), 1,
jsgraph()->HeapConstant(p.name()));
On 2015/10/22 05:03:52, Benedikt Meurer wrote:
> Insert the global object here, loading it from the context of the node.

Done.

[commit-bot: I haz the power, 2015-10-22 09:11:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-22 09:11:54]

Dry run: This issue passed the CQ dry run.

[Igor Sheludko, 2015-10-22 09:14:58]

The CQ bit was checked by ishell@chromium.org

[Igor Sheludko, 2015-10-22 09:14:58]

The patchset sent to the CQ was uploaded after l-g-t-m from bmeurer@chromium.org
Link to the patchset: https://codereview.chromium.org/1419823003/#ps60001
(title: "Addressing comments")

[commit-bot: I haz the power, 2015-10-22 09:15:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1419823003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1419823003/60001

[commit-bot: I haz the power, 2015-10-22 09:17:10]

Committed patchset #2 (id:60001)

[commit-bot: I haz the power, 2015-10-22 09:17:34]

Patchset 2 (id:??) landed as
https://crrev.com/14b31970e7a1b6363e72e1b7649c853d9cfe42f6
Cr-Commit-Position: refs/heads/master@{#31458}

[rmcilroy, 2015-10-22 09:18:18]

On 2015/10/21 19:32:41, rmcilroy wrote:
> On 2015/10/21 17:08:55, Igor Sheludko wrote:
> >
>
https://codereview.chromium.org/1419823003/diff/20001/test/cctest/interpreter...
> > File test/cctest/interpreter/test-bytecode-generator.cc (right):
> > 
> >
>
https://codereview.chromium.org/1419823003/diff/20001/test/cctest/interpreter...
> > test/cctest/interpreter/test-bytecode-generator.cc:1044: B(Return)          
 
>  
> >                               //
> > On 2015/10/21 16:52:15, rmcilroy wrote:
> > > This is really a lot more verbose than the LdaGlobal bytecode. What is the
> > plan
> > > going forward for loading via the feedback vector? Will we be able to have
a
> > > LdaGlobal type bytecode again which looks up a lot on the feedback vector
or
> > > will we have to do LoadIC bytecodes with the global name?
> > 
> > I totally agree! The plan is to make new kinds of ICs: (Load|Store)GlobalIC
> > which will receive only two/three parameters: slot, vector and value. In the
> > fast case the feedback vector will contain a right PropertyCell to
read/write
> > from, and in case of IC miss is will load global object from the context and
> > name the type feedback metadata (which is a shareable part of the feedback
> > vector). This approach will work for both declared globals and unallocated
> free
> > variables.
> > 
> > Unfortunately, we will proceed only next quarter.
> 
> Ok, good to know the plan. Given that this is going to be around for a while I
> think we need a specific globals bytecode. It should be possible to add
> something which loads the global object and calls the load/storeIC in a single
> bytecode. I wanted to tweak how named loadic bytecodes work to make them less
> verbose, so I can do this at the same time. I'll cook up a CL tomorrow morning
> which does this and unifies global and unallocated variable loads / stores and
> cc you on it.

OK, I've made the change to unify unallocated and global loads in
https://codereview.chromium.org/1419003002/. Once that CL lands none of the
interpreter changes in this CL should be necessary.

[rmcilroy, 2015-10-22 09:18:53]

On 2015/10/22 09:17:34, commit-bot: I haz the power wrote:
> Patchset 2 (id:??) landed as
> https://crrev.com/14b31970e7a1b6363e72e1b7649c853d9cfe42f6
> Cr-Commit-Position: refs/heads/master@{#31458}

Seriously, you landed this while I still had outstanding comments?