X64: Faster push/pop implementation.

X64: Faster push/pop implementation.
Also snuck in an intended optimization for fast api call preparation and a few indentation fixes.

[Lasse Reichstein, 2010-04-23 10:59:19]

Port of your Push/Pop optimization to X64.
Bug compatible.

[antonm, 2010-04-23 12:05:48]

http://codereview.chromium.org/1689010/diff/1/2
File src/ia32/stub-cache-ia32.cc (right):

http://codereview.chromium.org/1689010/diff/1/2#newcode1331
src/ia32/stub-cache-ia32.cc:1331: __ j(above, &call_builtin);
just for my education: why this change?

http://codereview.chromium.org/1689010/diff/1/2#newcode1429
src/ia32/stub-cache-ia32.cc:1429: __ j(equal, &return_undefined);
what if there is another object in prototype chain with this element, should'nt
we return it?

a = new Array(1)
a.__proto__[0] = 1
assertEquals(1, a.pop())

but with your change I would expect undefined here, correct?

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

http://codereview.chromium.org/1689010/diff/1/3#newcode7779
src/x64/codegen-x64.cc:7779: masm->RecordWriteHelper(object_, addr_, scratch_);
just for my education, why masm-> form instead of __ ?  I know it's the same on
ia32, just asking

http://codereview.chromium.org/1689010/diff/1/5
File src/x64/macro-assembler-x64.cc (right):

http://codereview.chromium.org/1689010/diff/1/5#newcode198
src/x64/macro-assembler-x64.cc:198: shr(scratch, Immediate(kPointerSizeLog2));
why this change?

http://codereview.chromium.org/1689010/diff/1/6
File src/x64/macro-assembler-x64.h (right):

http://codereview.chromium.org/1689010/diff/1/6#newcode543
src/x64/macro-assembler-x64.h:543: Register scratch, int save_at_depth,
do you need it in this change?  that should be API calls directed

http://codereview.chromium.org/1689010/diff/1/7
File src/x64/stub-cache-x64.cc (right):

http://codereview.chromium.org/1689010/diff/1/7#newcode558
src/x64/stub-cache-x64.cc:558: // Holds information about possible function call
optimizations.
ditto is not probably needed for this change.  And I suspect Pavel is already
doing this port

http://codereview.chromium.org/1689010/diff/1/7#newcode1199
src/x64/stub-cache-x64.cc:1199: __ j(greater, &attempt_to_grow_elements);
above instead of greater?  just asking

http://codereview.chromium.org/1689010/diff/1/7#newcode1209
src/x64/stub-cache-x64.cc:1209: index.reg, index.scale,
indent off

http://codereview.chromium.org/1689010/diff/1/7#newcode1348
src/x64/stub-cache-x64.cc:1348: __ j(equal, &return_undefined);
ditto if it's valid for elements on proto

[Lasse Reichstein, 2010-04-23 12:33:04]

Thanks for comments. I'll remove my over-optimization, see if I can remove the
api-stuff and reupload.
If I don't make it by today, have a good week-end.

http://codereview.chromium.org/1689010/diff/1/2
File src/ia32/stub-cache-ia32.cc (right):

http://codereview.chromium.org/1689010/diff/1/2#newcode1331
src/ia32/stub-cache-ia32.cc:1331: __ j(above, &call_builtin);
Because it is conceivable (although not likely) that our heap can cross the
0x80000000 address boundary, and that would make "greater" give the wrong result
(since the larger address is considered negative).

Well, actually, it can't happen the way we currently allocate our NewSpace, but
using unsigned comparison for addresses is still "more correct".

http://codereview.chromium.org/1689010/diff/1/2#newcode1429
src/ia32/stub-cache-ia32.cc:1429: __ j(equal, &return_undefined);
Good point. Too optimistic an assumption here.

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

http://codereview.chromium.org/1689010/diff/1/3#newcode7779
src/x64/codegen-x64.cc:7779: masm->RecordWriteHelper(object_, addr_, scratch_);
Heh, funny story. It's this way here because I copied it from IA32 :).
I.e., I have no idea, and it probably should just be __ for consistency.

http://codereview.chromium.org/1689010/diff/1/5
File src/x64/macro-assembler-x64.cc (right):

http://codereview.chromium.org/1689010/diff/1/5#newcode198
src/x64/macro-assembler-x64.cc:198: shr(scratch, Immediate(kPointerSizeLog2));
We are finding the index of a pointer position in the page. We have one bit in
the RSet per pointer (per kPointerSize bytes), so the correct thing to do here
is to divide by kPointerSize (which we can do by a shift since it's a power of
2).
Using kObjectAlignmentBits is only working by coincidence. We could have an
object alignment that is larger than pointer-size (although it better be a
multiple of it :).

http://codereview.chromium.org/1689010/diff/1/6
File src/x64/macro-assembler-x64.h (right):

http://codereview.chromium.org/1689010/diff/1/6#newcode543
src/x64/macro-assembler-x64.h:543: Register scratch, int save_at_depth,
I don't know. It seems to be used, although I'm not sure exactly what it means.

http://codereview.chromium.org/1689010/diff/1/7
File src/x64/stub-cache-x64.cc (right):

http://codereview.chromium.org/1689010/diff/1/7#newcode558
src/x64/stub-cache-x64.cc:558: // Holds information about possible function call
optimizations.
Great. Let me see if I can remove it and still run the rest.

http://codereview.chromium.org/1689010/diff/1/7#newcode1199
src/x64/stub-cache-x64.cc:1199: __ j(greater, &attempt_to_grow_elements);
I thought about it and decided aginst it.
Smi values are signed, so to use unsigned comparison should only be to combine a
greater-than-or-negative test in one comparison. I see no reason to believe that
either register should hold a negative value (both are read from fields that
must only hold positive values), so testing for negativity would actually be
confusing at this point (the reader would start thinking about how the value
could possibly be positive).

http://codereview.chromium.org/1689010/diff/1/7#newcode1209
src/x64/stub-cache-x64.cc:1209: index.reg, index.scale,
Fixed.

http://codereview.chromium.org/1689010/diff/1/7#newcode1348
src/x64/stub-cache-x64.cc:1348: __ j(equal, &return_undefined);
Over-optimized. Will fix.

[Lasse Reichstein, 2010-04-28 08:07:41]

http://codereview.chromium.org/1689010/diff/1/7
File src/x64/stub-cache-x64.cc (right):

http://codereview.chromium.org/1689010/diff/1/7#newcode558
src/x64/stub-cache-x64.cc:558: // Holds information about possible function call
optimizations.
A thought: Have I ported the entire API optimization, or just part of it. If
it's all of it, I might as well commit it instead of trying to figure out what I
can remove :)

[podivilov, 2010-04-28 14:16:33]

Hi Lasse,

I've already ported inline type checks for API calls
-http://codereview.chromium.org/1650011/show,
http://code.google.com/p/v8/source/detail?r=4540.

Sorry for inconvenience, I'd better commit it earlier.

BR,
Pavel Podivilov

[Lasse Reichstein, 2010-05-03 13:18:28]

Think I git it cleaned up now.
Care for another look?

[antonm, 2010-05-04 12:42:45]

LGTM and thanks a lot for port.

http://codereview.chromium.org/1689010/diff/9001/10003
File src/x64/codegen-x64.h (right):

http://codereview.chromium.org/1689010/diff/9001/10003#newcode1014
src/x64/codegen-x64.h:1014: class RecordWriteStub : public CodeStub {
I think Mads already has ported that to x64 :)

http://codereview.chromium.org/1689010/diff/9001/10006
File src/x64/stub-cache-x64.cc (right):

http://codereview.chromium.org/1689010/diff/9001/10006#newcode567
src/x64/stub-cache-x64.cc:567: __ movq(scratch, Operand(rsp, 0));
I am fine if you keep those changes here.  But it might be a good idea to extend
CL description.  Or maybe do it in a separate CL as all modifications in this
file are not related to array's push/pop

http://codereview.chromium.org/1689010/diff/9001/10007
File test/mjsunit/array-pop.js (right):

http://codereview.chromium.org/1689010/diff/9001/10007#newcode72
test/mjsunit/array-pop.js:72: assertEquals(j+1, a.length, "inherit-pre-length");
should + be surrounded with space?

http://codereview.chromium.org/1689010/diff/9001/10007#newcode72
test/mjsunit/array-pop.js:72: assertEquals(j+1, a.length, "inherit-pre-length");
do you want add j to message, something like "inherit-pre-length-" + j?

[Lasse Reichstein, 2010-05-04 13:23:33]

http://codereview.chromium.org/1689010/diff/9001/10003
File src/x64/codegen-x64.h (right):

http://codereview.chromium.org/1689010/diff/9001/10003#newcode1014
src/x64/codegen-x64.h:1014: class RecordWriteStub : public CodeStub {
Correct, it got me a few merge failures when forwarding my change to head of
bleeding_edge (but only in indentation and comments, the code must be obvious).

http://codereview.chromium.org/1689010/diff/9001/10006
File src/x64/stub-cache-x64.cc (right):

http://codereview.chromium.org/1689010/diff/9001/10006#newcode567
src/x64/stub-cache-x64.cc:567: __ movq(scratch, Operand(rsp, 0));
Will modify CL description.

http://codereview.chromium.org/1689010/diff/9001/10007
File test/mjsunit/array-pop.js (right):

http://codereview.chromium.org/1689010/diff/9001/10007#newcode72
test/mjsunit/array-pop.js:72: assertEquals(j+1, a.length, "inherit-pre-length");
On 2010/05/04 12:42:45, antonm wrote:
> should + be surrounded with space?

Done.

http://codereview.chromium.org/1689010/diff/9001/10007#newcode72
test/mjsunit/array-pop.js:72: assertEquals(j+1, a.length, "inherit-pre-length");
Good idea. Done.