For the values defined in the loop and spilled outside sink spill store out of the loop down to loo…

For the values defined in the loop and spilled outside sink spill store out of the loop down to loop exit whenever possible.

BUG=v8:2450

[Vyacheslav Egorov (Google), 2012-12-14 16:31:07]

Another tight loops improvement in the register allocator that was sitting on my
table for a while.

[danno, 2013-01-10 15:33:27]

https://codereview.chromium.org/11575030/diff/1/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/11575030/diff/1/src/hydrogen.cc#newcode688
src/hydrogen.cc:688: }
a bit of a nit, but how about just setting exit_block to either break_block or
loop_successor and then:

if (loop_entry->IsLoopHeader()) {
  loop_entry->loop_information()->set_exit_block(exit_block);
}
return exit_block;

https://codereview.chromium.org/11575030/diff/1/src/lithium-allocator.cc
File src/lithium-allocator.cc (right):

https://codereview.chromium.org/11575030/diff/1/src/lithium-allocator.cc#newc...
src/lithium-allocator.cc:1112: HBasicBlock* block =
loop_header->loop_information()->exit_block();
How about a loop here that tries to spill to the outer-most first and backs off
to the most nested loop (which is what you currently handle?)

https://codereview.chromium.org/11575030/diff/1/src/lithium-allocator.cc#newc...
src/lithium-allocator.cc:2159: // If the spill store was not eagerly at parent's
start then we need
nit: not eagerly emitted

https://codereview.chromium.org/11575030/diff/1/src/lithium-allocator.cc#newc...
src/lithium-allocator.cc:2167: (GetBlock(range->Start())->parent_loop_header()
!= loop_header)) {
Why don't you have to handle the case there
GetBlock(range->Start()->IsLoopHeader()) is true in a slightly different way
like you do with first_block?

https://codereview.chromium.org/11575030/diff/1/src/lithium-allocator.cc#newc...
src/lithium-allocator.cc:2170: // reduce amount of memory writes.
You check that it's a different loop, but don't you need to check the loop depth
of loop_header and and GetBlock(range->Start())->parent_loop_header()? You don't
want to make the situation worse by moving a spill from a loop into an inner
loop? Oh, I see. I suppose this is handled by making sure to always trying to
sink to the exit_block().

https://codereview.chromium.org/11575030/diff/1/src/lithium-allocator.h
File src/lithium-allocator.h (right):

https://codereview.chromium.org/11575030/diff/1/src/lithium-allocator.h#newco...
src/lithium-allocator.h:603: // Emit all pending spill stores for ranged that
have spilled siblings sinking
nit: ranges

[Vyacheslav Egorov (Google), 2013-01-25 13:49:16]

PTAL

https://codereview.chromium.org/11575030/diff/1/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/11575030/diff/1/src/hydrogen.cc#newcode688
src/hydrogen.cc:688: }
On 2013/01/10 15:33:27, danno wrote:
> a bit of a nit, but how about just setting exit_block to either break_block or
> loop_successor and then:
> 
> if (loop_entry->IsLoopHeader()) {
>   loop_entry->loop_information()->set_exit_block(exit_block);
> }
> return exit_block;

Done.

https://codereview.chromium.org/11575030/diff/1/src/lithium-allocator.cc
File src/lithium-allocator.cc (right):

https://codereview.chromium.org/11575030/diff/1/src/lithium-allocator.cc#newc...
src/lithium-allocator.cc:1112: HBasicBlock* block =
loop_header->loop_information()->exit_block();
On 2013/01/10 15:33:27, danno wrote:
> How about a loop here that tries to spill to the outer-most first and backs
off
> to the most nested loop (which is what you currently handle?)

Done.

https://codereview.chromium.org/11575030/diff/1/src/lithium-allocator.cc#newc...
src/lithium-allocator.cc:2159: // If the spill store was not eagerly at parent's
start then we need
On 2013/01/10 15:33:27, danno wrote:
> nit: not eagerly emitted

Done.

https://codereview.chromium.org/11575030/diff/1/src/lithium-allocator.cc#newc...
src/lithium-allocator.cc:2167: (GetBlock(range->Start())->parent_loop_header()
!= loop_header)) {
On 2013/01/10 15:33:27, danno wrote:
> Why don't you have to handle the case there
> GetBlock(range->Start()->IsLoopHeader()) is true in a slightly different way
> like you do with first_block?

Done.

https://codereview.chromium.org/11575030/diff/1/src/lithium-allocator.cc#newc...
src/lithium-allocator.cc:2170: // reduce amount of memory writes.
On 2013/01/10 15:33:27, danno wrote:
> You check that it's a different loop, but don't you need to check the loop
depth
> of loop_header and and GetBlock(range->Start())->parent_loop_header()? You
don't
> want to make the situation worse by moving a spill from a loop into an inner
> loop? Oh, I see. I suppose this is handled by making sure to always trying to
> sink to the exit_block().

We are always moving spilling *up*, so it can't go into loops that are deeper
nested.

https://codereview.chromium.org/11575030/diff/1/src/lithium-allocator.h
File src/lithium-allocator.h (right):

https://codereview.chromium.org/11575030/diff/1/src/lithium-allocator.h#newco...
src/lithium-allocator.h:603: // Emit all pending spill stores for ranged that
have spilled siblings sinking
On 2013/01/10 15:33:27, danno wrote:
> nit: ranges

Done.

[danno, 2013-02-01 11:41:59]

lgtm