Change MergeTo code for virtual frames to use register indices.

Change MergeTo code for virtual frames to use register indices.

Committed: http://code.google.com/p/v8/source/detail?r=1808

[William Hesse, 2009-04-27 14:20:12]

Use register locations in two of the MergeTo passes.

http://codereview.chromium.org/99052/diff/1/2
File src/ia32/virtual-frame-ia32.cc (right):

http://codereview.chromium.org/99052/diff/1/2#newcode362
Line 362: __ mov(target.reg(), Operand(ebp, fp_relative(i)));
Change i to j

[Kevin Millikin (Chromium), 2009-04-27 17:41:57]

http://codereview.chromium.org/99052/diff/1/2
File src/ia32/virtual-frame-ia32.cc (right):

http://codereview.chromium.org/99052/diff/1/2#newcode315
Line 315: if (register_locations_[i] == expected->register_locations_[i])
continue;
register_locations_[i] and register_index(from) are used multiple times in the
loop body and don't change.  It's clearer to name them with a local variable.

http://codereview.chromium.org/99052/diff/1/2#newcode317
Line 317: && expected->elements_[register_locations_[i]].is_register()) {
Is it the case that if is_used(i) then
expected->elements_[register_locations_[i]] must be a register?  We have already
done register-to-memory and register-to-invalid moves, and I don't think
anything else is possible.

http://codereview.chromium.org/99052/diff/1/2#newcode320
Line 320: if (is_used(to) && !to.is(from)) {
Doesn't register_locations_[i] != expected->register_locations_[i] (and the
property that registers are singly referenced in the frame) imply !to.is(from)? 
Maybe that should just be changed to an assert if you're worried about it.

http://codereview.chromium.org/99052/diff/1/2#newcode328
Line 328: // Terrible thing to do, but necessary:
I agree it's terrible, but is it really necessary?  What's really going on is
that there are nested loops---the outer one iterates over all registers in the
current frame, and the inner one is a while loop iterating until a register
reaches its proper frame index.

It's a little confusing to share the same loop for both purposes.  It might even
eliminate some work (like the test is_used(i) and initializing from) to add an
explicit inner loop.

http://codereview.chromium.org/99052/diff/1/2#newcode330
Line 330: } else if (!to.is(from)) {
Is it possible for to.is(from)?

http://codereview.chromium.org/99052/diff/1/2#newcode350
Line 350: int j = expected->register_locations_[i];
Since j is not a loop index, choose a descriptive name (eg, frame_index or
something).

[William Hesse, 2009-04-28 10:59:40]

Rewritten and improved.

http://codereview.chromium.org/99052/diff/1/2
File src/ia32/virtual-frame-ia32.cc (right):

http://codereview.chromium.org/99052/diff/1/2#newcode315
Line 315: if (register_locations_[i] == expected->register_locations_[i])
continue;
On 2009/04/27 17:41:57, Kevin Millikin wrote:
> register_locations_[i] and register_index(from) are used multiple times in the
> loop body and don't change.  It's clearer to name them with a local variable.

Changed to index and use_index, along with algorithm change.

http://codereview.chromium.org/99052/diff/1/2#newcode317
Line 317: && expected->elements_[register_locations_[i]].is_register()) {
On 2009/04/27 17:41:57, Kevin Millikin wrote:
> Is it the case that if is_used(i) then
> expected->elements_[register_locations_[i]] must be a register?  We have
already
> done register-to-memory and register-to-invalid moves, and I don't think
> anything else is possible.

This was true, correct.  I have changed the algorithm to iterate over the target
registers, so this is no longer true.  We haven't done memory-to-register moves
yet.

http://codereview.chromium.org/99052/diff/1/2#newcode320
Line 320: if (is_used(to) && !to.is(from)) {
On 2009/04/27 17:41:57, Kevin Millikin wrote:
> Doesn't register_locations_[i] != expected->register_locations_[i] (and the
> property that registers are singly referenced in the frame) imply
!to.is(from)? 
> Maybe that should just be changed to an assert if you're worried about it.

Done.

http://codereview.chromium.org/99052/diff/1/2#newcode328
Line 328: // Terrible thing to do, but necessary:
On 2009/04/27 17:41:57, Kevin Millikin wrote:
> I agree it's terrible, but is it really necessary?  What's really going on is
> that there are nested loops---the outer one iterates over all registers in the
> current frame, and the inner one is a while loop iterating until a register
> reaches its proper frame index.
> 
> It's a little confusing to share the same loop for both purposes.  It might
even
> eliminate some work (like the test is_used(i) and initializing from) to add an
> explicit inner loop.

Nested loop eliminated.  Iteration i's postcondition is that
the value that should be in register i is in register i if it was previously in
a register, but not a copy.

http://codereview.chromium.org/99052/diff/1/2#newcode330
Line 330: } else if (!to.is(from)) {
On 2009/04/27 17:41:57, Kevin Millikin wrote:
> Is it possible for to.is(from)?

Done.

http://codereview.chromium.org/99052/diff/1/2#newcode350
Line 350: int j = expected->register_locations_[i];
On 2009/04/27 17:41:57, Kevin Millikin wrote:
> Since j is not a loop index, choose a descriptive name (eg, frame_index or
> something).

Done.

More changes also made to this loop.

http://codereview.chromium.org/99052/diff/1/2#newcode362
Line 362: __ mov(target.reg(), Operand(ebp, fp_relative(i)));
On 2009/04/27 14:20:12, William Hesse wrote:
> Change i to j

Done.

[Kevin Millikin (Chromium), 2009-04-28 11:39:46]

http://codereview.chromium.org/99052/diff/1004/6
File src/ia32/virtual-frame-ia32.cc (right):

http://codereview.chromium.org/99052/diff/1004/6#newcode324
Line 324: // Copy contents of from from from to to to merge to expected.
This comment is cute, but it's probably better to have something that the reader
will understand no matter what's their native language.  The other code in this
file uses "source" and "target".

http://codereview.chromium.org/99052/diff/1004/6#newcode327
Line 327: elements_[index].set_reg(to);
It doesn't make a huge difference, but the rest of the virtual frame tends to
make state changes first, then emit code to make them true.

http://codereview.chromium.org/99052/diff/1004/6#newcode345
Line 345: // final step and is done from the bottom up so that the backing
There is a code quality regression to address: before, we moved memory to
register before moving possible copies to registers (so we could minimize memory
reads).  Now we can emit extra memory reads depending on register iteration
order.

http://codereview.chromium.org/99052/diff/1004/6#newcode389
Line 389: if (!source.is_register()) { Use(target.reg(), index); }
The style rule for V8 code is if a single line body is on the same line as the
test, no braces.  (And if there's braces, then it's not on the same line as the
test.  You shouldn't have both.)

[William Hesse, 2009-04-28 13:24:29]

Updated again.  Code regression fixed.

http://codereview.chromium.org/99052/diff/1004/6
File src/ia32/virtual-frame-ia32.cc (right):

http://codereview.chromium.org/99052/diff/1004/6#newcode324
Line 324: // Copy contents of from from from to to to merge to expected.
On 2009/04/28 11:39:47, Kevin Millikin wrote:
> This comment is cute, but it's probably better to have something that the
reader
> will understand no matter what's their native language.  The other code in
this
> file uses "source" and "target".
> 

Done.

http://codereview.chromium.org/99052/diff/1004/6#newcode327
Line 327: elements_[index].set_reg(to);
On 2009/04/28 11:39:47, Kevin Millikin wrote:
> It doesn't make a huge difference, but the rest of the virtual frame tends to
> make state changes first, then emit code to make them true.

Done.

http://codereview.chromium.org/99052/diff/1004/6#newcode345
Line 345: // final step and is done from the bottom up so that the backing
On 2009/04/28 11:39:47, Kevin Millikin wrote:
> There is a code quality regression to address: before, we moved memory to
> register before moving possible copies to registers (so we could minimize
memory
> reads).  Now we can emit extra memory reads depending on register iteration
> order.

Done.

http://codereview.chromium.org/99052/diff/1004/6#newcode389
Line 389: if (!source.is_register()) { Use(target.reg(), index); }
On 2009/04/28 11:39:47, Kevin Millikin wrote:
> The style rule for V8 code is if a single line body is on the same line as the
> test, no braces.  (And if there's braces, then it's not on the same line as
the
> test.  You shouldn't have both.)

Done.

[Kevin Millikin (Chromium), 2009-04-28 13:35:16]

LGTM.