First version of the new generators implementation.

src/interpreter/bytecode-generator.cc

Index: src/interpreter/bytecode-generator.cc
diff --git a/src/interpreter/bytecode-generator.cc b/src/interpreter/bytecode-generator.cc
index 0227f13b146454a347e623159b6606ce2325bc14..a488fc04b1272032a21a20d7aa7e57e338e8570e 100644
--- a/src/interpreter/bytecode-generator.cc
+++ b/src/interpreter/bytecode-generator.cc
@@ -565,8 +565,10 @@ BytecodeGenerator::BytecodeGenerator(Isolate* isolate, Zone* zone)
       execution_context_(nullptr),
       execution_result_(nullptr),
       register_allocator_(nullptr),
+      resume_points_(0, zone),
       try_catch_nesting_level_(0),
-      try_finally_nesting_level_(0) {
+      try_finally_nesting_level_(0),
+      yield_count_(0) {
   InitializeAstVisitor(isolate);
 }
 
@@ -586,6 +588,10 @@ Handle<BytecodeArray> BytecodeGenerator::MakeBytecode(CompilationInfo* info) {
   // Initialize control scope.
   ControlScopeForTopLevel control(this);
 
+  if (IsGeneratorFunction(info->literal()->kind())) {
+    BuildGeneratorPrologue();
+  }
+
   // Build function context only if there are context allocated variables.
   if (scope()->NeedsContext()) {
     // Push a new inner context scope for the function.
@@ -634,6 +640,40 @@ void BytecodeGenerator::MakeBytecodeBody() {
   VisitStatements(info()->literal()->body());
 }
 
+void BytecodeGenerator::BuildGeneratorPrologue() {
+  resume_points_.clear();

[rmcilroy, 2016/04/15 13:17:46]

Do we need the clear? it should be empty when we reach here anyway.

[neis, 2016/04/18 08:04:17]

MakeBytecode could be called several times on the same BytecodeGenerator
instance, right?

If that's not supposed to happen, then I don't understand why CompilationInfo is
passed to MakeBytecode and not to the BytecodeGenerator constructor.

[rmcilroy, 2016/04/18 10:01:57]

No it's only ever used for a single compile info. You are right though, we
should probably change the constructor to take the CompileInfo, I'll upload a CL
to make that change.

[neis, 2016/04/18 14:07:52]

Ok. After your change I can probably initialize it directly with the correct
size.

+  resume_points_.resize(info()->literal()->yield_count());
+
+  BytecodeLabel regular_call;
+  builder()
+      ->LoadAccumulatorWithRegister(Register::new_target())
+      .JumpIfUndefined(&regular_call);
+
+  // This is a resume call. Restore registers and perform state dispatch.
+  // (The current context has already been restored by the trampoline.)
+  {
+    RegisterAllocationScope register_scope(this);
+    Register state = register_allocator()->NewRegister();
+    builder()
+        ->CallRuntime(Runtime::kResumeIgnitionGenerator, Register::new_target(),
+                      1)
+        .StoreAccumulatorInRegister(state);
+
+    // TODO(neis): Optimize this by using a proper jump table.
+    for (size_t i = 0; i < resume_points_.size(); ++i) {
+      builder()
+          ->LoadLiteral(Smi::FromInt(static_cast<int>(i)))
+          .CompareOperation(Token::Value::EQ_STRICT, state)
+          .JumpIfTrue(&(resume_points_[i]));
+    }
+    builder()->Illegal();  // Should never get here.
+  }
+
+  builder()->Bind(&regular_call);
+  // This is a regular call. Fall through to the ordinary function prologue,
+  // after which we will run into the generator object creation and the initial
+  // yield (both inserted by the parser).
+}
 
 void BytecodeGenerator::VisitBlock(Block* stmt) {
   // Visit declarations and statements.
@@ -2229,16 +2269,83 @@ void BytecodeGenerator::VisitAssignment(Assignment* expr) {
   execution_result()->SetResultInAccumulator();
 }
 
+void BytecodeGenerator::VisitYield(Yield* expr) {
+  int id = yield_count_++;

[rmcilroy, 2016/04/15 13:17:46]

Rather than doing this id naming implicitly, would it be safer to have the Yield
AST object include an ID (e.g., the one used for state calculation) which we use
as the id here and as the index to the resume_points_ ?

[neis, 2016/04/18 08:04:17]

I don't think that would be safer and I don't see any advantage. Also, my
transformation for dispatching in loops will rely on yields being numbered
consecutively.

[rmcilroy, 2016/04/18 10:01:57]

OK. I'm fine with this for this CL, but maybe it's worth considering maintaining
yield_count_ and resume_points_ in a small class to abstract the yield / resume
point maintanance (particularly if the dispatching in loops CL adds more logic
to this). 

[neis, 2016/04/18 14:07:52]

OK.  Also, I probably need to move them into arraybuilder for the next CL.

 
-void BytecodeGenerator::VisitYield(Yield* expr) { UNIMPLEMENTED(); }
+  builder()->SetExpressionPosition(expr);
+  Register value = VisitForRegisterValue(expr->expression());

[rmcilroy, 2016/04/15 13:17:46]

nit - maybe scope the pre-yield, and post-resume parts with seperate inner
scopes, and have each allocate it's own RegisterAllocationScope, that way you
will free up temporary registers between the yield and resume points.

[neis, 2016/04/18 08:04:17]

Thanks, I didn't think of that.  I added a scope for the post-resume part.  One
of the registers from the pre-resume part is used later on.

+
+  register_allocator()->PrepareForConsecutiveAllocations(2);
+  Register generator = register_allocator()->NextConsecutiveRegister();
+  Register state = register_allocator()->NextConsecutiveRegister();
+
+  // Save context, registers, and state. Then return.
+  VisitForRegisterValue(expr->generator_object(), generator);
+  builder()
+      ->LoadLiteral(Smi::FromInt(id))
+      .StoreAccumulatorInRegister(state)
+      .CallRuntime(Runtime::kSuspendIgnitionGenerator, generator, 2)
+      .LoadAccumulatorWithRegister(value)
+      .Return();  // Hard return (ignore any finally blocks).
 
+  builder()->Bind(&(resume_points_[id]));
+  // Upon resume, we continue here.
+
+  Register input = register_allocator()->NewRegister();
+  builder()
+      ->CallRuntime(Runtime::kGeneratorGetInput, generator, 1)
+      .StoreAccumulatorInRegister(input);
+
+  Register resume_mode = register_allocator()->NewRegister();
+  builder()
+      ->CallRuntime(Runtime::kGeneratorGetResumeMode, generator, 1)

[rmcilroy, 2016/04/15 13:17:46]

Maybe you could have a runtime function which returns both Input and Resume mode
rather than making two successive runtime calls here?

[neis, 2016/04/18 08:04:17]

Let's keep it as is for now.  I think we want to get rid of these calls in the
long run anyway.

[rmcilroy, 2016/04/18 10:01:57]

Acknowledged.

+      .StoreAccumulatorInRegister(resume_mode);
+
+  // Dispatch on resume mode.
+  {
+    BytecodeLabel resume_with_next;
+    BytecodeLabel resume_with_return;
+    BytecodeLabel resume_with_throw;
+
+    builder()
+        ->LoadLiteral(Smi::FromInt(JSGeneratorObject::kReturn))
+        .CompareOperation(Token::EQ_STRICT, resume_mode)
+        .JumpIfTrue(&resume_with_return)
+        .LoadLiteral(Smi::FromInt(JSGeneratorObject::kThrow))
+        .CompareOperation(Token::EQ_STRICT, resume_mode)
+        .JumpIfTrue(&resume_with_throw)
+        .Jump(&resume_with_next);

[rmcilroy, 2016/04/15 13:17:46]

(micro optimization) - presumably JSGeneratorObject::kNext is the most common
resume mode, if we test for that first we would execute less bytecode in the
common case.

[neis, 2016/04/18 08:04:17]

Done.

+
+    builder()->Bind(&resume_with_return);
+    {
+      register_allocator()->PrepareForConsecutiveAllocations(2);
+      Register value = register_allocator()->NextConsecutiveRegister();
+      Register done = register_allocator()->NextConsecutiveRegister();
+      builder()
+          ->MoveRegister(input, value)
+          .LoadTrue()
+          .StoreAccumulatorInRegister(done)
+          .CallRuntime(Runtime::kCreateIterResultObject, value, 2);
+      execution_control()->ReturnAccumulator();
+    }
+
+    builder()->Bind(&resume_with_throw);
+    builder()
+        ->LoadAccumulatorWithRegister(input)
+        .Throw();
+
+    builder()->Bind(&resume_with_next);
+    builder()->LoadAccumulatorWithRegister(input);
+    execution_result()->SetResultInAccumulator();
+  }
+}
 
 void BytecodeGenerator::VisitThrow(Throw* expr) {
   VisitForAccumulatorValue(expr->exception());
   builder()->SetExpressionPosition(expr);
   builder()->Throw();
-  // Throw statments are modeled as expression instead of statments. These are
-  // converted from assignment statements in Rewriter::ReWrite pass. An
+  // Throw statements are modeled as expressions instead of statements. These
+  // are converted from assignment statements in Rewriter::ReWrite pass. An

[rmcilroy, 2016/04/15 13:17:46]

Thanks!

   // assignment statement expects a value in the accumulator. This is a hack to
   // avoid DCHECK fails assert accumulator has been set.
   execution_result()->SetResultInAccumulator();