Elements kind conversion in generated code (ia32).

Elements kind conversion in generated code (ia32).

BUG=
TEST=

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

[Yang, 2011-10-12 14:30:02]

PTaL.

[Jakob Kummerow, 2011-10-13 09:15:59]

Looks good so far, my comments are mostly nits. As discussed, you'll have to
take care of the other architectures before you can land this.

http://codereview.chromium.org/8241003/diff/1008/src/code-stubs.h
File src/code-stubs.h (right):

http://codereview.chromium.org/8241003/diff/1008/src/code-stubs.h#newcode1041
src/code-stubs.h:1041: private:
nit: classes, then methods, then data members. (see
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar...)

http://codereview.chromium.org/8241003/diff/1008/src/ia32/code-stubs-ia32.cc
File src/ia32/code-stubs-ia32.cc (right):

http://codereview.chromium.org/8241003/diff/1008/src/ia32/code-stubs-ia32.cc#...
src/ia32/code-stubs-ia32.cc:7047: // edi: source FixedArray
Please move this comment to some place where the statement is true.

http://codereview.chromium.org/8241003/diff/1008/src/ia32/code-stubs-ia32.cc#...
src/ia32/code-stubs-ia32.cc:7065: // edi: source FixedArray
You can omit all comments for registers whose contents have not changed since
the last comment. Up to you.

http://codereview.chromium.org/8241003/diff/1008/src/ia32/code-stubs-ia32.cc#...
src/ia32/code-stubs-ia32.cc:7115: // Restore necessary values do store.
I don't understand this comment.

http://codereview.chromium.org/8241003/diff/1008/src/ia32/code-stubs-ia32.cc#...
src/ia32/code-stubs-ia32.cc:7132: // Restore esp and esi before jumping into
runtime.
Slightly outdated. s/esp and esi/registers/?

http://codereview.chromium.org/8241003/diff/1008/src/ia32/code-stubs-ia32.cc#...
src/ia32/code-stubs-ia32.cc:7141: void
FastElementsConversionStub::GenerateDoubleToObject(
TODO(jkummerow): continue reviewing here.

http://codereview.chromium.org/8241003/diff/1008/src/ia32/code-stubs-ia32.cc#...
src/ia32/code-stubs-ia32.cc:7159: // edx: receiver
Again, you can omit comments about registers that haven't changed.

http://codereview.chromium.org/8241003/diff/1008/src/ia32/code-stubs-ia32.cc#...
src/ia32/code-stubs-ia32.cc:7164: // eax: destination FixedDoubleArray
s/FixedDoubleArray/FixedArray/

http://codereview.chromium.org/8241003/diff/1008/src/ia32/code-stubs-ia32.cc#...
src/ia32/code-stubs-ia32.cc:7167: // edi: source FixedArray
Please move this to where it's true. Also, s/FixedArray/FixedDoubleArray/. Or
just delete this comment here since you have the correct version four lines
below.

http://codereview.chromium.org/8241003/diff/1008/src/ia32/code-stubs-ia32.cc#...
src/ia32/code-stubs-ia32.cc:7176: // ebx: index of current element (smi-tagged)
Move this comment.

http://codereview.chromium.org/8241003/diff/1008/src/ia32/code-stubs-ia32.cc#...
src/ia32/code-stubs-ia32.cc:7187: __ mov(esi, FieldOperand(edi, ebx, times_4,
FixedDoubleArray::kHeaderSize));
You could check if SSE2 is available and if yes, do a single 64bit move using an
xmm register, falling back to two 32bit moves via esi otherwise.

http://codereview.chromium.org/8241003/diff/1008/src/ia32/code-stubs-ia32.cc#...
src/ia32/code-stubs-ia32.cc:7223: __ mov(ebx, eax);
This move is unnecessary. EAX is popped next anyway, so it's OK for it to be
clobbered.

http://codereview.chromium.org/8241003/diff/1008/src/ic.cc
File src/ic.cc (right):

http://codereview.chromium.org/8241003/diff/1008/src/ic.cc#newcode1784
src/ic.cc:1784: 
nit: remove this empty line

http://codereview.chromium.org/8241003/diff/1008/test/mjsunit/elements-kind.js
File test/mjsunit/elements-kind.js (right):

http://codereview.chromium.org/8241003/diff/1008/test/mjsunit/elements-kind.j...
test/mjsunit/elements-kind.js:103: assertEquals(expected, getKind(obj),
name_opt);
Nice! I like this refactoring.

http://codereview.chromium.org/8241003/diff/1008/test/mjsunit/elements-kind.j...
test/mjsunit/elements-kind.js:186: //%OptimizeFunctionOnNextCall(polymorphic);
Please add a comment stating why this section is disabled. On a side note, I'd
prefer /*...*/, but leave that up to you.

http://codereview.chromium.org/8241003/diff/1008/test/mjsunit/elements-transi...
File test/mjsunit/elements-transition.js (right):

http://codereview.chromium.org/8241003/diff/1008/test/mjsunit/elements-transi...
test/mjsunit/elements-transition.js:33: var me = new Array(length);
As discussed, please add a comment explaining why every operation here needs to
be performed for two identical arrays.

[Yang, 2011-10-13 09:58:53]

PTAL.

http://codereview.chromium.org/8241003/diff/1008/src/code-stubs.h
File src/code-stubs.h (right):

http://codereview.chromium.org/8241003/diff/1008/src/code-stubs.h#newcode1041
src/code-stubs.h:1041: private:
On 2011/10/13 09:15:59, Jakob wrote:
> nit: classes, then methods, then data members. (see
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar...)

Done.

http://codereview.chromium.org/8241003/diff/1008/src/ia32/code-stubs-ia32.cc
File src/ia32/code-stubs-ia32.cc (right):

http://codereview.chromium.org/8241003/diff/1008/src/ia32/code-stubs-ia32.cc#...
src/ia32/code-stubs-ia32.cc:7065: // edi: source FixedArray
On 2011/10/13 09:15:59, Jakob wrote:
> You can omit all comments for registers whose contents have not changed since
> the last comment. Up to you.

Done.

http://codereview.chromium.org/8241003/diff/1008/src/ia32/code-stubs-ia32.cc#...
src/ia32/code-stubs-ia32.cc:7115: // Restore necessary values do store.
On 2011/10/13 09:15:59, Jakob wrote:
> I don't understand this comment.

Removed and replaced by comments below.

http://codereview.chromium.org/8241003/diff/1008/src/ia32/code-stubs-ia32.cc#...
src/ia32/code-stubs-ia32.cc:7132: // Restore esp and esi before jumping into
runtime.
On 2011/10/13 09:15:59, Jakob wrote:
> Slightly outdated. s/esp and esi/registers/?

Done.

http://codereview.chromium.org/8241003/diff/1008/src/ia32/code-stubs-ia32.cc#...
src/ia32/code-stubs-ia32.cc:7187: __ mov(esi, FieldOperand(edi, ebx, times_4,
FixedDoubleArray::kHeaderSize));
On 2011/10/13 09:15:59, Jakob wrote:
> You could check if SSE2 is available and if yes, do a single 64bit move using
an
> xmm register, falling back to two 32bit moves via esi otherwise.

Done.

http://codereview.chromium.org/8241003/diff/1008/src/ia32/code-stubs-ia32.cc#...
src/ia32/code-stubs-ia32.cc:7223: __ mov(ebx, eax);
On 2011/10/13 09:15:59, Jakob wrote:
> This move is unnecessary. EAX is popped next anyway, so it's OK for it to be
> clobbered.

Done.

http://codereview.chromium.org/8241003/diff/1008/src/ic.cc
File src/ic.cc (right):

http://codereview.chromium.org/8241003/diff/1008/src/ic.cc#newcode1784
src/ic.cc:1784: 
On 2011/10/13 09:15:59, Jakob wrote:
> nit: remove this empty line

Done.

[Jakob Kummerow, 2011-10-13 10:40:55]

LGTM