Merge virtual frames by copying registers to registers....

Merge virtual frames by copying registers to registers.

[William Hesse, 2008-12-10 16:17:55]

This is now separated from the changes to VisitCompare.
The register-to-register part of MergeTo, implemented simply.
We can use xchg instruction on register cycles, if needed,
after checking about its performance.

[Kevin Millikin (Chromium), 2008-12-10 17:27:06]

I haven't looked at the code it generates yet.  Lots of formatting suggestions.

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

http://codereview.chromium.org/13344/diff/1/2#newcode423
Line 423: // Then register-to-register moves, not yet implemented.
You can change this comment.

http://codereview.chromium.org/13344/diff/1/2#newcode427
Line 427: if (source.is_register() && target.is_register() &&
I think this entire loop should be moved into the function MergeToMoveRegisters.

http://codereview.chromium.org/13344/diff/1/2#newcode502
Line 502: bool moves_blocked; // Did we fail to make all the moves on this
iteration?
Style guide calls for two spaces between the semicolon and the start of the
comment.  I prefer names lie "are_moves_blocked" and "should_break_cycles" and
"were_moves_made" to indicate their flaggishness (they read like counts to me
otherwise).

http://codereview.chromium.org/13344/diff/1/2#newcode508
Line 508: int first_move_blocked = -1;  // Dead initializer.
I think we have kIllegalIndex.

http://codereview.chromium.org/13344/diff/1/2#newcode516
Line 516: continue;
The continues are somewhat confusing.  I think deeper nesting is better here. 
I'll send you my suggestion separately, because Rietveld doesn't like pasted
code here.

http://codereview.chromium.org/13344/diff/1/2#newcode537
Line 537: Use(target->reg());
I've been trying to follow the convention of updating internal state first and
then emitting code to make it true, whenever it's reasonable.  That is, you
could move the Use and Unuse to before the mov instruction.

http://codereview.chromium.org/13344/diff/1/2#newcode542
Line 542: *source = *target;
I would rather have source and target not be pointers, and then have
elements_[i] = target here.

http://codereview.chromium.org/13344/diff/1/3
File src/virtual-frame-ia32.h (right):

http://codereview.chromium.org/13344/diff/1/3#newcode451
Line 451: // merge the current frame with the expected frame.
"merge this frame with the expected frame"

http://codereview.chromium.org/13344/diff/1/3#newcode453
Line 453: // and memory to register moves will follow this call.
Is that actually a precondition, or just the way we happen to do it?

http://codereview.chromium.org/13344/diff/1/3#newcode459
Line 459: void MergeToMoveRegisters(VirtualFrame *expected,
Not clear if it's moving from registers, to registers, or both.  Try
ProcessRegisterToRegisterMoves?

[iposva, 2008-12-11 08:25:52]

On top of the comments I have made here I think that this class is missing a way
to communicate that an item is in memory. At least on IA32 memory operands are
perfectly useful.

PS. I ran out of time looking at the actual register moves implementation. I
will do that once the first set of comments is addressed.

http://codereview.chromium.org/13344/diff/1/3
File src/virtual-frame-ia32.h (right):

http://codereview.chromium.org/13344/diff/1/3#newcode43
Line 43: explicit Result(Register reg, CodeGenerator* cgen);
Do you need explicit here?

http://codereview.chromium.org/13344/diff/1/3#newcode54
Line 54: if (is_register() && !reg().is(no_reg)) {
Maybe you should introduce a special type INVALID. The fact that a Result can be
is_register() but return no_reg for the register value is confusing and will
likely lead to edge cases which will not be caught in regular code. Basically
every user of the Result class will have to not only check for is_register() but
also that the register corresponding to this result is not a no_reg.

http://codereview.chromium.org/13344/diff/1/3#newcode456
Line 456: // memory-to-register moves to break cycles.
This part of the comment is unclear. If the value is in a register what does it
mean to be converted to a memory-to-register move? And even so, is it even
necessary to put the internal implementation of how cycles are broken into the
function description?

http://codereview.chromium.org/13344/diff/1/3#newcode460
Line 460: int first_register_move);
Why does the caller of the API have to pass this parameter? Isn't it the task of
this function to figure out where to start?

[William Hesse, 2008-12-11 13:32:04]

OK, all changes made.

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

http://codereview.chromium.org/13344/diff/1/2#newcode423
Line 423: // Then register-to-register moves, not yet implemented.
On 2008/12/10 17:27:06, Kevin Millikin wrote:
> You can change this comment.

Done.

http://codereview.chromium.org/13344/diff/1/2#newcode427
Line 427: if (source.is_register() && target.is_register() &&
On 2008/12/10 17:27:06, Kevin Millikin wrote:
> I think this entire loop should be moved into the function
MergeToMoveRegisters.

Done. It was actually already there, as the main loop that does work.  If no
work is needed, it efficiently checks that.

http://codereview.chromium.org/13344/diff/1/2#newcode434
Line 434: // Finally, constant-to-register and memory-to-register.  We do these
from
Can we move memory-to-register moves into a separate function? 
It is quite a distinct operation, with a trivial interface (this and expected).

http://codereview.chromium.org/13344/diff/1/2#newcode502
Line 502: bool moves_blocked; // Did we fail to make all the moves on this
iteration?
On 2008/12/10 17:27:06, Kevin Millikin wrote:
> Style guide calls for two spaces between the semicolon and the start of the
> comment.  I prefer names lie "are_moves_blocked" and "should_break_cycles" and
> "were_moves_made" to indicate their flaggishness (they read like counts to me
> otherwise).

Done.

http://codereview.chromium.org/13344/diff/1/2#newcode508
Line 508: int first_move_blocked = -1;  // Dead initializer.
On 2008/12/10 17:27:06, Kevin Millikin wrote:
> I think we have kIllegalIndex.

Done.

http://codereview.chromium.org/13344/diff/1/2#newcode516
Line 516: continue;
On 2008/12/10 17:27:06, Kevin Millikin wrote:
> The continues are somewhat confusing.  I think deeper nesting is better here. 
> I'll send you my suggestion separately, because Rietveld doesn't like pasted
> code here.

Done.

http://codereview.chromium.org/13344/diff/1/2#newcode537
Line 537: Use(target->reg());
On 2008/12/10 17:27:06, Kevin Millikin wrote:
> I've been trying to follow the convention of updating internal state first and
> then emitting code to make it true, whenever it's reasonable.  That is, you
> could move the Use and Unuse to before the mov instruction.

Done.

http://codereview.chromium.org/13344/diff/1/2#newcode542
Line 542: *source = *target;
On 2008/12/10 17:27:06, Kevin Millikin wrote:
> I would rather have source and target not be pointers, and then have
> elements_[i] = target here.

Done.

http://codereview.chromium.org/13344/diff/1/3
File src/virtual-frame-ia32.h (right):

http://codereview.chromium.org/13344/diff/1/3#newcode43
Line 43: explicit Result(Register reg, CodeGenerator* cgen);
On 2008/12/11 08:25:52, iposva wrote:
> Do you need explicit here?

Done.

http://codereview.chromium.org/13344/diff/1/3#newcode54
Line 54: if (is_register() && !reg().is(no_reg)) {
On 2008/12/11 08:25:52, iposva wrote:
> Maybe you should introduce a special type INVALID. The fact that a Result can
be
> is_register() but return no_reg for the register value is confusing and will
> likely lead to edge cases which will not be caught in regular code. Basically
> every user of the Result class will have to not only check for is_register()
but
> also that the register corresponding to this result is not a no_reg.

Done.

http://codereview.chromium.org/13344/diff/1/3#newcode451
Line 451: // merge the current frame with the expected frame.
On 2008/12/10 17:27:06, Kevin Millikin wrote:
> "merge this frame with the expected frame"

Done.

http://codereview.chromium.org/13344/diff/1/3#newcode453
Line 453: // and memory to register moves will follow this call.
On 2008/12/10 17:27:06, Kevin Millikin wrote:
> Is that actually a precondition, or just the way we happen to do it?

It is a precondition, at least the second part, because additional mem-to-reg
moves are generated.

http://codereview.chromium.org/13344/diff/1/3#newcode456
Line 456: // memory-to-register moves to break cycles.
On 2008/12/11 08:25:52, iposva wrote:
> This part of the comment is unclear. If the value is in a register what does
it
> mean to be converted to a memory-to-register move? And even so, is it even
> necessary to put the internal implementation of how cycles are broken into the
> function description?

Done.

http://codereview.chromium.org/13344/diff/1/3#newcode459
Line 459: void MergeToMoveRegisters(VirtualFrame *expected,
On 2008/12/10 17:27:06, Kevin Millikin wrote:
> Not clear if it's moving from registers, to registers, or both.  Try
> ProcessRegisterToRegisterMoves?

MergeMoveRegistersToRegisters?

http://codereview.chromium.org/13344/diff/1/3#newcode460
Line 460: int first_register_move);
On 2008/12/11 08:25:52, iposva wrote:
> Why does the caller of the API have to pass this parameter? Isn't it the task
of
> this function to figure out where to start?

Done.

[Kevin Millikin (Chromium), 2008-12-11 15:36:58]

LGTM.  Wait and see if Ivan has any comments on the register-to-register move
stuff.

[iposva, 2008-12-12 18:13:03]

LGTM

http://codereview.chromium.org/13344/diff/208/12
File src/virtual-frame-ia32.cc (right):

http://codereview.chromium.org/13344/diff/208/12#newcode538
Line 538: end = last_move_blocked;
This is potentially a premature optimization. These stacks should never be very
large to start with.

[Kevin Millikin (Chromium), 2008-12-14 13:05:04]

LGTM with the sync state bug fixed.

http://codereview.chromium.org/13344/diff/208/12
File src/virtual-frame-ia32.cc (right):

http://codereview.chromium.org/13344/diff/208/12#newcode504
Line 504: !target.reg().is(source.reg())) {
As we discussed, the source and targets could be the same register but with
different sync states.  In that case, the source's sync bit may need to be
cleared, or a move generated to set the target's sync bit.

http://codereview.chromium.org/13344/diff/208/12#newcode526
Line 526: SyncElementAt(i);
It seems better to call sync (which will work on the source register) before
changing the reference counts to reflect the transfer.

[William Hesse, 2008-12-15 13:43:59]

Committed as revision 979.

http://codereview.chromium.org/13344/diff/208/12
File src/virtual-frame-ia32.cc (right):

http://codereview.chromium.org/13344/diff/208/12#newcode504
Line 504: !target.reg().is(source.reg())) {
On 2008/12/14 13:05:04, Kevin Millikin wrote:
> As we discussed, the source and targets could be the same register but with
> different sync states.  In that case, the source's sync bit may need to be
> cleared, or a move generated to set the target's sync bit.

Done.

http://codereview.chromium.org/13344/diff/208/12#newcode526
Line 526: SyncElementAt(i);
On 2008/12/14 13:05:04, Kevin Millikin wrote:
> It seems better to call sync (which will work on the source register) before
> changing the reference counts to reflect the transfer.

Done.

http://codereview.chromium.org/13344/diff/208/12#newcode538
Line 538: end = last_move_blocked;
On 2008/12/12 18:13:03, iposva wrote:
> This is potentially a premature optimization. These stacks should never be
very
> large to start with.

You are right.  But should we actually remove it?  The locals could be big. 
Some people are passing big arrays as argument arrays.  Are they on the frame? 
Not removed, for now.