Use unspilled virtual frames when compiling compare operations....

Use unspilled virtual frames when compiling compare operations.
Add xchg instruction to assembler and disassembler.
Add jump targets with two Result arguments.

[William Hesse, 2008-12-23 14:53:31]

A Christmas changelist!

[Kevin Millikin (Chromium), 2008-12-29 09:05:38]

Still needs some work.  I think you've (uniformly) swapped the order of the
frame elements in Comparison.

I'm also worried that the explicit register shuffling code before calling the
compare stub won't scale well.  At this site it is only temporary because we
will probably specialize the compare stub to its argument registers.

In the places where we actually need to shuffle registers, if any, we should
consider ways to reuse the same register-to-register move code as for the frame
merge.  We ought to be able to emit the same code automatically.

http://codereview.chromium.org/16450/diff/1/7
File src/assembler-ia32.cc (right):

http://codereview.chromium.org/16450/diff/1/7#newcode751
Line 751: dst = src;  // No need to copy eax to src, we assume src is eax.
More straightforward:
EMIT(0x90 | (dst.is(eax) ? src.code() : dst.code()));

http://codereview.chromium.org/16450/diff/1/8
File src/codegen-ia32.cc (right):

http://codereview.chromium.org/16450/diff/1/8#newcode1193
Line 1193: right_side = frame_->Pop();
I'm confused by this: the rhs is normally on top of the lhs on the stack, so I
would expect the rhs to be popped first in the normal case and the lhs to be
popped first in the reversed case.

http://codereview.chromium.org/16450/diff/1/8#newcode1213
Line 1213: __ cmp(right_side.reg(), Operand(left_side.reg()));
Here is where the variable naming/popping order confusion manifests itself.  Why
is right_side on the left?

http://codereview.chromium.org/16450/diff/1/8#newcode1214
Line 1214: // Fall through to |done|.
Did we need to change the order we emit the smi and non-smi cases?  We've now
grown an extra unconditional jump on the fast path.  We should instead work hard
to make merging work properly with the original basic block order (and leave the
original order for now even if its temporarily suboptimal).

If you leave it as you've changed it, please update the comments (here we're not
falling through to done, we're jumping to it....).

http://codereview.chromium.org/16450/diff/1/8#newcode1222
Line 1222: 
Remove the extra blank line.

http://codereview.chromium.org/16450/diff/1/8#newcode4201
Line 4201: frame_->Push(eax);  // push the result
Get rid of this explicit (and non-counted) reference to eax in non-spilled code.
 Instead, VirtualFrame::InvokeBuiltin could return a result that can be used to
manage the result register resource (you should also get rid of the version of
VirtualFrame::Push that takes a register).

http://codereview.chromium.org/16450/diff/1/8#newcode4209
Line 4209: __ test(eax, Operand(eax));
Ditto.

http://codereview.chromium.org/16450/diff/1/5
File src/jump-target-ia32.cc (right):

http://codereview.chromium.org/16450/diff/1/5#newcode178
Line 178: Result::Type arg_1_type = arg_1->type();
These assertions don't scale so well.  We should try to come up with a macro for
the binding site and a function for the test.  That can be a separate change.

http://codereview.chromium.org/16450/diff/1/6
File src/jump-target.h (right):

http://codereview.chromium.org/16450/diff/1/6#newcode101
Line 101: void Branch(Condition cc, Result* arg_1, Result* arg_2, Hint hint =
no_hint);
Please call these arguments arg0 and arg1 (zero based indexing like C++ and
without the underscore).

http://codereview.chromium.org/16450/diff/1/6#newcode107
Line 107: void Bind(Result* arg_1, Result* arg_2);
Ditto.

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

http://codereview.chromium.org/16450/diff/1/3#newcode135
Line 135: if (!frame_registers_.is_used(target.code())) return;
Use the overloaded version of is_used that takes a Register argument.

[William Hesse, 2008-12-29 11:47:42]

http://codereview.chromium.org/16450/diff/1/7
File src/assembler-ia32.cc (right):

http://codereview.chromium.org/16450/diff/1/7#newcode751
Line 751: dst = src;  // No need to copy eax to src, we assume src is eax.
On 2008/12/29 09:05:38, Kevin Millikin wrote:
> More straightforward:
> EMIT(0x90 | (dst.is(eax) ? src.code() : dst.code()));

Done.

http://codereview.chromium.org/16450/diff/1/8
File src/codegen-ia32.cc (right):

http://codereview.chromium.org/16450/diff/1/8#newcode1193
Line 1193: right_side = frame_->Pop();
On 2008/12/29 09:05:38, Kevin Millikin wrote:
> I'm confused by this: the rhs is normally on top of the lhs on the stack, so I
> would expect the rhs to be popped first in the normal case and the lhs to be
> popped first in the reversed case.

True.  Left and right were consistently flipped through the entire function. 
Left has been renamed to right, and right to left, so now they are correct.

http://codereview.chromium.org/16450/diff/1/8#newcode1213
Line 1213: __ cmp(right_side.reg(), Operand(left_side.reg()));
On 2008/12/29 09:05:38, Kevin Millikin wrote:
> Here is where the variable naming/popping order confusion manifests itself. 
Why
> is right_side on the left?

Done.

http://codereview.chromium.org/16450/diff/1/8#newcode1214
Line 1214: // Fall through to |done|.
On 2008/12/29 09:05:38, Kevin Millikin wrote:
> Did we need to change the order we emit the smi and non-smi cases?  We've now
> grown an extra unconditional jump on the fast path.  We should instead work
hard
> to make merging work properly with the original basic block order (and leave
the
> original order for now even if its temporarily suboptimal).
> 
> If you leave it as you've changed it, please update the comments (here we're
not
> falling through to done, we're jumping to it....).

Changed back, so there is not an unconditional jump on the fast case.
The JumpTarget "done" is bound to the slow case first, so we are probably
suboptimal until we handle merges better.

http://codereview.chromium.org/16450/diff/1/8#newcode1222
Line 1222: 
On 2008/12/29 09:05:38, Kevin Millikin wrote:
> Remove the extra blank line.

Done.

http://codereview.chromium.org/16450/diff/1/8#newcode4201
Line 4201: frame_->Push(eax);  // push the result
On 2008/12/29 09:05:38, Kevin Millikin wrote:
> Get rid of this explicit (and non-counted) reference to eax in non-spilled
code.
>  Instead, VirtualFrame::InvokeBuiltin could return a result that can be used
to
> manage the result register resource (you should also get rid of the version of
> VirtualFrame::Push that takes a register).

I would like to make this a separate change, to be done immediately.

http://codereview.chromium.org/16450/diff/1/8#newcode4209
Line 4209: __ test(eax, Operand(eax));
On 2008/12/29 09:05:38, Kevin Millikin wrote:
> Ditto.

To be done in separate change.

http://codereview.chromium.org/16450/diff/1/6
File src/jump-target.h (right):

http://codereview.chromium.org/16450/diff/1/6#newcode101
Line 101: void Branch(Condition cc, Result* arg_1, Result* arg_2, Hint hint =
no_hint);
On 2008/12/29 09:05:38, Kevin Millikin wrote:
> Please call these arguments arg0 and arg1 (zero based indexing like C++ and
> without the underscore).

Done.

http://codereview.chromium.org/16450/diff/1/6#newcode107
Line 107: void Bind(Result* arg_1, Result* arg_2);
On 2008/12/29 09:05:38, Kevin Millikin wrote:
> Ditto.

Done.

[William Hesse, 2008-12-29 12:57:06]

This changelist has been copied to a new issue number, because edits were made
on a different checked-out copy of V8.  Further comments and work are at
http://codereview.chromium.org/16481


On 2008/12/29 11:47:42, William Hesse wrote:
> http://codereview.chromium.org/16450/diff/1/7
> File src/assembler-ia32.cc (right):
> 
> http://codereview.chromium.org/16450/diff/1/7#newcode751
> Line 751: dst = src;  // No need to copy eax to src, we assume src is eax.
> On 2008/12/29 09:05:38, Kevin Millikin wrote:
> > More straightforward:
> > EMIT(0x90 | (dst.is(eax) ? src.code() : dst.code()));
> 
> Done.
> 
> http://codereview.chromium.org/16450/diff/1/8
> File src/codegen-ia32.cc (right):
> 
> http://codereview.chromium.org/16450/diff/1/8#newcode1193
> Line 1193: right_side = frame_->Pop();
> On 2008/12/29 09:05:38, Kevin Millikin wrote:
> > I'm confused by this: the rhs is normally on top of the lhs on the stack, so
I
> > would expect the rhs to be popped first in the normal case and the lhs to be
> > popped first in the reversed case.
> 
> True.  Left and right were consistently flipped through the entire function. 
> Left has been renamed to right, and right to left, so now they are correct.
> 
> http://codereview.chromium.org/16450/diff/1/8#newcode1213
> Line 1213: __ cmp(right_side.reg(), Operand(left_side.reg()));
> On 2008/12/29 09:05:38, Kevin Millikin wrote:
> > Here is where the variable naming/popping order confusion manifests itself. 
> Why
> > is right_side on the left?
> 
> Done.
> 
> http://codereview.chromium.org/16450/diff/1/8#newcode1214
> Line 1214: // Fall through to |done|.
> On 2008/12/29 09:05:38, Kevin Millikin wrote:
> > Did we need to change the order we emit the smi and non-smi cases?  We've
now
> > grown an extra unconditional jump on the fast path.  We should instead work
> hard
> > to make merging work properly with the original basic block order (and leave
> the
> > original order for now even if its temporarily suboptimal).
> > 
> > If you leave it as you've changed it, please update the comments (here we're
> not
> > falling through to done, we're jumping to it....).
> 
> Changed back, so there is not an unconditional jump on the fast case.
> The JumpTarget "done" is bound to the slow case first, so we are probably
> suboptimal until we handle merges better.
> 
> http://codereview.chromium.org/16450/diff/1/8#newcode1222
> Line 1222: 
> On 2008/12/29 09:05:38, Kevin Millikin wrote:
> > Remove the extra blank line.
> 
> Done.
> 
> http://codereview.chromium.org/16450/diff/1/8#newcode4201
> Line 4201: frame_->Push(eax);  // push the result
> On 2008/12/29 09:05:38, Kevin Millikin wrote:
> > Get rid of this explicit (and non-counted) reference to eax in non-spilled
> code.
> >  Instead, VirtualFrame::InvokeBuiltin could return a result that can be used
> to
> > manage the result register resource (you should also get rid of the version
of
> > VirtualFrame::Push that takes a register).
> 
> I would like to make this a separate change, to be done immediately.
> 
> http://codereview.chromium.org/16450/diff/1/8#newcode4209
> Line 4209: __ test(eax, Operand(eax));
> On 2008/12/29 09:05:38, Kevin Millikin wrote:
> > Ditto.
> 
> To be done in separate change.
> 
> http://codereview.chromium.org/16450/diff/1/6
> File src/jump-target.h (right):
> 
> http://codereview.chromium.org/16450/diff/1/6#newcode101
> Line 101: void Branch(Condition cc, Result* arg_1, Result* arg_2, Hint hint =
> no_hint);
> On 2008/12/29 09:05:38, Kevin Millikin wrote:
> > Please call these arguments arg0 and arg1 (zero based indexing like C++ and
> > without the underscore).
> 
> Done.
> 
> http://codereview.chromium.org/16450/diff/1/6#newcode107
> Line 107: void Bind(Result* arg_1, Result* arg_2);
> On 2008/12/29 09:05:38, Kevin Millikin wrote:
> > Ditto.
> 
> Done.