Force context allocation for variables in generator scopes.

Force context allocation for variables in generator scopes.

* src/scopes.h (ForceContextAllocation, has_forced_context_allocation):
  New interface to force context allocation for an entire function's
  scope.

* src/scopes.cc: Unless a new scope is a function scope, if its outer
  scope has forced context allocation, it should also force context
  allocation.
  (MustAllocateInContext): Return true if the scope as a whole has
  forced context allocation.
  (CollectStackAndContextLocals): Allow temporaries to be
  context-allocated.

* src/parser.cc (ParseFunctionLiteral): Force context allocation for
  generator scopes.

* src/v8globals.h (VariableMode): Update comment on TEMPORARY.

* src/arm/full-codegen-arm.cc (Generate):
* src/ia32/full-codegen-ia32.cc (Generate):
* src/x64/full-codegen-x64.cc (Generate): Assert that generators have no
  stack slots.

* test/mjsunit/harmony/generators-instantiation.js: New test.

BUG=v8:2355
TEST=mjsunit/harmony/generators-instantiation

Committed: http://code.google.com/p/v8/source/detail?r=14152

[Michael Starzinger, 2013-04-05 11:38:42]

https://codereview.chromium.org/13408005/diff/1/src/arm/full-codegen-arm.cc
File src/arm/full-codegen-arm.cc (right):

https://codereview.chromium.org/13408005/diff/1/src/arm/full-codegen-arm.cc#n...
src/arm/full-codegen-arm.cc:165: int locals_count =
info->scope()->num_stack_slots();
nit: I know this is not part of your change, but can we move down the
"locals_count" variable to where it's used, so that the code is in sync with
ia32 and x64?

https://codereview.chromium.org/13408005/diff/1/src/ia32/full-codegen-ia32.cc
File src/ia32/full-codegen-ia32.cc (right):

https://codereview.chromium.org/13408005/diff/1/src/ia32/full-codegen-ia32.cc...
src/ia32/full-codegen-ia32.cc:168: ASSERT(!(info->function()->is_generator() &&
locals_count));
nit: Can we rewrite this as "!info->function()->is_generator() || locals_count
== 0", I think that's easier to read. This also applies to the other
architectures.

https://codereview.chromium.org/13408005/diff/1/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/13408005/diff/1/src/parser.cc#newcode4395
src/parser.cc:4395: if (is_generator)
style: Either the whole conditional on one line or add curly braces around the
condition body.

https://codereview.chromium.org/13408005/diff/1/src/scopes.cc
File src/scopes.cc (right):

https://codereview.chromium.org/13408005/diff/1/src/scopes.cc#newcode122
src/scopes.cc:122: if (!is_function_scope() &&
I would feel more comfortable if we could move this into SetDefaults(), even
though it is already correct here AFAICT. Something like the following there ...

force_context_allocation _ = (outer_scope != NULL && is_function_scope())
    ? outer_scope->has_forced_context_allocation() : false;

https://codereview.chromium.org/13408005/diff/1/src/scopes.h
File src/scopes.h (right):

https://codereview.chromium.org/13408005/diff/1/src/scopes.h#newcode273
src/scopes.h:273: // example in generators, allocating variables in contexts
minimizes the work
Can we move the second sentence of this comment to parser.cc where the context
allocation for generators is forced? Also I would rephrase it to "For now
generators ..." because we might want to change that in the (distant) future
when we add Crankshaft support for generators.

https://codereview.chromium.org/13408005/diff/1/src/v8globals.h
File src/v8globals.h (right):

https://codereview.chromium.org/13408005/diff/1/src/v8globals.h#newcode500
src/v8globals.h:500: // stack-allocated
nit: Let's add "... (unless the scope forces context allocation)"

https://codereview.chromium.org/13408005/diff/1/test/mjsunit/harmony/generato...
File test/mjsunit/harmony/generators-iteration.js (right):

https://codereview.chromium.org/13408005/diff/1/test/mjsunit/harmony/generato...
test/mjsunit/harmony/generators-iteration.js:30: // Test instantations of
generators.
This test doesn't really test iteration, so I think the filename is misleading.
Can we rename this file to "generators-instance.js" or something like that?

[wingo, 2013-04-05 12:25:06]

https://codereview.chromium.org/13408005/diff/1/src/arm/full-codegen-arm.cc
File src/arm/full-codegen-arm.cc (right):

https://codereview.chromium.org/13408005/diff/1/src/arm/full-codegen-arm.cc#n...
src/arm/full-codegen-arm.cc:165: int locals_count =
info->scope()->num_stack_slots();
On 2013/04/05 11:38:42, Michael Starzinger wrote:
> nit: I know this is not part of your change, but can we move down the
> "locals_count" variable to where it's used, so that the code is in sync with
> ia32 and x64?

Done.

https://codereview.chromium.org/13408005/diff/1/src/ia32/full-codegen-ia32.cc
File src/ia32/full-codegen-ia32.cc (right):

https://codereview.chromium.org/13408005/diff/1/src/ia32/full-codegen-ia32.cc...
src/ia32/full-codegen-ia32.cc:168: ASSERT(!(info->function()->is_generator() &&
locals_count));
On 2013/04/05 11:38:42, Michael Starzinger wrote:
> nit: Can we rewrite this as "!info->function()->is_generator() || locals_count
> == 0", I think that's easier to read. This also applies to the other
> architectures.

Done.

https://codereview.chromium.org/13408005/diff/1/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/13408005/diff/1/src/parser.cc#newcode4395
src/parser.cc:4395: if (is_generator)
On 2013/04/05 11:38:42, Michael Starzinger wrote:
> style: Either the whole conditional on one line or add curly braces around the
> condition body.

Done.

https://codereview.chromium.org/13408005/diff/1/src/scopes.cc
File src/scopes.cc (right):

https://codereview.chromium.org/13408005/diff/1/src/scopes.cc#newcode122
src/scopes.cc:122: if (!is_function_scope() &&
On 2013/04/05 11:38:42, Michael Starzinger wrote:
> I would feel more comfortable if we could move this into SetDefaults(), even
> though it is already correct here AFAICT. Something like the following there
...
> 
> force_context_allocation _ = (outer_scope != NULL && is_function_scope())
>     ? outer_scope->has_forced_context_allocation() : false;

Done.  I assume you meant !is_function_scope() here.

https://codereview.chromium.org/13408005/diff/1/src/scopes.h
File src/scopes.h (right):

https://codereview.chromium.org/13408005/diff/1/src/scopes.h#newcode273
src/scopes.h:273: // example in generators, allocating variables in contexts
minimizes the work
On 2013/04/05 11:38:42, Michael Starzinger wrote:
> Can we move the second sentence of this comment to parser.cc where the context
> allocation for generators is forced? Also I would rephrase it to "For now
> generators ..." because we might want to change that in the (distant) future
> when we add Crankshaft support for generators.

Done.

https://codereview.chromium.org/13408005/diff/1/src/v8globals.h
File src/v8globals.h (right):

https://codereview.chromium.org/13408005/diff/1/src/v8globals.h#newcode500
src/v8globals.h:500: // stack-allocated
On 2013/04/05 11:38:42, Michael Starzinger wrote:
> nit: Let's add "... (unless the scope forces context allocation)"

Done.

https://codereview.chromium.org/13408005/diff/1/test/mjsunit/harmony/generato...
File test/mjsunit/harmony/generators-iteration.js (right):

https://codereview.chromium.org/13408005/diff/1/test/mjsunit/harmony/generato...
test/mjsunit/harmony/generators-iteration.js:30: // Test instantations of
generators.
On 2013/04/05 11:38:42, Michael Starzinger wrote:
> This test doesn't really test iteration, so I think the filename is
misleading.
> Can we rename this file to "generators-instance.js" or something like that?

Changed to generators-instantiation.js, if that works for you.

[Michael Starzinger, 2013-04-05 13:06:56]

LGTM. I'll land this.

https://codereview.chromium.org/13408005/diff/1/src/scopes.cc
File src/scopes.cc (right):

https://codereview.chromium.org/13408005/diff/1/src/scopes.cc#newcode122
src/scopes.cc:122: if (!is_function_scope() &&
On 2013/04/05 12:25:06, wingo wrote:
> On 2013/04/05 11:38:42, Michael Starzinger wrote:
> > I would feel more comfortable if we could move this into SetDefaults(), even
> > though it is already correct here AFAICT. Something like the following there
> ...
> > 
> > force_context_allocation _ = (outer_scope != NULL && is_function_scope())
> >     ? outer_scope->has_forced_context_allocation() : false;
> 
> Done.  I assume you meant !is_function_scope() here.

Yep, that was a typo on my end.

https://codereview.chromium.org/13408005/diff/1/test/mjsunit/harmony/generato...
File test/mjsunit/harmony/generators-iteration.js (right):

https://codereview.chromium.org/13408005/diff/1/test/mjsunit/harmony/generato...
test/mjsunit/harmony/generators-iteration.js:30: // Test instantations of
generators.
On 2013/04/05 12:25:06, wingo wrote:
> On 2013/04/05 11:38:42, Michael Starzinger wrote:
> > This test doesn't really test iteration, so I think the filename is
> misleading.
> > Can we rename this file to "generators-instance.js" or something like that?
> 
> Changed to generators-instantiation.js, if that works for you.

Yep, fine with me.

[Michael Starzinger, 2013-04-05 13:19:47]

Committed patchset #2 manually as r14152 (presubmit successful).