[Interpreter] Add support for keyed load / store ICs and named store IC to

[Interpreter] Add support for keyed load / store ICs and named store IC to
bytecode graph builder

Adds implementation and tests for KeyedLoadIC, KeyedStoreIC and StoreIC to
bytecode graph builder.

BUG=v8:4280
LOG=N

Committed: https://crrev.com/4bb6e7c8c0794c638763b6ed835e6508a582a1dc
Cr-Commit-Position: refs/heads/master@{#32116}

[mythria, 2015-11-16 14:45:54]

mythria@chromium.org changed reviewers:
+ mstarzinger@chromium.org, oth@chromium.org

[mythria, 2015-11-16 14:45:54]

Could you please review my changes. This builds on an earlier cl that implements
loadICs and follows a similar approach.
 
Michi, Please let me know if you are busy, I will redirect it to someone else
may be Benedikt. 

Thanks,
Mythri

[oth, 2015-11-16 16:39:32]

lgtm. Please fix the title though (s/sotre/store/).

Thanks!

[mythria, 2015-11-18 08:34:24]

Description was changed from

==========
[Interpreter] Add support for  keyed load / sotre ICs and named store IC to
bytecode graph builder

Adds implementation and tests for KeyedLoadIC, KeyedStoreIC and StoreIC to
bytecode graph builder.

BUG=v8:4280
LOG=N
==========

to

==========
[Interpreter] Add support for  keyed load / store ICs and named store IC to
bytecode graph builder

Adds implementation and tests for KeyedLoadIC, KeyedStoreIC and StoreIC to
bytecode graph builder.

BUG=v8:4280
LOG=N
==========

[mythria, 2015-11-18 16:13:10]

Fixed a bug similar to https://codereview.chromium.org/1453973004/.

Thanks,
Mythri

[oth, 2015-11-18 16:17:12]

Description was changed from

==========
[Interpreter] Add support for  keyed load / store ICs and named store IC to
bytecode graph builder

Adds implementation and tests for KeyedLoadIC, KeyedStoreIC and StoreIC to
bytecode graph builder.

BUG=v8:4280
LOG=N
==========

to

==========
[Interpreter] Add support for keyed load / store ICs and named store IC to
bytecode graph builder

Adds implementation and tests for KeyedLoadIC, KeyedStoreIC and StoreIC to
bytecode graph builder.

BUG=v8:4280
LOG=N
==========

[oth, 2015-11-18 16:17:40]

On 2015/11/18 16:13:10, mythria wrote:
> Fixed a bug similar to https://codereview.chromium.org/1453973004/.
> 
> Thanks,
> Mythri

Thanks, lgtm.

[Michael Starzinger, 2015-11-19 11:59:38]

LGTM. Just one comment.

https://codereview.chromium.org/1448913002/diff/40001/src/compiler/bytecode-g...
File src/compiler/bytecode-graph-builder.cc (right):

https://codereview.chromium.org/1448913002/diff/40001/src/compiler/bytecode-g...
src/compiler/bytecode-graph-builder.cc:530:
environment()->BindAccumulator(node);
The JSStoreNamed operator doesn't produce value outputs. Should the completion
value actually be bound to "value" instead? Or am I missing something?

https://codereview.chromium.org/1448913002/diff/40001/src/compiler/bytecode-g...
src/compiler/bytecode-graph-builder.cc:572:
environment()->BindAccumulator(node);
Likewise for s/node/value/ here?

[mythria, 2015-11-19 12:41:24]

Thank you for the review. I fixed the bug with return value of stores.

https://codereview.chromium.org/1448913002/diff/40001/src/compiler/bytecode-g...
File src/compiler/bytecode-graph-builder.cc (right):

https://codereview.chromium.org/1448913002/diff/40001/src/compiler/bytecode-g...
src/compiler/bytecode-graph-builder.cc:530:
environment()->BindAccumulator(node);
On 2015/11/19 11:59:38, Michael Starzinger wrote:
> The JSStoreNamed operator doesn't produce value outputs. Should the completion
> value actually be bound to "value" instead? Or am I missing something?

Done.

https://codereview.chromium.org/1448913002/diff/40001/src/compiler/bytecode-g...
src/compiler/bytecode-graph-builder.cc:530:
environment()->BindAccumulator(node);
On 2015/11/19 11:59:38, Michael Starzinger wrote:
> The JSStoreNamed operator doesn't produce value outputs. Should the completion
> value actually be bound to "value" instead? Or am I missing something?

Thanks for pointing this. Stores, do not produce a value. Accumulator should be
bound to value. Since the value is already in the accumulator, we can just omit
this. I also added tests to actually test the returned value.

https://codereview.chromium.org/1448913002/diff/40001/src/compiler/bytecode-g...
src/compiler/bytecode-graph-builder.cc:572:
environment()->BindAccumulator(node);
On 2015/11/19 11:59:38, Michael Starzinger wrote:
> Likewise for s/node/value/ here?

Done.

[Michael Starzinger, 2015-11-19 12:44:05]

https://codereview.chromium.org/1448913002/diff/40001/src/compiler/bytecode-g...
File src/compiler/bytecode-graph-builder.cc (right):

https://codereview.chromium.org/1448913002/diff/40001/src/compiler/bytecode-g...
src/compiler/bytecode-graph-builder.cc:530:
environment()->BindAccumulator(node);
On 2015/11/19 12:41:24, mythria wrote:
> On 2015/11/19 11:59:38, Michael Starzinger wrote:
> > The JSStoreNamed operator doesn't produce value outputs. Should the
completion
> > value actually be bound to "value" instead? Or am I missing something?
> 
> Thanks for pointing this. Stores, do not produce a value. Accumulator should
be
> bound to value. Since the value is already in the accumulator, we can just
omit
> this. I also added tests to actually test the returned value. 

I would vote for being explicit about the accumulator binding. But it's up to
you.

[mythria, 2015-11-19 13:24:06]

https://codereview.chromium.org/1448913002/diff/40001/src/compiler/bytecode-g...
File src/compiler/bytecode-graph-builder.cc (right):

https://codereview.chromium.org/1448913002/diff/40001/src/compiler/bytecode-g...
src/compiler/bytecode-graph-builder.cc:530:
environment()->BindAccumulator(node);
On 2015/11/19 12:44:05, Michael Starzinger wrote:
> On 2015/11/19 12:41:24, mythria wrote:
> > On 2015/11/19 11:59:38, Michael Starzinger wrote:
> > > The JSStoreNamed operator doesn't produce value outputs. Should the
> completion
> > > value actually be bound to "value" instead? Or am I missing something?
> > 
> > Thanks for pointing this. Stores, do not produce a value. Accumulator should
> be
> > bound to value. Since the value is already in the accumulator, we can just
> omit
> > this. I also added tests to actually test the returned value. 
> 
> I would vote for being explicit about the accumulator binding. But it's up to
> you.

I Made it explicit. 
Done.

[mythria, 2015-11-19 14:15:15]

The CQ bit was checked by mythria@chromium.org

[mythria, 2015-11-19 14:15:15]

The patchset sent to the CQ was uploaded after l-g-t-m from oth@chromium.org,
mstarzinger@chromium.org
Link to the patchset: https://codereview.chromium.org/1448913002/#ps80001
(title: "Made the binding of return value of the store explicit.")

[commit-bot: I haz the power, 2015-11-19 14:15:19]

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

[commit-bot: I haz the power, 2015-11-19 14:16:39]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-11-19 14:17:01]

Patchset 5 (id:??) landed as
https://crrev.com/4bb6e7c8c0794c638763b6ed835e6508a582a1dc
Cr-Commit-Position: refs/heads/master@{#32116}