Adds Nip() function, makes UnloadReference() work without spilling

Adds Nip() function, makes UnloadReference() work without spilling
frames.

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

[William Hesse, 2009-01-07 13:13:12]

Also fixes stack pointers in MergeTo.

[Kevin Millikin (Chromium), 2009-01-07 16:00:34]

Looks OK, with comments.

http://codereview.chromium.org/16572/diff/8/9
File src/virtual-frame-ia32.cc (right):

http://codereview.chromium.org/16572/diff/8/9#newcode414
Line 414: if (height_difference > 0) {
This is harder to read than it needs to be since there are separate cases for
the two ways height_difference can be nonzero.  I would write if (stack_pointer_
> expected->stack_pointer_) here and inverted below.

http://codereview.chromium.org/16572/diff/8/9#newcode415
Line 415: __ add(Operand(esp), Immediate(height_difference * kPointerSize));
There needs to be an assert that we are not skipping the stack pointer below
memory or synced elements with this.

http://codereview.chromium.org/16572/diff/8/9#newcode419
Line 419: for (int i = 0 ; i < -height_difference; i++) {
No space before semicolon.  Don't like having to negate the upper bound.  Why
not while (stack_pointer_ < expected->stack_pointer_) with an increment of
stack_pointer_ in the loop?

http://codereview.chromium.org/16572/diff/8/9#newcode690
Line 690: if (elements_[frame_index].is_register()) {
Please name elements_[frame_index].

http://codereview.chromium.org/16572/diff/8/9#newcode696
Line 696: // If target is already value->reg(), and is synced, keep it.
This comment is wrong.  We keep the element even if it's not synced (and the
sense of the comment is wrong for the comparison it describes---try something
like "Unless the frame element agrees with the value, change it").

http://codereview.chromium.org/16572/diff/8/9#newcode1031
Line 1031: SetElementAt(num_dropped - 1, &tos);
Isn't it a bit clearer to drop all but one of the elements first and then set
the element at 0 to be the old tos element?

http://codereview.chromium.org/16572/diff/8/10
File src/virtual-frame-ia32.h (right):

http://codereview.chromium.org/16572/diff/8/10#newcode388
Line 388: // Nip removes one or more elements from immediately below the top
Zero or more elements.

http://codereview.chromium.org/16572/diff/8/10#newcode389
Line 389: // of the stack, leaving TOS unchanged, but lower.  Nip(k) is
equivalent
Not necessarily lower, but the more accurate "unchanged, but not higher" sounds
goofy.  Just say that it leaves the previous top-of-frame value on top of the
frame (I've been trying to use frame for the virtual frame and stack to refer
only to the processor's actual stack).

[William Hesse, 2009-01-07 16:31:27]

Ok, tested and submitting.

http://codereview.chromium.org/16572/diff/8/9
File src/virtual-frame-ia32.cc (right):

http://codereview.chromium.org/16572/diff/8/9#newcode414
Line 414: if (height_difference > 0) {
On 2009/01/07 16:00:35, Kevin Millikin wrote:
> This is harder to read than it needs to be since there are separate cases for
> the two ways height_difference can be nonzero.  I would write if
(stack_pointer_
> > expected->stack_pointer_) here and inverted below.

Done.

http://codereview.chromium.org/16572/diff/8/9#newcode415
Line 415: __ add(Operand(esp), Immediate(height_difference * kPointerSize));
On 2009/01/07 16:00:35, Kevin Millikin wrote:
> There needs to be an assert that we are not skipping the stack pointer below
> memory or synced elements with this.

Done.

http://codereview.chromium.org/16572/diff/8/9#newcode419
Line 419: for (int i = 0 ; i < -height_difference; i++) {
On 2009/01/07 16:00:35, Kevin Millikin wrote:
> No space before semicolon.  Don't like having to negate the upper bound.  Why
> not while (stack_pointer_ < expected->stack_pointer_) with an increment of
> stack_pointer_ in the loop?

Done.

http://codereview.chromium.org/16572/diff/8/9#newcode690
Line 690: if (elements_[frame_index].is_register()) {
On 2009/01/07 16:00:35, Kevin Millikin wrote:
> Please name elements_[frame_index].

Done.

http://codereview.chromium.org/16572/diff/8/9#newcode696
Line 696: // If target is already value->reg(), and is synced, keep it.
On 2009/01/07 16:00:35, Kevin Millikin wrote:
> This comment is wrong.  We keep the element even if it's not synced (and the
> sense of the comment is wrong for the comparison it describes---try something
> like "Unless the frame element agrees with the value, change it").

Done.

http://codereview.chromium.org/16572/diff/8/9#newcode1031
Line 1031: SetElementAt(num_dropped - 1, &tos);
On 2009/01/07 16:00:35, Kevin Millikin wrote:
> Isn't it a bit clearer to drop all but one of the elements first and then set
> the element at 0 to be the old tos element?

Done.

http://codereview.chromium.org/16572/diff/8/10
File src/virtual-frame-ia32.h (right):

http://codereview.chromium.org/16572/diff/8/10#newcode388
Line 388: // Nip removes one or more elements from immediately below the top
On 2009/01/07 16:00:35, Kevin Millikin wrote:
> Zero or more elements.

Done.

http://codereview.chromium.org/16572/diff/8/10#newcode389
Line 389: // of the stack, leaving TOS unchanged, but lower.  Nip(k) is
equivalent
On 2009/01/07 16:00:35, Kevin Millikin wrote:
> Not necessarily lower, but the more accurate "unchanged, but not higher"
sounds
> goofy.  Just say that it leaves the previous top-of-frame value on top of the
> frame (I've been trying to use frame for the virtual frame and stack to refer
> only to the processor's actual stack).

Done.