When selecting an optimal position for SplitBetween(from, to) ensure:

When selecting an optimal position for SplitBetween(from, to) ensure:

- that loop and block structure is taken into account;
- that split sibling has proper interference with values incoming into instruction corresponding to the to position. 


R=srdjan@google.com
BUG=4308
Committed: https://code.google.com/p/dart/source/detail?r=10208

[srdjan, 2012-08-02 20:04:37]

LGTM

https://chromiumcodereview.appspot.com/10850023/diff/1/runtime/vm/flow_graph_...
File runtime/vm/flow_graph_allocator.cc (right):

https://chromiumcodereview.appspot.com/10850023/diff/1/runtime/vm/flow_graph_...
runtime/vm/flow_graph_allocator.cc:951: static bool IsNestedLoop(BlockInfo*
outer, BlockInfo* inner) {
Can you make checks a little bit more robust since the method is outside the
context where it is currently used. E.g.:
ASSERT(outer == NULL || outer->is_loop_info())

Shouldn't it say false if there is no outer loop (outer == NULL) or should the
function be called IsLoop?

Do you want to check the postorder_number (as you do for detecting back-edges)
or block_id?

https://chromiumcodereview.appspot.com/10850023/diff/1/runtime/vm/flow_graph_...
runtime/vm/flow_graph_allocator.cc:957: // Discover structural (reducible) loops
nesting structure.
Aren't all Dart generated loops reducible?

https://chromiumcodereview.appspot.com/10850023/diff/1/runtime/vm/flow_graph_...
runtime/vm/flow_graph_allocator.cc:1204: // loops header if possible.
Please make comment easier to read.

[Vyacheslav Egorov (Google), 2012-08-03 06:29:00]

Thanks for the review. Landing.

https://chromiumcodereview.appspot.com/10850023/diff/1/runtime/vm/flow_graph_...
File runtime/vm/flow_graph_allocator.cc (right):

https://chromiumcodereview.appspot.com/10850023/diff/1/runtime/vm/flow_graph_...
runtime/vm/flow_graph_allocator.cc:951: static bool IsNestedLoop(BlockInfo*
outer, BlockInfo* inner) {
On 2012/08/02 20:04:37, srdjan wrote:
> Can you make checks a little bit more robust since the method is outside the
> context where it is currently used. E.g.:
> ASSERT(outer == NULL || outer->is_loop_info())
> 
> Shouldn't it say false if there is no outer loop (outer == NULL) or should the
> function be called IsLoop?
> 
> Do you want to check the postorder_number (as you do for detecting back-edges)
> or block_id?

Inlined function back into DiscoverLoops to avoid confusion.

It was just checking that a new backedge does not exit the current loop to
filter out cases where loops are improperly nested.

https://chromiumcodereview.appspot.com/10850023/diff/1/runtime/vm/flow_graph_...
runtime/vm/flow_graph_allocator.cc:957: // Discover structural (reducible) loops
nesting structure.
On 2012/08/02 20:04:37, srdjan wrote:
> Aren't all Dart generated loops reducible?

AFAIK you can generate an irreducible control flow with 

switch (a) {
  case 1: break 2;
  case 2: break 1;
}

if it still compiles.

https://chromiumcodereview.appspot.com/10850023/diff/1/runtime/vm/flow_graph_...
runtime/vm/flow_graph_allocator.cc:1204: // loops header if possible.
On 2012/08/02 20:04:37, srdjan wrote:
> Please make comment easier to read.

Done.

[srdjan, 2012-08-03 21:40:49]

https://chromiumcodereview.appspot.com/10850023/diff/1/runtime/vm/flow_graph_...
File runtime/vm/flow_graph_allocator.cc (right):

https://chromiumcodereview.appspot.com/10850023/diff/1/runtime/vm/flow_graph_...
runtime/vm/flow_graph_allocator.cc:957: // Discover structural (reducible) loops
nesting structure.
On 2012/08/03 06:29:00, Vyacheslav Egorov (Google) wrote:
> On 2012/08/02 20:04:37, srdjan wrote:
> > Aren't all Dart generated loops reducible?
> 
> AFAIK you can generate an irreducible control flow with 
> 
> switch (a) {
>   case 1: break 2;
>   case 2: break 1;
> }
> 
> if it still compiles.

You cannot do this. The break goes only  to outer enclosing structure (loop,
switch, etc).