[Interpreter] Add support for context slot loads / stores to BytecodeGraphBuilder.

[Interpreter] Add support for context slot loads / stores to BytecodeGraphBuilder.

Adds implementation and tests for LdaContextSlot, StaeContextSlot, PushContext,
and PopContext to bytecode graph builder

BUG=v8:4280
LOG=N

Committed: https://crrev.com/128e75cbd956d20118fc4cd18e4e57e7db4d16fb
Cr-Commit-Position: refs/heads/master@{#32474}

[mythria, 2015-12-01 15:00:00]

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

[mythria, 2015-12-01 15:00:00]

I added support for context slot loads and stores to bytecode graph builder.
Could you please review my changes.

https://codereview.chromium.org/1489863002/diff/1/src/compiler/bytecode-graph...
File src/compiler/bytecode-graph-builder.cc (right):

https://codereview.chromium.org/1489863002/diff/1/src/compiler/bytecode-graph...
src/compiler/bytecode-graph-builder.cc:228: CreateGraphBody(stack_check);
Function contexts are built when we generate bytecode in bytecode-generator. If
a new context is required bytecode generator builds it and generates PushContext
bytecode. So, I thought no additional handling is required here.

https://codereview.chromium.org/1489863002/diff/1/src/compiler/bytecode-graph...
src/compiler/bytecode-graph-builder.cc:468: javascript()->LoadContext(0,
iterator.GetIndexOperand(1), false);
I set the immutable flag to false. If it is not safe to set it to false, I will
add another bytecode LoadImmutableContextSlots that will be called for immutable
objects. So that I can set it to correct value.

[Michael Starzinger, 2015-12-01 15:54:57]

LGTM.

https://codereview.chromium.org/1489863002/diff/1/src/compiler/bytecode-graph...
File src/compiler/bytecode-graph-builder.cc (right):

https://codereview.chromium.org/1489863002/diff/1/src/compiler/bytecode-graph...
src/compiler/bytecode-graph-builder.cc:228: CreateGraphBody(stack_check);
On 2015/12/01 15:00:00, mythria wrote:
> Function contexts are built when we generate bytecode in bytecode-generator.
If
> a new context is required bytecode generator builds it and generates
PushContext
> bytecode. So, I thought no additional handling is required here. 

Acknowledged.

https://codereview.chromium.org/1489863002/diff/1/src/compiler/bytecode-graph...
src/compiler/bytecode-graph-builder.cc:468: javascript()->LoadContext(0,
iterator.GetIndexOperand(1), false);
On 2015/12/01 15:00:00, mythria wrote:
> I set the immutable flag to false. If it is not safe to set it to false, I
will
> add another bytecode LoadImmutableContextSlots that will be called for
immutable
> objects. So that I can set it to correct value. 

Acknowledged. Yes, using "false" here is a safe default.

https://codereview.chromium.org/1489863002/diff/1/test/cctest/compiler/test-r...
File test/cctest/compiler/test-run-bytecode-graph-builder.cc (right):

https://codereview.chromium.org/1489863002/diff/1/test/cctest/compiler/test-r...
test/cctest/compiler/test-run-bytecode-graph-builder.cc:1083: "     return new
Inner(2).innerFunc;"
nit: s/2/3/ so that we recognize when the wrong slot would be chosen.

[oth, 2015-12-01 15:58:52]

lgtm

[mythria, 2015-12-01 16:12:12]

Thanks for your reviews.

https://codereview.chromium.org/1489863002/diff/1/test/cctest/compiler/test-r...
File test/cctest/compiler/test-run-bytecode-graph-builder.cc (right):

https://codereview.chromium.org/1489863002/diff/1/test/cctest/compiler/test-r...
test/cctest/compiler/test-run-bytecode-graph-builder.cc:1083: "     return new
Inner(2).innerFunc;"
On 2015/12/01 15:54:57, Michael Starzinger wrote:
> nit: s/2/3/ so that we recognize when the wrong slot would be chosen.

Done.

[mythria, 2015-12-01 17:08:25]

The CQ bit was checked by mythria@chromium.org

[mythria, 2015-12-01 17:08:25]

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/1489863002/#ps20001
(title: "Addressed comments.")

[commit-bot: I haz the power, 2015-12-01 17:08:35]

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

[commit-bot: I haz the power, 2015-12-01 17:28:49]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-12-01 17:33:08]

Description was changed from

==========
[Interpreter] Add support for context slot loads / stores to
BytecodeGraphBuilder.

Adds implementation and tests for LdaContextSlot, StaeContextSlot, PushContext,
and PopContext to bytecode graph builder

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

to

==========
[Interpreter] Add support for context slot loads / stores to
BytecodeGraphBuilder.

Adds implementation and tests for LdaContextSlot, StaeContextSlot, PushContext,
and PopContext to bytecode graph builder

BUG=v8:4280
LOG=N

Committed: https://crrev.com/128e75cbd956d20118fc4cd18e4e57e7db4d16fb
Cr-Commit-Position: refs/heads/master@{#32474}
==========

[commit-bot: I haz the power, 2015-12-01 17:33:09]

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