Change inlined smi binary operations to use an unspilled frame.

Change inlined smi binary operations to use an unspilled frame.

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

[William Hesse, 2009-01-13 16:02:34]

Here is a change that uses unspilled frames for binary ops on Smis.  It is not
yet fully tested - it fails 6 tests with  CHECK(elements_.length() ==
expected->elements_.length()) failed.

But I would like feedback on it.  Thanks.

[William Hesse, 2009-01-14 14:08:16]

The code now passes all tests.  Take a look.

[Kevin Millikin (Chromium), 2009-01-15 10:28:56]

I just started looking at this.  Here are a few quick comments, I'll have more
later.

http://codereview.chromium.org/17380/diff/5/6
File src/codegen-ia32.cc (right):

http://codereview.chromium.org/17380/diff/5/6#newcode753
Line 753: // The arguments to the binary operation are passed in on the frame
(stack).
Unclear.  They may or may not be on the stack at run time, and there is no stack
at compile time.  "The arguments are on top of the code generator's frame."

http://codereview.chromium.org/17380/diff/5/6#newcode763
Line 763: Result eax_reg = generator()->allocator()->Allocate(eax);
Allocate the result in VirtualFrame::CallStub and return it, rather than at
every call site.  That gives a single place to change it rather than a bunch.

Just call it "result" or something.  Even though it might be eax, we don't care
here.

http://codereview.chromium.org/17380/diff/5/6#newcode813
Line 813: // TODO(): When this code is updated to use the virtual frame, it
Remove this comment.

http://codereview.chromium.org/17380/diff/5/6#newcode819
Line 819: // The operands are on the stack.
Frame?  I think it's obvious anyway.

http://codereview.chromium.org/17380/diff/5/6#newcode4784
Line 4784: // This function's implementation is a copy of
This comment is no longer true as of this change.  Please update.

http://codereview.chromium.org/17380/diff/5/6#newcode4805
Line 4805: Result right = frame->Pop();  // Get the right input to the
operation.
The comment doesn't seem to say any more (in fact, it says less) than the code
itself.

http://codereview.chromium.org/17380/diff/5/6#newcode4809
Line 4809: // Temp is used to compute the answer, leaving left and right
unchanged.
Then it's not just "temp".  Call it result or answer, maybe?

http://codereview.chromium.org/17380/diff/5/6#newcode4842
Line 4842: // Remove tag from one of the operands (but keep sign).
Not just one of the arguments, but the left one.

http://codereview.chromium.org/17380/diff/5/6#newcode4844
Line 4844: // We no longer require that the destination be eax.
Document the code as currently written, not what we used to do.  This comment
makes little sense to the reader.

http://codereview.chromium.org/17380/diff/5/6#newcode4853
Line 4853: Label non_zero_product_mul;
Comment why it's safe to have a raw Label here (no frame effect on either
branch).

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

http://codereview.chromium.org/17380/diff/5/8#newcode296
Line 296: ASSERT(!had_entry_frame || arg->type() == arg_type);
This ASSERT should go too (there is no reason that a register stays in a
register or a memory stays in a memory at a bind site).  Updating your workspace
should actually get rid of it anyway.

[William Hesse, 2009-01-15 13:50:38]

All comments so far, addressed.  Passes all tests.

http://codereview.chromium.org/17380/diff/5/6
File src/codegen-ia32.cc (right):

http://codereview.chromium.org/17380/diff/5/6#newcode753
Line 753: // The arguments to the binary operation are passed in on the frame
(stack).
On 2009/01/15 10:28:57, Kevin Millikin wrote:
> Unclear.  They may or may not be on the stack at run time, and there is no
stack
> at compile time.  "The arguments are on top of the code generator's frame."

Done.

http://codereview.chromium.org/17380/diff/5/6#newcode763
Line 763: Result eax_reg = generator()->allocator()->Allocate(eax);
On 2009/01/15 10:28:57, Kevin Millikin wrote:
> Allocate the result in VirtualFrame::CallStub and return it, rather than at
> every call site.  That gives a single place to change it rather than a bunch.
> 
> Just call it "result" or something.  Even though it might be eax, we don't
care
> here.

Done.

http://codereview.chromium.org/17380/diff/5/6#newcode813
Line 813: // TODO(): When this code is updated to use the virtual frame, it
On 2009/01/15 10:28:57, Kevin Millikin wrote:
> Remove this comment.

Done.

http://codereview.chromium.org/17380/diff/5/6#newcode4784
Line 4784: // This function's implementation is a copy of
On 2009/01/15 10:28:57, Kevin Millikin wrote:
> This comment is no longer true as of this change.  Please update.

Done.

http://codereview.chromium.org/17380/diff/5/6#newcode4805
Line 4805: Result right = frame->Pop();  // Get the right input to the
operation.
On 2009/01/15 10:28:57, Kevin Millikin wrote:
> The comment doesn't seem to say any more (in fact, it says less) than the code
> itself.

Done.

http://codereview.chromium.org/17380/diff/5/6#newcode4809
Line 4809: // Temp is used to compute the answer, leaving left and right
unchanged.
On 2009/01/15 10:28:57, Kevin Millikin wrote:
> Then it's not just "temp".  Call it result or answer, maybe?

Done.

http://codereview.chromium.org/17380/diff/5/6#newcode4842
Line 4842: // Remove tag from one of the operands (but keep sign).
On 2009/01/15 10:28:57, Kevin Millikin wrote:
> Not just one of the arguments, but the left one.

Done.

http://codereview.chromium.org/17380/diff/5/6#newcode4844
Line 4844: // We no longer require that the destination be eax.
On 2009/01/15 10:28:57, Kevin Millikin wrote:
> Document the code as currently written, not what we used to do.  This comment
> makes little sense to the reader.

Done.

http://codereview.chromium.org/17380/diff/5/6#newcode4853
Line 4853: Label non_zero_product_mul;
On 2009/01/15 10:28:57, Kevin Millikin wrote:
> Comment why it's safe to have a raw Label here (no frame effect on either
> branch).

Done.

[Kevin Millikin (Chromium), 2009-01-15 13:56:37]

It looks pretty good.  I'd like to see the revision.

http://codereview.chromium.org/17380/diff/5/6
File src/codegen-ia32.cc (right):

http://codereview.chromium.org/17380/diff/5/6#newcode4850
Line 4850: {
Not sure why there's a block here?

http://codereview.chromium.org/17380/diff/5/6#newcode4876
Line 4876: Label non_zero_product_div;
The result of division is not a "product" :)

http://codereview.chromium.org/17380/diff/5/6#newcode4894
Line 4894: __ lea(reg_eax.reg(), Operand(eax, eax, times_1, kSmiTag));
Isn't this temp.reg()?

http://codereview.chromium.org/17380/diff/5/6#newcode4910
Line 4910: Label non_zero_product_mod;
The result of mod, whatever it's called, is probably not a "product".

http://codereview.chromium.org/17380/diff/5/6#newcode4918
Line 4918: __ mov(reg_eax.reg(), Operand(reg_edx.reg()));
Shouldn't this be temp.reg() instead of reg_eax.reg()?

http://codereview.chromium.org/17380/diff/5/6#newcode4936
Line 4936: // Move right into ecx.
Comment explaining why (because we will use a shift by cl encoding).

http://codereview.chromium.org/17380/diff/5/6#newcode4973
Line 4973: __ sar(ecx, kSmiTagSize);
Why not right.reg()?

http://codereview.chromium.org/17380/diff/5/6#newcode4993
Line 4993: JumpTarget shr_result_ok(generator());
Is it safe to make this a raw Label, because there is no frame effect that needs
to be merged on either path?  Otherwise, you will need a temp.ToRegister for
safety below just before the lea instruction.

http://codereview.chromium.org/17380/diff/5/6#newcode5009
Line 5009: Result smi_test_reg = generator()->allocator()->Allocate();
Throw in an assert that this register is valid for safety.

[William Hesse, 2009-01-15 15:33:39]

http://codereview.chromium.org/17380/diff/5/6
File src/codegen-ia32.cc (right):

http://codereview.chromium.org/17380/diff/5/6#newcode4850
Line 4850: {
On 2009/01/15 13:56:37, Kevin Millikin wrote:
> Not sure why there's a block here?

The enclosing switch statement jumps past the initialization of Label
non_zero_product_mul when jumping to other cases, and the compiler complains.

http://codereview.chromium.org/17380/diff/5/6#newcode4876
Line 4876: Label non_zero_product_div;
On 2009/01/15 13:56:37, Kevin Millikin wrote:
> The result of division is not a "product" :)

Done.

http://codereview.chromium.org/17380/diff/5/6#newcode4894
Line 4894: __ lea(reg_eax.reg(), Operand(eax, eax, times_1, kSmiTag));
On 2009/01/15 13:56:37, Kevin Millikin wrote:
> Isn't this temp.reg()?
They are the same in this case.  Changed.

Done.

http://codereview.chromium.org/17380/diff/5/6#newcode4910
Line 4910: Label non_zero_product_mod;
On 2009/01/15 13:56:37, Kevin Millikin wrote:
> The result of mod, whatever it's called, is probably not a "product".

Done.

http://codereview.chromium.org/17380/diff/5/6#newcode4936
Line 4936: // Move right into ecx.
On 2009/01/15 13:56:37, Kevin Millikin wrote:
> Comment explaining why (because we will use a shift by cl encoding).

Done.

http://codereview.chromium.org/17380/diff/5/6#newcode4993
Line 4993: JumpTarget shr_result_ok(generator());
On 2009/01/15 13:56:37, Kevin Millikin wrote:
> Is it safe to make this a raw Label, because there is no frame effect that
needs
> to be merged on either path?  Otherwise, you will need a temp.ToRegister for
> safety below just before the lea instruction.

Yes.

http://codereview.chromium.org/17380/diff/5/6#newcode5009
Line 5009: Result smi_test_reg = generator()->allocator()->Allocate();
On 2009/01/15 13:56:37, Kevin Millikin wrote:
> Throw in an assert that this register is valid for safety.

Done.

[Kevin Millikin (Chromium), 2009-01-15 15:47:03]

LGTM.

http://codereview.chromium.org/17380/diff/219/220
File src/codegen-ia32.cc (right):

http://codereview.chromium.org/17380/diff/219/220#newcode4921
Line 4921: // Tag the result and store it in register temp.
the answer register.