Implement JumpNode for break/continue in for loops.

Implement JumpNode for break/continue in for loops.
Committed: https://code.google.com/p/dart/source/detail?r=6050

[srdjan, 2012-03-28 23:29:34]

1.8% bailouts

[regis, 2012-03-29 02:31:43]

LGTM

[Kevin Millikin (Google), 2012-03-29 11:38:31]

I think there's a bug in the translation of JumpNode (setting entry_ to the jump
target).

It might be clearer to lazily allocate the join node in JumpNode, but you might
disagree about that.

https://chromiumcodereview.appspot.com/9907001/diff/6001/runtime/vm/flow_grap...
File runtime/vm/flow_graph_builder.cc (right):

https://chromiumcodereview.appspot.com/9907001/diff/6001/runtime/vm/flow_grap...
runtime/vm/flow_graph_builder.cc:760: if (node->label()->is_break_jump_target())
{
You could do this lazily in VisitJumpNode:

if ((node->kind() == Token::kBREAK) &&
    (node->label()->join_for_break() == NULL)) {
  node->label()->join_for_break() = new Join...;
}

It has two small advantages that I can see: avoids a join for unreachable breaks
(rare) and keeps the logic for break and continue out the logic for looping
graph structure, which is already kind of complicated.

https://chromiumcodereview.appspot.com/9907001/diff/6001/runtime/vm/flow_grap...
runtime/vm/flow_graph_builder.cc:767: BlockEntryInstr* increment_entry = NULL;
You could also do this lazily in VisitJumpNode.

https://chromiumcodereview.appspot.com/9907001/diff/6001/runtime/vm/flow_grap...
runtime/vm/flow_graph_builder.cc:777: EffectGraphVisitor for_increment(owner(),
temp_index());
Then you can:

1. Visit the body first.  After visiting the body, ask if
node->label()->join_for_continue() is NULL.  If so and the body is open, set its
exit's successor to the join for continue.

If there is no join for continue, then we can accumulate the increment
instructions in the same block as the body (avoiding an unnecessary basic block
in the common case) as before.

2. Visit the condition.  Tie it to the body, and a false successor to a fresh
target (if there is no join for break) or an edge (split by a target) to the
join for break.

Does that make sense?

https://chromiumcodereview.appspot.com/9907001/diff/6001/runtime/vm/flow_grap...
runtime/vm/flow_graph_builder.cc:864: entry_ = exit_ =
node->label()->join_for_break();
Hmm.  We don't need to set exit_, because CloseFragment will just clear it (the
jump node has no fall through).

I'm not sure it's right to set entry_ either.  Won't that leave the inlined
finally blocks and the context unchaining instructions dangling when the caller
tries to Append the result of translating the JumpNode?

[srdjan, 2012-03-30 00:53:50]

I like the idea of allocating join node in JumpNode as it should make the loop
coding simpler. I have added bailout tests to loops/switch/case and sequences.

This CL reminded me how painful it is to build the graph correctly. I am not
that happy with the current state, and think that we should have some higher
level abstractions for connecting the graph correctly.


On 2012/03/29 11:38:31, kmillikin wrote:
> I think there's a bug in the translation of JumpNode (setting entry_ to the
jump
> target).
> 
> It might be clearer to lazily allocate the join node in JumpNode, but you
might
> disagree about that.
> 
>
https://chromiumcodereview.appspot.com/9907001/diff/6001/runtime/vm/flow_grap...
> File runtime/vm/flow_graph_builder.cc (right):
> 
>
https://chromiumcodereview.appspot.com/9907001/diff/6001/runtime/vm/flow_grap...
> runtime/vm/flow_graph_builder.cc:760: if
(node->label()->is_break_jump_target())
> {
> You could do this lazily in VisitJumpNode:
> 
> if ((node->kind() == Token::kBREAK) &&
>     (node->label()->join_for_break() == NULL)) {
>   node->label()->join_for_break() = new Join...;
> }
> 
> It has two small advantages that I can see: avoids a join for unreachable
breaks
> (rare) and keeps the logic for break and continue out the logic for looping
> graph structure, which is already kind of complicated.
> 
>
https://chromiumcodereview.appspot.com/9907001/diff/6001/runtime/vm/flow_grap...
> runtime/vm/flow_graph_builder.cc:767: BlockEntryInstr* increment_entry = NULL;
> You could also do this lazily in VisitJumpNode.
> 
>
https://chromiumcodereview.appspot.com/9907001/diff/6001/runtime/vm/flow_grap...
> runtime/vm/flow_graph_builder.cc:777: EffectGraphVisitor
for_increment(owner(),
> temp_index());
> Then you can:
> 
> 1. Visit the body first.  After visiting the body, ask if
> node->label()->join_for_continue() is NULL.  If so and the body is open, set
its
> exit's successor to the join for continue.
> 
> If there is no join for continue, then we can accumulate the increment
> instructions in the same block as the body (avoiding an unnecessary basic
block
> in the common case) as before.
> 
> 2. Visit the condition.  Tie it to the body, and a false successor to a fresh
> target (if there is no join for break) or an edge (split by a target) to the
> join for break.
> 
> Does that make sense?
> 
>
https://chromiumcodereview.appspot.com/9907001/diff/6001/runtime/vm/flow_grap...
> runtime/vm/flow_graph_builder.cc:864: entry_ = exit_ =
> node->label()->join_for_break();
> Hmm.  We don't need to set exit_, because CloseFragment will just clear it
(the
> jump node has no fall through).
> 
> I'm not sure it's right to set entry_ either.  Won't that leave the inlined
> finally blocks and the context unchaining instructions dangling when the
caller
> tries to Append the result of translating the JumpNode?