Await always waits

Await always waits

The expression ‘await e’ always suspends the enclosing function. If e does not
evaluate to a an object o of type Future, a new Future is created using Future.value(o).

A small change in the backend allows a return instruction to be followed by
other instructions. In async functions, a return can suspend the function and the
continuation point is in the same code block after the return.

Other small changes:
- More user-friendly error message if async/await is not enabled.
- Programs no longer need to import dart:async when using async/await.

R=fschneider@google.com

Committed: https://code.google.com/p/dart/source/detail?r=41105

[hausner, 2014-10-06 16:02:13]

hausner@google.com changed reviewers:
+ fschneider@google.com, iposva@google.com

[Florian Schneider, 2014-10-06 17:32:55]

Here is my suggestion:

https://codereview.chromium.org/634603002/diff/1/runtime/vm/flow_graph_builde...
File runtime/vm/flow_graph_builder.cc (right):

https://codereview.chromium.org/634603002/diff/1/runtime/vm/flow_graph_builde...
runtime/vm/flow_graph_builder.cc:1106: }
I'd imagine this looking like this:

AddReturnExit(node->token_pos(), return_value);

if (function.is_async_closure() &&
    (node->return_type() == ReturnNode::kContinuation)) {
  JoinEntryInstr* join = new(I) JoinEntryInstr(
      owner()->AllocateBlockId(), owner()->try_index());
  owner()->AddAwaitJoin(join);
  exit_ = join;
}

where AddAwaitJoin is like AddCatchEntry and the await-joins blocks are added to
the successors of GraphEntryInstr like catch-blocks.

https://codereview.chromium.org/634603002/diff/1/runtime/vm/flow_graph_builde...
runtime/vm/flow_graph_builder.cc:2251: if (node->marker_type() ==
AwaitMarkerNode::kTargetForContinuation) {
This always follows immediatly after a continuation-return, so we do all of this
as part of ReturnInstr if the type if ReturnType::kContinuation.

Then, there is no need for this type of AwaitMarkerNode anymore.

https://codereview.chromium.org/634603002/diff/1/runtime/vm/flow_graph_builde...
runtime/vm/flow_graph_builder.cc:2257: owner()->await_joins()->Add(join);
These targets should be added to the successors of the graph entry block (see
how we handle CatchBlockEntryInstr).

https://codereview.chromium.org/634603002/diff/1/runtime/vm/flow_graph_builde...
runtime/vm/flow_graph_builder.cc:2258: Goto(join);
Remove this Goto. It is dead code and should not be needed.

[Florian Schneider, 2014-10-09 11:29:31]

I think it should work with undoing the changes to GraphEntry and moving the
creation of the await-join block to VisitReturn like you did in the last patch
set.

https://codereview.chromium.org/634603002/diff/20001/runtime/vm/intermediate_...
File runtime/vm/intermediate_language.cc (right):

https://codereview.chromium.org/634603002/diff/20001/runtime/vm/intermediate_...
runtime/vm/intermediate_language.cc:1125: return 1 + catch_entries_.length() +
await_joins_.length();
After looking into the issue I don't think this needed - the await_joins don't
need to be successors of the graph entry since they will be connected later.

https://codereview.chromium.org/634603002/diff/20001/runtime/vm/intermediate_...
runtime/vm/intermediate_language.cc:1131: index--;
Revert this to the original code as well.

[hausner, 2014-10-13 23:41:26]

New version. PTAL

- Undoing the changes to GraphEntryInstr.
- Disable optimization of async closure code until we know what
  causes the failures that you suspect in the load optimizer.

https://codereview.chromium.org/634603002/diff/20001/runtime/vm/intermediate_...
File runtime/vm/intermediate_language.cc (right):

https://codereview.chromium.org/634603002/diff/20001/runtime/vm/intermediate_...
runtime/vm/intermediate_language.cc:1125: return 1 + catch_entries_.length() +
await_joins_.length();
On 2014/10/09 11:29:31, Florian Schneider wrote:
> After looking into the issue I don't think this needed - the await_joins don't
> need to be successors of the graph entry since they will be connected later.

Done.

https://codereview.chromium.org/634603002/diff/20001/runtime/vm/intermediate_...
runtime/vm/intermediate_language.cc:1131: index--;
On 2014/10/09 11:29:31, Florian Schneider wrote:
> Revert this to the original code as well.

Done.

[Florian Schneider, 2014-10-14 11:47:30]

lgtm

https://codereview.chromium.org/634603002/diff/90001/runtime/vm/ast_transform...
File runtime/vm/ast_transformer.cc (right):

https://codereview.chromium.org/634603002/diff/90001/runtime/vm/ast_transform...
runtime/vm/ast_transformer.cc:179: GetVariableInScope(preamble_->scope(),
Symbols::AwaitJumpVar());
It looks a little strange that the result of this call is not used here. Why is
the side-effect needed here? and could it be moved to the place where the
variable is actually used? (e.g. EffectGraphVisitor::VisitAwaitMarkerNode)

https://codereview.chromium.org/634603002/diff/90001/runtime/vm/ast_transform...
runtime/vm/ast_transformer.cc:180: GetVariableInScope(preamble_->scope(),
Symbols::AwaitContextVar());
Same question for this variable.

https://codereview.chromium.org/634603002/diff/90001/runtime/vm/flow_graph_bu...
File runtime/vm/flow_graph_builder.cc (right):

https://codereview.chromium.org/634603002/diff/90001/runtime/vm/flow_graph_bu...
runtime/vm/flow_graph_builder.cc:2235: const intptr_t jump_cnt =
owner()->next_await_counter();
Abbreviations that are not a prefix of the original are cumbersome to search for
- especially because we use both (context/ctx, cnt/count, etc) in different
places:

s/jump_cnt/jump_count/g
s/ctx_var/context_var/g

https://codereview.chromium.org/634603002/diff/90001/runtime/vm/object_store.cc
File runtime/vm/object_store.cc (right):

https://codereview.chromium.org/634603002/diff/90001/runtime/vm/object_store....
runtime/vm/object_store.cc:172: bool ObjectStore::InitAsyncObjects() {
If the return value is never used, maybe just make it a void function.

[Florian Schneider, 2014-10-14 15:04:37]

PS

https://codereview.chromium.org/634603002/diff/90001/runtime/vm/ast_transform...
File runtime/vm/ast_transformer.cc (right):

https://codereview.chromium.org/634603002/diff/90001/runtime/vm/ast_transform...
runtime/vm/ast_transformer.cc:179: GetVariableInScope(preamble_->scope(),
Symbols::AwaitJumpVar());
On 2014/10/14 11:47:30, Florian Schneider wrote:
> It looks a little strange that the result of this call is not used here. Why
is
> the side-effect needed here? and could it be moved to the place where the
> variable is actually used? (e.g. EffectGraphVisitor::VisitAwaitMarkerNode)

I tried fixing this in my CL (https://codereview.chromium.org/655773003/) and
mark those as captured variables in the parser instead.

[hausner, 2014-10-14 19:56:39]

Thank you for finding the load elimination bug.

https://codereview.chromium.org/634603002/diff/90001/runtime/vm/ast_transform...
File runtime/vm/ast_transformer.cc (right):

https://codereview.chromium.org/634603002/diff/90001/runtime/vm/ast_transform...
runtime/vm/ast_transformer.cc:179: GetVariableInScope(preamble_->scope(),
Symbols::AwaitJumpVar());
On 2014/10/14 15:04:37, Florian Schneider wrote:
> On 2014/10/14 11:47:30, Florian Schneider wrote:
> > It looks a little strange that the result of this call is not used here. Why
> is
> > the side-effect needed here? and could it be moved to the place where the
> > variable is actually used? (e.g. EffectGraphVisitor::VisitAwaitMarkerNode)
> 
> I tried fixing this in my CL (https://codereview.chromium.org/655773003/) and
> mark those as captured variables in the parser instead.

Let's move these with your change list, as a separate change from this one.

As i commented in your CL, I am not sure why these statements are here, but
there may be a reason for it. Looking up a variable for a particular scope has a
side effect on that scope. We may depend on this side effect here.

https://codereview.chromium.org/634603002/diff/90001/runtime/vm/flow_graph_bu...
File runtime/vm/flow_graph_builder.cc (right):

https://codereview.chromium.org/634603002/diff/90001/runtime/vm/flow_graph_bu...
runtime/vm/flow_graph_builder.cc:2235: const intptr_t jump_cnt =
owner()->next_await_counter();
On 2014/10/14 11:47:30, Florian Schneider wrote:
> Abbreviations that are not a prefix of the original are cumbersome to search
for
> - especially because we use both (context/ctx, cnt/count, etc) in different
> places:
> 
> s/jump_cnt/jump_count/g
> s/ctx_var/context_var/g

We use ctx as much as we use context across our source.

Changed cnt->count in various places.

https://codereview.chromium.org/634603002/diff/90001/runtime/vm/object_store.cc
File runtime/vm/object_store.cc (right):

https://codereview.chromium.org/634603002/diff/90001/runtime/vm/object_store....
runtime/vm/object_store.cc:172: bool ObjectStore::InitAsyncObjects() {
On 2014/10/14 11:47:30, Florian Schneider wrote:
> If the return value is never used, maybe just make it a void function.

Done.

[hausner, 2014-10-14 19:58:22]

Committed patchset #6 (id:110001) manually as r41105 (presubmit successful).