Rewrite of VisitCountOperation that should speed it up

Rewrite of VisitCountOperation that should speed it up

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

[Kevin Millikin (Chromium), 2009-04-02 09:24:46]

I guess you're still working on a different approach, so I'll hold off an a real
code review.

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

http://codereview.chromium.org/56151/diff/1/3#newcode4705
Line 4705: if (!is_postfix_) {  // Undo the optimistic SMI increment.
I think we normally just write "smi".  It's not an acronym.

http://codereview.chromium.org/56151/diff/1/3#newcode4707
Line 4707: frame->Spill(value.reg());  // Is this needed?
I don't think it is needed.  You could assert that the register's count is 1. 
Spill(Register) has an early bailout for not occurring in the frame, so it's OK
to leave it in.

http://codereview.chromium.org/56151/diff/1/3#newcode4775
Line 4775: value.ToRegister();
Maybe this is too eager (at least, if we think the setcc stuff that follows is
an optimization).  If the value is a constant, we might be putting it in the
last byte register which inhibits the optimization.  OTOH, delaying may cause a
spill and missing the optimization doesn't.

As a separate change, we should preform the operation at compile time if the
value is a constant smi.

http://codereview.chromium.org/56151/diff/1/3#newcode4776
Line 4776: ASSERT(value.is_valid());
No need for this after ToRegister.

[William Hesse, 2009-04-02 11:21:03]

New version uploaded, comments addressed.  The ToRegister is back to its even
more eager previous position, since constants don't work well with our
optimization.  We can handle constants earlier, or on a separate path, in the
future.

[Kevin Millikin (Chromium), 2009-04-02 12:39:32]

As we discussed, it might be good to try to avoid calling to number via the INC
builtin, since we know we've called it.  We should also assess whether the code
growth due to the slow case path is worth it.

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

http://codereview.chromium.org/56151/diff/1007/8#newcode4699
Line 4699: bool target_size)
How can this be a bool?  Aren't the possible target sizes 0, 1, or 2?

http://codereview.chromium.org/56151/diff/1007/8#newcode4721
Line 4721: // Undo the optimistic smi increment.
"smi increment or decrement"?  "smi operation"?

http://codereview.chromium.org/56151/diff/1007/8#newcode4757
Line 4757: // by a new register explicitly, to optimize the fast case.
Backed by a register until spilled, but it doesn't seem important to mention it
here.

http://codereview.chromium.org/56151/diff/1007/8#newcode4785
Line 4785: // old_value must be a register here, so that it is spilled by any
call.
This is confusing.  It's not that we need it to be spilled by a call, it's that
it can't be a constant or copy because we (might) modify it and it's below the
water mark for the deferred code exit.

Since something weird is (intentionally) happening here we should be really
explicit about what it is: we are not allowed to modify constants and copies
below the water mark on deferred exit frames.

[William Hesse, 2009-04-03 09:41:55]

ARM code generator has also been changed, to eliminate INC and DEC builtin.

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

http://codereview.chromium.org/56151/diff/1007/8#newcode4699
Line 4699: bool target_size)
On 2009/04/02 12:39:32, Kevin Millikin wrote:
> How can this be a bool?  Aren't the possible target sizes 0, 1, or 2?

Done.

http://codereview.chromium.org/56151/diff/1007/8#newcode4721
Line 4721: // Undo the optimistic smi increment.
On 2009/04/02 12:39:32, Kevin Millikin wrote:
> "smi increment or decrement"?  "smi operation"?

Done.

http://codereview.chromium.org/56151/diff/1007/8#newcode4785
Line 4785: // old_value must be a register here, so that it is spilled by any
call.
On 2009/04/02 12:39:32, Kevin Millikin wrote:
> This is confusing.  It's not that we need it to be spilled by a call, it's
that
> it can't be a constant or copy because we (might) modify it and it's below the
> water mark for the deferred code exit.
> 
> Since something weird is (intentionally) happening here we should be really
> explicit about what it is: we are not allowed to modify constants and copies
> below the water mark on deferred exit frames.

Done.

[Kevin Millikin (Chromium), 2009-04-03 11:59:42]

LGTM, but you should add a mjsunit test for at least the var x = "string"; x++
case.

You could also preemptively run the Webkit layout tests
fast/js/codegen-temporaries and fast/js/constant-folding.

http://codereview.chromium.org/56151/diff/1014/1015
File src/codegen-ia32.cc (right):

http://codereview.chromium.org/56151/diff/1014/1015#newcode4759
Line 4759: // by a new register explicitly, to optimize the fast case.
The comment about the new register seems out of place here.  I'd get rid of it.