[async-await] (simpler) fix for Return in try/finally in async functions

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 cfc45caefe82a366f30f0a3b856ebe44c6bee757..82c9a7cf55797384a0354820bade802f7b7c71df 100644
--- a/src/interpreter/bytecode-generator.cc
+++ b/src/interpreter/bytecode-generator.cc
@@ -706,8 +706,11 @@ void BytecodeGenerator::GenerateBytecode(uintptr_t stack_limit) {
   // 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()) {
+    // Do a hard return as execution_control is the top level and will not
+    // issue a deferred return command.
+    DCHECK(execution_control() == &control);
     builder()->LoadUndefined();
-    BuildReturn();
+    BuildHardReturn();

[Dan Ehrenberg, 2017/02/09 06:40:00]

Why is this a hard return? I would expect implicit returns to resolve the
promise with 'undefined', rather than actually return 'undefined'.

[caitp, 2017/02/09 12:53:07]

This change is just too shrink bytecode a bit.

The parser inserts an actual "implicit return" as the last statement in the try
block, so this thing is actually unreachable. Even so, all blocks need a return,
so this generates "LdaUndefined Return" instead of the full promise resolution
thing, because for async functions and generators it doesn't actually matter.

[Dan Ehrenberg, 2017/02/09 13:23:36]

I thought this would be reached in a case like "async function() { }". How does
that end up resolving the Promise with undefined?

[rmcilroy, 2017/02/09 13:33:49]

I didn't realize this was only a to optimize bytecode size, I thought it was
necessary with the promise resolution. I'm a bit less happy with having this as
a hard return just because it's dead code in async code. Could we just make it a
normal return for now?

Going forward, if the parser is smart enough to add implicit returns for async
functions, could it add implicit returns for all functions? If so we could get
rid of this entirely.

[caitp, 2017/02/09 13:38:47]

My understanding is that this is needed to ensure that the last block has an
exit, in order to be a valid graph, which is presumably important for TF.

So, ordinary functions could have an inserted `return undefined` in the parser,
but the return that the parser inserts for async functions doesn't end up being
the exit for the last block in the function, due to the implicit try/catch. So
in that situation, the implicit return here would still be needed.

   }
   DCHECK(!builder()->RequiresImplicitReturn());
 }
@@ -1058,6 +1061,14 @@ void BytecodeGenerator::VisitBreakStatement(BreakStatement* stmt) {
 void BytecodeGenerator::VisitReturnStatement(ReturnStatement* stmt) {
   builder()->SetStatementPosition(stmt);
   VisitForAccumulatorValue(stmt->expression());
+
+  if (stmt->is_hard_return()) {
+    // TODO(caitp): remove hard_return handling and ReturnStatement AST node
+    // flags once BytecodeGenerator performs all implicit control flow for
+    // async functions.
+    return BuildHardReturn();
+  }
+
   execution_control()->ReturnAccumulator();
 }
 
@@ -1991,6 +2002,30 @@ void BytecodeGenerator::BuildVariableLoadForAccumulatorValue(
 }
 
 void BytecodeGenerator::BuildReturn() {
+  if (IsAsyncFunction(info()->literal()->kind())) {
+    RegisterAllocationScope register_scope(this);
+    RegisterList args = register_allocator()->NewRegisterList(3);
+    Register receiver = args[0];
+    Register promise = args[1];
+    Register return_value = args[2];
+    builder()->StoreAccumulatorInRegister(return_value);
+
+    Variable* var_promise = scope()->promise_var();
+    DCHECK_NOT_NULL(var_promise);
+    BuildVariableLoad(var_promise, FeedbackSlot::Invalid(),
+                      HoleCheckMode::kElided);
+    builder()
+        ->StoreAccumulatorInRegister(promise)
+        .LoadUndefined()
+        .StoreAccumulatorInRegister(receiver)
+        .CallJSRuntime(Context::PROMISE_RESOLVE_INDEX, args)
+        .LoadAccumulatorWithRegister(promise);
+  }
+
+  BuildHardReturn();
+}
+
+void BytecodeGenerator::BuildHardReturn() {
   if (FLAG_trace) {
     RegisterAllocationScope register_scope(this);
     Register result = register_allocator()->NewRegister();