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

Review URL: https://codereview.chromium.org/13408005
Patch from Andy Wingo <wingo@igalia.com>.

git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@14152 ce2b1a6d-e550-0410-aec6-3dcde31c8c00

diff --git a/src/arm/full-codegen-arm.cc b/src/arm/full-codegen-arm.cc
index 6e85bd1..755143d 100644 (file)
--- a/src/arm/full-codegen-arm.cc
+++ b/src/arm/full-codegen-arm.cc
@@ -162,8 +162,6 @@ void FullCodeGenerator::Generate() {
   // the frame (that is done below).
   FrameScope frame_scope(masm_, StackFrame::MANUAL);
 
-  int locals_count = info->scope()->num_stack_slots();
-
   info->set_prologue_offset(masm_->pc_offset());
   {
     PredictableCodeSizeScope predictible_code_size_scope(
@@ -179,6 +177,9 @@ void FullCodeGenerator::Generate() {
   }
 
   { Comment cmnt(masm_, "[ Allocate locals");
+    int locals_count = info->scope()->num_stack_slots();
+    // Generators allocate locals, if any, in context slots.
+    ASSERT(!info->function()->is_generator() || locals_count == 0);
     for (int i = 0; i < locals_count; i++) {
       __ push(ip);
     }

diff --git a/src/ia32/full-codegen-ia32.cc b/src/ia32/full-codegen-ia32.cc
index 5d3a1b9..9b2952b 100644 (file)
--- a/src/ia32/full-codegen-ia32.cc
+++ b/src/ia32/full-codegen-ia32.cc
@@ -164,6 +164,8 @@ void FullCodeGenerator::Generate() {
 
   { Comment cmnt(masm_, "[ Allocate locals");
     int locals_count = info->scope()->num_stack_slots();
+    // Generators allocate locals, if any, in context slots.
+    ASSERT(!info->function()->is_generator() || locals_count == 0);
     if (locals_count == 1) {
       __ push(Immediate(isolate()->factory()->undefined_value()));
     } else if (locals_count > 1) {

diff --git a/src/parser.cc b/src/parser.cc
index 5f80f87..b80e701 100644 (file)
--- a/src/parser.cc
+++ b/src/parser.cc
@@ -4391,6 +4391,9 @@ FunctionLiteral* Parser::ParseFunctionLiteral(Handle<String> function_name,
   // Parse function body.
   { FunctionState function_state(this, scope, is_generator, isolate());
     top_scope_->SetScopeName(function_name);
+    // For generators, allocating variables in contexts is currently a win
+    // because it minimizes the work needed to suspend and resume an activation.
+    if (is_generator) top_scope_->ForceContextAllocation();
 
     //  FormalParameterList ::
     //    '(' (Identifier)*[','] ')'

diff --git a/src/scopes.cc b/src/scopes.cc
index 4ac9d0e..10548f9 100644 (file)
--- a/src/scopes.cc
+++ b/src/scopes.cc
@@ -197,6 +197,8 @@ void Scope::SetDefaults(ScopeType type,
   outer_scope_calls_non_strict_eval_ = false;
   inner_scope_calls_eval_ = false;
   force_eager_compilation_ = false;
+  force_context_allocation_ = (outer_scope != NULL && !is_function_scope())
+      ? outer_scope->has_forced_context_allocation() : false;
   num_var_or_const_ = 0;
   num_stack_slots_ = 0;
   num_heap_slots_ = 0;
@@ -603,12 +605,18 @@ void Scope::CollectStackAndContextLocals(ZoneList<Variable*>* stack_locals,
     }
   }
 
-  // Collect temporaries which are always allocated on the stack.
+  // Collect temporaries which are always allocated on the stack, unless the
+  // context as a whole has forced context allocation.
   for (int i = 0; i < temps_.length(); i++) {
     Variable* var = temps_[i];
     if (var->is_used()) {
-      ASSERT(var->IsStackLocal());
-      stack_locals->Add(var, zone());
+      if (var->IsContextSlot()) {
+        ASSERT(has_forced_context_allocation());
+        context_locals->Add(var, zone());
+      } else {
+        ASSERT(var->IsStackLocal());
+        stack_locals->Add(var, zone());
+      }
     }
   }
 
@@ -1182,8 +1190,11 @@ bool Scope::MustAllocateInContext(Variable* var) {
   // an eval() call or a runtime with lookup), it must be allocated in the
   // context.
   //
-  // Exceptions: temporary variables are never allocated in a context;
-  // catch-bound variables are always allocated in a context.
+  // Exceptions: If the scope as a whole has forced context allocation, all
+  // variables will have context allocation, even temporaries.  Otherwise
+  // temporary variables are always stack-allocated.  Catch-bound variables are
+  // always context-allocated.
+  if (has_forced_context_allocation()) return true;
   if (var->mode() == TEMPORARY) return false;
   if (var->mode() == INTERNAL) return true;
   if (is_catch_scope() || is_block_scope() || is_module_scope()) return true;

diff --git a/src/scopes.h b/src/scopes.h
index 3ca2dcf..66384a1 100644 (file)
--- a/src/scopes.h
+++ b/src/scopes.h
@@ -269,6 +269,15 @@ class Scope: public ZoneObject {
     end_position_ = statement_pos;
   }
 
+  // In some cases we want to force context allocation for a whole scope.
+  void ForceContextAllocation() {
+    ASSERT(!already_resolved());
+    force_context_allocation_ = true;
+  }
+  bool has_forced_context_allocation() const {
+    return force_context_allocation_;
+  }
+
   // ---------------------------------------------------------------------------
   // Predicates.
 
@@ -494,6 +503,7 @@ class Scope: public ZoneObject {
   bool outer_scope_calls_non_strict_eval_;
   bool inner_scope_calls_eval_;
   bool force_eager_compilation_;
+  bool force_context_allocation_;
 
   // True if it doesn't need scope resolution (e.g., if the scope was
   // constructed based on a serialized scope info or a catch context).

diff --git a/src/v8globals.h b/src/v8globals.h
index 4b4d312..588d592 100644 (file)
--- a/src/v8globals.h
+++ b/src/v8globals.h
@@ -496,8 +496,8 @@ enum VariableMode {
   INTERNAL,        // like VAR, but not user-visible (may or may not
                    // be in a context)
 
-  TEMPORARY,       // temporary variables (not user-visible), never
-                   // in a context
+  TEMPORARY,       // temporary variables (not user-visible), stack-allocated
+                   // unless the scope as a whole has forced context allocation
 
   DYNAMIC,         // always require dynamic lookup (we don't know
                    // the declaration)

diff --git a/src/x64/full-codegen-x64.cc b/src/x64/full-codegen-x64.cc
index 93452e5..83706ad 100644 (file)
--- a/src/x64/full-codegen-x64.cc
+++ b/src/x64/full-codegen-x64.cc
@@ -160,6 +160,8 @@ void FullCodeGenerator::Generate() {
 
   { Comment cmnt(masm_, "[ Allocate locals");
     int locals_count = info->scope()->num_stack_slots();
+    // Generators allocate locals, if any, in context slots.
+    ASSERT(!info->function()->is_generator() || locals_count == 0);
     if (locals_count == 1) {
       __ PushRoot(Heap::kUndefinedValueRootIndex);
     } else if (locals_count > 1) {

diff --git a/test/mjsunit/harmony/generators-instantiation.js b/test/mjsunit/harmony/generators-instantiation.js
new file mode 100644 (file)
index 0000000..1255f97
--- /dev/null
+++ b/test/mjsunit/harmony/generators-instantiation.js
@@ -0,0 +1,49 @@
+// Copyright 2013 the V8 project authors. All rights reserved.
+// Redistribution and use in source and binary forms, with or without
+// modification, are permitted provided that the following conditions are
+// met:
+//
+//     * Redistributions of source code must retain the above copyright
+//       notice, this list of conditions and the following disclaimer.
+//     * Redistributions in binary form must reproduce the above
+//       copyright notice, this list of conditions and the following
+//       disclaimer in the documentation and/or other materials provided
+//       with the distribution.
+//     * Neither the name of Google Inc. nor the names of its
+//       contributors may be used to endorse or promote products derived
+//       from this software without specific prior written permission.
+//
+// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+// "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+// LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+// A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+// OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+// LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+// DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+// THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+// (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+
+// Flags: --harmony-generators --harmony-scoping
+
+// Test instantations of generators.
+
+// Generators shouldn't allocate stack slots.  This test will abort in debug
+// mode if generators have stack slots.
+function TestContextAllocation() {
+  function* g1(a, b, c) { yield 1; return [a, b, c]; }
+  function* g2() { yield 1; return arguments; }
+  function* g3() { yield 1; return this; }
+  function* g4() { var x = 10; yield 1; return x; }
+  // Temporary variable context allocation
+  function* g5(l) { "use strict"; yield 1; for (let x in l) { yield x; } }
+
+  g1();
+  g2();
+  g3();
+  g4();
+  g5(["foo"]);
+}
+
+TestContextAllocation();