[ignition] desugar async functions/generators/modules in BytecodeGenerator

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 c28d4687d8a8cf5b742626739d8cbeb50ee9dc54..e8f5e442209d61ba9d5696f6f4855a74f6cc39f1 100644
--- a/src/interpreter/bytecode-generator.cc
+++ b/src/interpreter/bytecode-generator.cc
@@ -601,6 +601,7 @@ BytecodeGenerator::BytecodeGenerator(CompilationInfo* info)
       generator_resume_points_(info->literal()->yield_count(), info->zone()),
       generator_state_(),
       loop_depth_(0),
+      catch_prediction_(HandlerTable::UNCAUGHT),
       home_object_symbol_(info->isolate()->factory()->home_object_symbol()),
       iterator_symbol_(info->isolate()->factory()->iterator_symbol()),
       prototype_string_(info->isolate()->factory()->prototype_string()),
@@ -696,14 +697,6 @@ void BytecodeGenerator::GenerateBytecode(uintptr_t stack_limit) {
     GenerateBytecodeBody();
   }
 
-  // In generator functions, we may not have visited every yield in the AST
-  // since we skip some obviously dead code. Hence the generated bytecode may
-  // contain jumps to unbound labels (resume points that will never be used).
-  // We bind these now.
-  for (auto& label : generator_resume_points_) {
-    if (!label.is_bound()) builder()->Bind(&label);
-  }
-
   // Emit an implicit return instruction in case control flow can fall off the
   // end of the function without an explicit return being present on all paths.
   if (builder()->RequiresImplicitReturn()) {
@@ -739,10 +732,220 @@ void BytecodeGenerator::GenerateBytecodeBody() {
   // Perform a stack-check before the body.
   builder()->StackCheck(info()->literal()->start_position());
 
+  // Build assignment to variable <function name> if function is a named
+  // expression and the variable is used.
+  if (info()->literal()->is_named_expression()) {
+    Variable* function_var = scope()->function_var();
+    if (function_var != nullptr && !function_var->IsUnallocated()) {
+      builder()->LoadAccumulatorWithRegister(Register::function_closure());
+      BuildVariableAssignment(function_var, Token::INIT,
+                              FeedbackVectorSlot::Invalid(),
+                              HoleCheckMode::kElided);
+    }
+  }

[rmcilroy, 2017/02/06 22:27:50]

This change doesn't seem related to the generator / resumable change, could it
be made in another CL?

[caitp, 2017/02/06 22:48:23]

well, I would love to do it in a separate CL, but there's a reason it's here:

My reasoning is as follows:

1) In order to do the implicit control-flow magic in BytecodeGenerator,
BytecodeGenerator (and thus the FunctionLiteral AST) need knowledge of stuff
that gets added implicitly to function bodies by the Parser. In particular,
parameter initialization needs to happen within the implicit try-block in async
functions, so that any errors during initialization are propagated to
RejectPromise().

2) It's possible (and there are tests verifying) that the function-named
variable is possible to use during parameter initialization, so needs to be
bound before parameter initialization.

---

So given that, it is unfortunately related. I wouldn't mind submitting it in
separate patches, but that would require changing test262.status and
mjsunit.status temporarily, and it's a relatively small change to do it here. I
think there is a net benefit to this change though, in that it allows omitting
allocation/assignment of the function-var when it's guaranteed to not be used,
which is probably the vast majority of cases.

[rmcilroy, 2017/02/07 17:24:41]

OK I can live with this change in this CL, thanks for the explination.

[caitp, 2017/02/07 18:13:08]

Actually, I brought this up in the meeting today, and Adam would prefer if I
didn't prevent common code (named function expressions where the function-named
variable is actually used) from using the full-codegen/crankshaft path in this
release.

So, if I want to do this, I either need to implement it in FCG/Crankshaft (which
shouldn't be too hard, but I suspect nobody wants me to do this), or hold back
until --turbo is enabled by default.

For now, I'll try to go with a simpler change. Will open another CL shortly.
I'll keep this one open in case anyone decides it's okay to make this change or
okay to add code handling it in FCG/CS

+
+  if (IsAsyncFunction(info()->literal()->kind())) {
+    return GenerateBytecodeBodyForAsyncFunction();
+  }
+  if (IsGeneratorFunction(info()->literal()->kind())) {
+    return GenerateBytecodeBodyForGenerator();
+  }
+  if (IsModule(info()->literal()->kind())) {
+    return GenerateBytecodeBodyForModule();
+  }

[rmcilroy, 2017/02/06 22:27:50]

Thanks for splitting the logic out of GenerateBytecodeBody. I'm not keen on
having the VisitStatements operation split out across multiple functions though.
Would something like the following be possible instead:

ResumableFunctionBodyBuilder* resumable_builder;
if (IsResumableFunction(...)) {
  resumable_builder =
ResumableFunctionBodyBuilder::Create(info()->literal()->kind());
  resumable_builder->BuildPrologue();
}
VisitStatements(info()->literal()->body());
if (IsResumableFunction(...)) {
  resumable_builder->BuildEpilogue();
}

With the ResumableFunctionBodyBuilder switching based on the function kind to
decide what prologue / epilogue to generate?

[caitp, 2017/02/06 22:48:23]

I'm not entirely sure how this would work due to how the implicit
try/catch/finally stuff is done, unless I can make that work with the try/catch
builder change you suggested below. It seems difficult to do that due to the
reliance on RAII ControlScopes.

[rmcilroy, 2017/02/07 17:24:41]

Yeah your right regarding RAII ControlScopes. I'd still prefer this was an
if/else chain with the default case being in an else at the end (and no
fallthrough)

+
+  Block* parameter_init_block = info()->literal()->parameter_init_block();
   // Visit statements in the function body.
+  if (parameter_init_block != nullptr) {
+    VisitBlock(parameter_init_block);
+  }

[rmcilroy, 2017/02/06 22:27:50]

Any reason for this change? Could we just do this unconditionally before any of
the generator alternatives?

[caitp, 2017/02/06 22:48:24]

Async functions need to incorporate the parameter initialization within the
implicit try/catch, as thrown errors need to result in a rejected Promise, so
it's handled separately for each case.

If we can eliminate the need for the try/catch in async functions (IE with an
alternative to Throw/ReThrow that always rejects a Promise), then it should be
possible to share that logic in one place

[rmcilroy, 2017/02/07 17:24:41]

I see. Is it necessary that the parameter initialization for generators is
out-with the try/finally? Or could we move it within the Try - either after the
generator creation, or update the finally block to check whether the generator
was created before closing it? If that was possible then we could have a
VisitParameterIntialiationAndStatements() function which did both of these
things and was called from the correct place here and in the resumable variants,
WDYT?

   VisitStatements(info()->literal()->body());
 }
 
+void BytecodeGenerator::BuildAllocateAndStoreJSGeneratorObject(
+    BuildJSGeneratorObject tag) {
+  RegisterAllocationScope register_scope(this);
+  RegisterList args = register_allocator()->NewRegisterList(2);
+  builder()->MoveRegister(Register::function_closure(), args[0]);
+
+  int yield_position = info()->literal()->position();

[rmcilroy, 2017/02/06 22:27:50]

Could you pass through literal as an argument rather than getting it from the
info() each time.

[caitp, 2017/02/06 22:48:23]

For the cases that would use this, aren't they always getting it from info()
anyways? But, I'll do that if that's preferable.

[caitp, 2017/02/06 23:29:36]

nvm, thought this was specifically about yield_position, but I guess there's
other uses in this function

[rmcilroy, 2017/02/07 17:24:41]

Yeah I was meaning for all the uses in the new functions (including
GenerateBytecodeBodyForX

+  builder()->SetExpressionPosition(yield_position);
+
+  if (IsArrowFunction(info()->literal()->kind())) {
+    // Lexical `this`
+    builder()->LoadUndefined().StoreAccumulatorInRegister(args[1]);
+  } else {
+    // Receiver parameter
+    builder()->MoveRegister(builder()->Parameter(0), args[1]);
+  }
+  builder()->CallRuntime(Runtime::kCreateJSGeneratorObject, args);
+
+  Variable* var_generator = scope()->generator_object_var();
+  DCHECK_NOT_NULL(var_generator);
+
+  switch (tag) {
+    case BuildJSGeneratorObject::kInitialYield: {
+      Register generator = args[0];
+      builder()->StoreAccumulatorInRegister(generator);
+      BuildVariableAssignment(var_generator, Token::INIT,
+                              FeedbackVectorSlot::Invalid(),
+                              HoleCheckMode::kElided);
+      BuildYield(0, generator, generator);
+      BuildYieldResumePoint(0, Yield::kOnExceptionThrow, generator,
+                            yield_position);
+      break;
+    }
+    case BuildJSGeneratorObject::kNoInitialYield: {
+      BuildVariableAssignment(var_generator, Token::INIT,
+                              FeedbackVectorSlot::Invalid(),
+                              HoleCheckMode::kElided);
+      break;
+    }
+  }
+}
+
+void BytecodeGenerator::BuildAllocateAndStoreJSPromise() {
+  Variable* var_promise = scope()->promise_var();
+  DCHECK_NOT_NULL(var_promise);
+
+  RegisterAllocationScope register_scope(this);
+  RegisterList args = register_allocator()->NewRegisterList(1);
+  builder()->LoadUndefined().StoreAccumulatorInRegister(args[0]).CallJSRuntime(
+      Context::ASYNC_FUNCTION_PROMISE_CREATE_INDEX, args);
+  BuildVariableAssignment(var_promise, Token::INIT,
+                          FeedbackVectorSlot::Invalid(),
+                          HoleCheckMode::kElided);
+}
+
+void BytecodeGenerator::BindUnboundGeneratorResumePoints() {
+  DCHECK(IsResumableFunction(info()->literal()->kind()));
+  for (auto& label : generator_resume_points_) {
+    if (!label.is_bound()) builder()->Bind(&label);
+  }
+}
+
+void BytecodeGenerator::GenerateBytecodeBodyForGenerator() {
+  DCHECK(IsGeneratorFunction(info()->literal()->kind()));
+
+  // Desugar parameters before generator is allocated.
+  Block* const parameter_init_block = info()->literal()->parameter_init_block();
+  if (parameter_init_block != nullptr) {
+    // Initialize non-simple parameters before initial yield.
+    VisitBlock(parameter_init_block);
+  }
+
+  BuildTryFinally(
+      [=]() {  // Try ...
+        BuildAllocateAndStoreJSGeneratorObject(
+            BuildJSGeneratorObject::kInitialYield);
+        VisitStatements(info()->literal()->body());
+      },
+      [=]() {  // Finally ...
+        Variable* var_generator = scope()->generator_object_var();
+        DCHECK_NOT_NULL(var_generator);
+
+        BuildVariableLoad(var_generator, FeedbackVectorSlot::Invalid(),
+                          HoleCheckMode::kElided);
+        RegisterAllocationScope register_scope(this);
+        Register generator = register_allocator()->NewRegister();
+        builder()->StoreAccumulatorInRegister(generator).CallRuntime(
+            Runtime::kInlineGeneratorClose, generator);
+      });
+
+  // In generator functions, we may not have visited every yield in the AST
+  // since we skip some obviously dead code. Hence the generated bytecode may
+  // contain jumps to unbound labels (resume points that will never be used).
+  // We bind these now.
+  BindUnboundGeneratorResumePoints();
+
+  if (builder()->RequiresImplicitReturn()) {

[rmcilroy, 2017/02/06 22:27:50]

From here to the end shouldn't be necessary (it's done in GenerateBytecode)

[caitp, 2017/02/06 22:48:24]

The one in GenerateBytecode uses BuildReturn(), which in the followup patch to
this (which fixes Return for async functions) produces a _lot_ of extra bytecode
which is supposed to be unreachable anyways), so I wanted to add a custom
implicit return here that avoided the full Return stuff.

Of course, an alternative fix would be to just use builder()->Return(); rather
than BuildReturn() in GenerateBytecode(), since I believe there are never any
deferred commands to handle at that point. If I do that, I think I can get rid
of this stuff.

[rmcilroy, 2017/02/07 17:24:41]

Let's make that change in the followup CL which changes what BuildReturn does
please.

+    // Unreachable, but required for BytecodeArrayBuilder
+    builder()->LoadUndefined().Return();
+  }
+  DCHECK(!builder()->RequiresImplicitReturn());
+}
+
+void BytecodeGenerator::GenerateBytecodeBodyForModule() {
+  DCHECK(IsModule(info()->literal()->kind()));
+
+  // Modules do not have non-simple parameters
+  DCHECK_NULL(info()->literal()->parameter_init_block());
+
+  BuildAllocateAndStoreJSGeneratorObject(BuildJSGeneratorObject::kInitialYield);
+  VisitStatements(info()->literal()->body());
+
+  // In modules, we may not have visited every yield in the AST
+  // since we skip some obviously dead code. Hence the generated bytecode may
+  // contain jumps to unbound labels (resume points that will never be used).
+  // We bind these now.
+  BindUnboundGeneratorResumePoints();
+}
+
+void BytecodeGenerator::GenerateBytecodeBodyForAsyncFunction() {
+  DCHECK(IsAsyncFunction(info()->literal()->kind()));
+
+  BuildAllocateAndStoreJSPromise();
+
+  BuildTryCatch(
+      HandlerTable::ASYNC_AWAIT,
+      [=]() {

[rmcilroy, 2017/02/06 22:27:49]

I don't like these lambdas, and the function pointers being passed to
BuildTryCatch. Instead, could you simply augment the TryCatch/FinallyBuilder so
that it does the additional work in VisitTryCatch/Finally (or create new
TryCatch/FinallyBuilders which augment the ones in control-builders if you need
access to the bytecode generator) and then simply use the try/catch builders
directly in the GenerateForAsync/Generator functions.

[caitp, 2017/02/06 22:48:24]

I'll give that a try.

+        // Try ...
+        Block* const parameter_init_block =
+            info()->literal()->parameter_init_block();
+        if (parameter_init_block != nullptr) {
+          // Initialize non-simple parameters if necessary, reject Promise in
+          // case of exception
+          VisitBlock(parameter_init_block);
+        }
+
+        BuildAllocateAndStoreJSGeneratorObject(
+            BuildJSGeneratorObject::kNoInitialYield);
+
+        // Finish generating function body
+        VisitStatements(info()->literal()->body());
+      },
+      [=](Register) {
+        // Catch ...
+        // Caught exception is used to reject Promise without emitting a debug
+        // event.
+        RegisterAllocationScope register_scope(this);
+        RegisterList args = register_allocator()->NewRegisterList(4);
+        Register promise = args[1];
+        Register exception = args[2];
+
+        builder()
+            ->StoreAccumulatorInRegister(exception)
+            .LoadTheHole()
+            .SetPendingMessage();
+
+        Variable* var_promise = scope()->promise_var();
+        DCHECK_NOT_NULL(var_promise);
+        BuildVariableLoad(var_promise, FeedbackVectorSlot::Invalid(),
+                          HoleCheckMode::kElided);
+        builder()
+            ->StoreAccumulatorInRegister(promise)
+            .LoadUndefined()
+            .StoreAccumulatorInRegister(args[0])
+            .LoadFalse()
+            .StoreAccumulatorInRegister(args[3])
+            .CallJSRuntime(Context::PROMISE_INTERNAL_REJECT_INDEX, args)
+            .LoadAccumulatorWithRegister(promise)
+            .Return();
+      });
+
+  // In async functions, we may not have visited every yield in the AST
+  // since we skip some obviously dead code. Hence the generated bytecode may
+  // contain jumps to unbound labels (resume points that will never be used).
+  // We bind these now.
+  BindUnboundGeneratorResumePoints();
+
+  if (builder()->RequiresImplicitReturn()) {

[rmcilroy, 2017/02/06 22:27:50]

Ditto

+    // Unreachable, but required for BytecodeArrayBuilder
+    builder()->LoadUndefined().Return();
+  }
+  DCHECK(!builder()->RequiresImplicitReturn());
+}
+
 void BytecodeGenerator::BuildIndexedJump(Register index, size_t start_index,
                                          size_t size,
                                          ZoneVector<BytecodeLabel>& targets) {
@@ -1334,8 +1537,11 @@ void BytecodeGenerator::VisitForOfStatement(ForOfStatement* stmt) {
   loop_builder.EndLoop();
 }
 
-void BytecodeGenerator::VisitTryCatchStatement(TryCatchStatement* stmt) {
-  TryCatchBuilder try_control_builder(builder(), stmt->catch_prediction());
+void BytecodeGenerator::BuildTryCatch(
+    HandlerTable::CatchPrediction catch_prediction,
+    std::function<void()> build_try,
+    std::function<void(Register)> build_catch) {
+  TryCatchBuilder try_control_builder(builder(), catch_prediction);
 
   // Preserve the context in a dedicated register, so that it can be restored
   // when the handler is entered by the stack-unwinding machinery.
@@ -1348,29 +1554,43 @@ void BytecodeGenerator::VisitTryCatchStatement(TryCatchStatement* stmt) {
   try_control_builder.BeginTry(context);
   {
     ControlScopeForTryCatch scope(this, &try_control_builder);
-    Visit(stmt->try_block());
+    HandlerTable::CatchPrediction last_catch_prediction = catch_prediction_;
+    if (catch_prediction != HandlerTable::UNCAUGHT) {
+      catch_prediction_ = catch_prediction;
+    }
+    build_try();
+    catch_prediction_ = last_catch_prediction;
   }
   try_control_builder.EndTry();
 
-  // Create a catch scope that binds the exception.
-  BuildNewLocalCatchContext(stmt->variable(), stmt->scope());
-  builder()->StoreAccumulatorInRegister(context);
+  // Evaluate the catch-block.
+  build_catch(context);
+  try_control_builder.EndCatch();
+}
 
-  // If requested, clear message object as we enter the catch block.
-  if (stmt->clear_pending_message()) {
-    builder()->LoadTheHole().SetPendingMessage();
-  }
+void BytecodeGenerator::VisitTryCatchStatement(TryCatchStatement* stmt) {
+  BuildTryCatch(stmt->catch_prediction(), [=]() { Visit(stmt->try_block()); },
+                [=](Register context) {
+                  // Create a catch scope that binds the exception.
+                  BuildNewLocalCatchContext(stmt->variable(), stmt->scope());
+                  builder()->StoreAccumulatorInRegister(context);
 
-  // Load the catch context into the accumulator.
-  builder()->LoadAccumulatorWithRegister(context);
+                  // I requested, clear message object as we enter the catch
+                  // block.
+                  if (stmt->clear_pending_message()) {
+                    builder()->LoadTheHole().SetPendingMessage();
+                  }
 
-  // Evaluate the catch-block.
-  VisitInScope(stmt->catch_block(), stmt->scope());
-  try_control_builder.EndCatch();
+                  // Load the catch context into the accumulator.
+                  builder()->LoadAccumulatorWithRegister(context);
+
+                  VisitInScope(stmt->catch_block(), stmt->scope());
+                });
 }
 
-void BytecodeGenerator::VisitTryFinallyStatement(TryFinallyStatement* stmt) {
-  TryFinallyBuilder try_control_builder(builder(), stmt->catch_prediction());
+void BytecodeGenerator::BuildTryFinally(std::function<void()> build_try,
+                                        std::function<void()> build_finally) {
+  TryFinallyBuilder try_control_builder(builder(), catch_prediction_);
 
   // We keep a record of all paths that enter the finally-block to be able to
   // dispatch to the correct continuation point after the statements in the
@@ -1402,7 +1622,7 @@ void BytecodeGenerator::VisitTryFinallyStatement(TryFinallyStatement* stmt) {
   try_control_builder.BeginTry(context);
   {
     ControlScopeForTryFinally scope(this, &try_control_builder, &commands);
-    Visit(stmt->try_block());
+    build_try();
   }
   try_control_builder.EndTry();
 
@@ -1421,7 +1641,7 @@ void BytecodeGenerator::VisitTryFinallyStatement(TryFinallyStatement* stmt) {
       message);
 
   // Evaluate the finally-block.
-  Visit(stmt->finally_block());
+  build_finally();
   try_control_builder.EndFinally();
 
   // Pending message object is restored on exit.
@@ -1431,6 +1651,11 @@ void BytecodeGenerator::VisitTryFinallyStatement(TryFinallyStatement* stmt) {
   commands.ApplyDeferredCommands();
 }
 
+void BytecodeGenerator::VisitTryFinallyStatement(TryFinallyStatement* stmt) {
+  BuildTryFinally([=]() { Visit(stmt->try_block()); },
+                  [=]() { Visit(stmt->finally_block()); });
+}
+
 void BytecodeGenerator::VisitDebuggerStatement(DebuggerStatement* stmt) {
   builder()->SetStatementPosition(stmt);
   builder()->Debugger();
@@ -2287,20 +2512,31 @@ void BytecodeGenerator::VisitAssignment(Assignment* expr) {
   }
 }
 
-void BytecodeGenerator::VisitYield(Yield* expr) {
-  builder()->SetExpressionPosition(expr);
-  Register value = VisitForRegisterValue(expr->expression());
-
-  Register generator = VisitForRegisterValue(expr->generator_object());
+void BytecodeGenerator::BuildYield(int yield_id, Register generator,
+                                   Register value) {
+  DCHECK_GE(yield_id, 0);
+  DCHECK_LT(yield_id, generator_resume_points_.size());
+  DCHECK(generator.is_valid());
+  DCHECK(value.is_valid());
 
   // Save context, registers, and state. Then return.
   builder()
-      ->LoadLiteral(Smi::FromInt(expr->yield_id()))
+      ->LoadLiteral(Smi::FromInt(yield_id))
       .SuspendGenerator(generator)
       .LoadAccumulatorWithRegister(value)
       .Return();  // Hard return (ignore any finally blocks).
+}
 
-  builder()->Bind(&(generator_resume_points_[expr->yield_id()]));
+void BytecodeGenerator::BuildYieldResumePoint(int yield_id,
+                                              Yield::OnException on_exception,
+                                              Register generator,
+                                              int position) {
+  DCHECK_GE(yield_id, 0);
+  DCHECK_LT(yield_id, generator_resume_points_.size());
+  DCHECK(generator.is_valid());
+
+  DCHECK(!generator_resume_points_[yield_id].is_bound());
+  builder()->Bind(&(generator_resume_points_[yield_id]));
   // Upon resume, we continue here.
 
   {
@@ -2328,6 +2564,7 @@ void BytecodeGenerator::VisitYield(Yield* expr) {
     BytecodeLabel resume_with_return;
     BytecodeLabel resume_with_throw;
 
+    // TODO(caitp): Don't generate `resume_with_return` block for non-generators
     builder()
         ->LoadLiteral(Smi::FromInt(JSGeneratorObject::kNext))
         .CompareOperation(Token::EQ_STRICT, resume_mode)
@@ -2349,9 +2586,9 @@ void BytecodeGenerator::VisitYield(Yield* expr) {
     }
 
     builder()->Bind(&resume_with_throw);
-    builder()->SetExpressionPosition(expr);
+    builder()->SetExpressionPosition(position);
     builder()->LoadAccumulatorWithRegister(input);
-    if (expr->rethrow_on_exception()) {
+    if (on_exception == Yield::kOnExceptionRethrow) {
       builder()->ReThrow();
     } else {
       builder()->Throw();
@@ -2362,6 +2599,17 @@ void BytecodeGenerator::VisitYield(Yield* expr) {
   }
 }
 
+void BytecodeGenerator::VisitYield(Yield* expr) {
+  Register generator = VisitForRegisterValue(expr->generator_object());
+
+  builder()->SetExpressionPosition(expr);
+  Register value = VisitForRegisterValue(expr->expression());
+
+  BuildYield(expr->yield_id(), generator, value);
+  BuildYieldResumePoint(expr->yield_id(), expr->on_exception(), generator,
+                        expr->position());
+}
+
 void BytecodeGenerator::VisitThrow(Throw* expr) {
   VisitForAccumulatorValue(expr->exception());
   builder()->SetExpressionPosition(expr);