[Interpreter] Add support for local context loads and stores.

[Interpreter] Add support for local context loads and stores.

Adds support for local context loads and stores. Also adds support for
creation of new block contexts (e.g., for let variables) and initializing
const / let variables with the hole appropriately.

Also adds some checks to ensure BytecodeArrayBuilder::context_count is set
appropriately and fixes tests to do so.

Adds the bytecode StaContextSlot.

BUG=v8:4280
LOG=N

Committed: https://crrev.com/2c8340dac438c317f6a66a45886aa25bd23bd351
Cr-Commit-Position: refs/heads/master@{#31343}

[rmcilroy, 2015-10-15 13:23:30]

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

[commit-bot: I haz the power, 2015-10-15 13:23:41]

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

[commit-bot: I haz the power, 2015-10-15 13:24:35]

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

[commit-bot: I haz the power, 2015-10-15 13:24:36]

Dry run: Try jobs failed on following builders:
  v8_android_arm_compile_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_android_arm_compile_rel/...)
  v8_linux64_asan_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel/builds/...)
  v8_linux_mips64el_compile_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)
  v8_linux_mipsel_compile_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)
  v8_linux_nodcheck_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel/build...)
  v8_linux_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel/builds/10584)
  v8_mac_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel/builds/10721)

[rmcilroy, 2015-10-15 13:56:32]

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

[rmcilroy, 2015-10-15 13:56:45]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2015-10-15 13:56:46]

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

[rmcilroy, 2015-10-15 14:04:20]

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

[rmcilroy, 2015-10-15 14:04:21]

PTAL, thanks.

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

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

[commit-bot: I haz the power, 2015-10-15 15:46:23]

Dry run: Try jobs failed on following builders:
  v8_win_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel/builds/10662)

[Michael Starzinger, 2015-10-16 09:10:04]

LGTM with comments.

https://codereview.chromium.org/1403943004/diff/20001/src/interpreter/bytecod...
File src/interpreter/bytecode-array-builder.cc (right):

https://codereview.chromium.org/1403943004/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.cc:86: parameter_count_, constant_pool);
nit: If we use parameter_count() here, then the DCHECK at the top wouldn't be
needed anymore.

https://codereview.chromium.org/1403943004/diff/20001/src/interpreter/bytecod...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/1403943004/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:42: if (outer_ && !is_function_context_) {
The semantics of the "is_function_context" flags actually means
"does_not_require_pop". Can we rename it in that regard, potentially even
negating the flag to "requires_pop"?

https://codereview.chromium.org/1403943004/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:734: // Evaluate all the non-constant
subexpressions and to store them into the
nit: Drop the "to" in this sentence.

https://codereview.chromium.org/1403943004/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:1225:
.LoadAccumulatorWithRegister(Register::function_closure())
For script (or any other top level code) the passed closure needs to be replaced
with Smi(0). The runtime function will then canonicalize the the closure to be
the canonical empty function instead of the anonymous closure that is used to
compile the top level code.

This is ugly as hell, but I haven't found a cleaner solution yet. Details about
this can be found in AstGraphBuilder::GetFunctionClosureForContext or in
FullCodeGenerator::PushFunctionArgumentForContextAllocation.

Nothing actionable right now, but better leave TODO in that regard.

[oth, 2015-10-16 09:30:44]

lgtm modulo comments. Thanks!

https://codereview.chromium.org/1403943004/diff/20001/src/interpreter/bytecod...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/1403943004/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:225: bool hole_init = mode == CONST ||
mode == CONST_LEGACY || mode == LET;
A comment would be helpful explaining the purpose of the hole here.

https://codereview.chromium.org/1403943004/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:1333: if (previous ==
Register::function_context()) {
Comments here explaining the logic for the returned register choices would be
helpful.

https://codereview.chromium.org/1403943004/diff/20001/src/interpreter/bytecod...
File src/interpreter/bytecode-generator.h (right):

https://codereview.chromium.org/1403943004/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.h:70: inline ControlScope*
execution_control() const { return execution_control_; }
Previously the name gave a hint to the likely type. Now it's unrelated. 

Would be better to sed -i s/ControlScope/ExecutionControl/g and
s/ContextScope/ExecutionContext/g or leave as-is.

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

https://codereview.chromium.org/1403943004/diff/20001/test/cctest/interpreter...
test/cctest/interpreter/test-bytecode-generator.cc:2764: B(LdaSmi8), U8(1),     
                                       //
Consecutive loads to the accumulator?

[rmcilroy, 2015-10-16 11:19:04]

Review comments addressed and also added support for context allocated
parameters with some tests, PTAL.

https://codereview.chromium.org/1403943004/diff/20001/src/interpreter/bytecod...
File src/interpreter/bytecode-array-builder.cc (right):

https://codereview.chromium.org/1403943004/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.cc:86: parameter_count_, constant_pool);
On 2015/10/16 09:10:04, Michael Starzinger wrote:
> nit: If we use parameter_count() here, then the DCHECK at the top wouldn't be
> needed anymore.

Done. (also done in ::Parameter())

https://codereview.chromium.org/1403943004/diff/20001/src/interpreter/bytecod...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/1403943004/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:42: if (outer_ && !is_function_context_) {
On 2015/10/16 09:10:04, Michael Starzinger wrote:
> The semantics of the "is_function_context" flags actually means
> "does_not_require_pop". Can we rename it in that regard, potentially even
> negating the flag to "requires_pop"?

Done, but named it should_pop_context.

https://codereview.chromium.org/1403943004/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:225: bool hole_init = mode == CONST ||
mode == CONST_LEGACY || mode == LET;
On 2015/10/16 09:30:43, oth wrote:
> A comment would be helpful explaining the purpose of the hole here.

Done.

https://codereview.chromium.org/1403943004/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:734: // Evaluate all the non-constant
subexpressions and to store them into the
On 2015/10/16 09:10:04, Michael Starzinger wrote:
> nit: Drop the "to" in this sentence.

Done.

https://codereview.chromium.org/1403943004/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:1225:
.LoadAccumulatorWithRegister(Register::function_closure())
On 2015/10/16 09:10:04, Michael Starzinger wrote:
> For script (or any other top level code) the passed closure needs to be
replaced
> with Smi(0). The runtime function will then canonicalize the the closure to be
> the canonical empty function instead of the anonymous closure that is used to
> compile the top level code.
> 
> This is ugly as hell, but I haven't found a cleaner solution yet. Details
about
> this can be found in AstGraphBuilder::GetFunctionClosureForContext or in
> FullCodeGenerator::PushFunctionArgumentForContextAllocation.
> 
> Nothing actionable right now, but better leave TODO in that regard.

Ahh I never spotted this, thanks for the hint. I've added a
VisitFunctionClosureForContext function which should do the right think now.

https://codereview.chromium.org/1403943004/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:1333: if (previous ==
Register::function_context()) {
On 2015/10/16 09:30:43, oth wrote:
> Comments here explaining the logic for the returned register choices would be
> helpful.

Done.

https://codereview.chromium.org/1403943004/diff/20001/src/interpreter/bytecod...
File src/interpreter/bytecode-generator.h (right):

https://codereview.chromium.org/1403943004/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.h:70: inline ControlScope*
execution_control() const { return execution_control_; }
On 2015/10/16 09:30:43, oth wrote:
> Previously the name gave a hint to the likely type. Now it's unrelated. 
> 
> Would be better to sed -i s/ControlScope/ExecutionControl/g and
> s/ContextScope/ExecutionContext/g or leave as-is.

I was going with consistency with AstGraphBuilder which has them named this way.
I think the type should have Scope in it to make it obvious the type has scoped
behaviour, but this isn't necessary when you want to get the current control
object, so execution_control makes sense there. Personally I don't think it's
necessary that the getter is called the same as the type (this is often the
case, e.g., globals() returns ZoneVector<Handle<Object>>*) happy to chat if you
feel strongly about this though.

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

https://codereview.chromium.org/1403943004/diff/20001/test/cctest/interpreter...
test/cctest/interpreter/test-bytecode-generator.cc:2764: B(LdaSmi8), U8(1),     
                                       //
On 2015/10/16 09:30:43, oth wrote:
> Consecutive loads to the accumulator?

This is because of the 'use strict'; which emits the constant load but doesn't
do anything with that constant. There are TODO comments in PropertyLoads - I'm
hoping your visitForEffect will come to the rescue here and we can omit
non-effectful operations in that execution scope.

[rmcilroy, 2015-10-16 11:26:37]

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

[commit-bot: I haz the power, 2015-10-16 11:26:42]

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

[commit-bot: I haz the power, 2015-10-16 12:21:01]

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

[commit-bot: I haz the power, 2015-10-16 12:21:02]

Dry run: This issue passed the CQ dry run.

[oth, 2015-10-16 13:12:36]

Thanks, looks good here.

[Michael Starzinger, 2015-10-16 13:29:06]

Yep, still looking good.

https://codereview.chromium.org/1403943004/diff/20001/src/interpreter/bytecod...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/1403943004/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:42: if (outer_ && !is_function_context_) {
On 2015/10/16 11:19:04, rmcilroy wrote:
> On 2015/10/16 09:10:04, Michael Starzinger wrote:
> > The semantics of the "is_function_context" flags actually means
> > "does_not_require_pop". Can we rename it in that regard, potentially even
> > negating the flag to "requires_pop"?
> 
> Done, but named it should_pop_context.

Acknowledged. Works for me.

https://codereview.chromium.org/1403943004/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:1225:
.LoadAccumulatorWithRegister(Register::function_closure())
On 2015/10/16 11:19:04, rmcilroy wrote:
> On 2015/10/16 09:10:04, Michael Starzinger wrote:
> > For script (or any other top level code) the passed closure needs to be
> replaced
> > with Smi(0). The runtime function will then canonicalize the the closure to
be
> > the canonical empty function instead of the anonymous closure that is used
to
> > compile the top level code.
> > 
> > This is ugly as hell, but I haven't found a cleaner solution yet. Details
> about
> > this can be found in AstGraphBuilder::GetFunctionClosureForContext or in
> > FullCodeGenerator::PushFunctionArgumentForContextAllocation.
> > 
> > Nothing actionable right now, but better leave TODO in that regard.
> 
> Ahh I never spotted this, thanks for the hint. I've added a
> VisitFunctionClosureForContext function which should do the right think now.

Acknowledged.

https://codereview.chromium.org/1403943004/diff/40001/src/interpreter/bytecod...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/1403943004/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:42: if (should_pop_context_) {
nit: Let's be defensive and keep checking "outer_ && should_pop_context_" here.

[rmcilroy, 2015-10-16 14:11:35]

https://codereview.chromium.org/1403943004/diff/40001/src/interpreter/bytecod...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/1403943004/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:42: if (should_pop_context_) {
On 2015/10/16 13:29:06, Michael Starzinger wrote:
> nit: Let's be defensive and keep checking "outer_ && should_pop_context_"
here.

Ok, done.

[rmcilroy, 2015-10-16 14:11:42]

The CQ bit was checked by rmcilroy@chromium.org

[rmcilroy, 2015-10-16 14:11:43]

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/1403943004/#ps60001
(title: "Add back outer_ &&]")

[commit-bot: I haz the power, 2015-10-16 14:11:47]

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

[commit-bot: I haz the power, 2015-10-16 15:29:10]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2015-10-16 15:29:27]

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