Porting r9456 to x64 (Optimize KeyedStoreGeneric for Smi arrays).

Porting r9456 to x64 (Optimize KeyedStoreGeneric for Smi arrays).

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

[Yang, 2011-09-28 08:03:02]

Please take a look. This is a port of http://codereview.chromium.org/8022002/

[Rico, 2011-09-28 09:11:36]

Drive by

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

http://codereview.chromium.org/8054043/diff/1/src/x64/macro-assembler-x64.cc#...
src/x64/macro-assembler-x64.cc:2729: push(kScratchRegister);
Why use the fpu stack here? why not use sse2 instructions to do this>

[Yang, 2011-09-28 09:22:42]

Changed int-to-double conversion to use SSE2 instructions.

[fschneider, 2011-09-29 10:18:50]

Comments also apply to ia32.

http://codereview.chromium.org/8054043/diff/8001/src/x64/ic-x64.cc
File src/x64/ic-x64.cc (right):

http://codereview.chromium.org/8054043/diff/8001/src/x64/ic-x64.cc#newcode668
src/x64/ic-x64.cc:668: __ CheckMap(rbx, FACTORY->fixed_array_map(),
Use masm->isolate()->factory() instead. FACTORY exands into
Isolate::Current()->factory which is usually more expensive.

http://codereview.chromium.org/8054043/diff/8001/src/x64/ic-x64.cc#newcode675
src/x64/ic-x64.cc:675: __ CheckMap(rbx, FACTORY->fixed_double_array_map(),
&slow, DONT_DO_SMI_CHECK);
You should combine the two map checks to avoid duplicating the code afterwards.
Somehow I prefer the lower level instructions to these complicated
macro-instructions. This way you can load the map into a register only once and
save a memory access.

[Yang, 2011-09-29 11:57:43]

Changed according to suggestions and also applied those to the ia32 code.

Please take another look.

http://codereview.chromium.org/8054043/diff/8001/src/x64/ic-x64.cc
File src/x64/ic-x64.cc (right):

http://codereview.chromium.org/8054043/diff/8001/src/x64/ic-x64.cc#newcode668
src/x64/ic-x64.cc:668: __ CheckMap(rbx, FACTORY->fixed_array_map(),
Using CompareRoot instead.

On 2011/09/29 10:18:55, fschneider wrote:
> Use masm->isolate()->factory() instead. FACTORY exands into
> Isolate::Current()->factory which is usually more expensive.

http://codereview.chromium.org/8054043/diff/8001/src/x64/ic-x64.cc#newcode675
src/x64/ic-x64.cc:675: __ CheckMap(rbx, FACTORY->fixed_double_array_map(),
&slow, DONT_DO_SMI_CHECK);
Loaded the map into rdi first. However, I'm not entirely sure what you had in
mind to avoid duplicating code. I don't see how the duplicate code for the
increment of the length field can be avoided.

On 2011/09/29 10:18:55, fschneider wrote:
> You should combine the two map checks to avoid duplicating the code
afterwards.
> Somehow I prefer the lower level instructions to these complicated
> macro-instructions. This way you can load the map into a register only once
and
> save a memory access.

[fschneider, 2011-09-29 13:38:32]

LGTM if you enable unit test coverage for smi-only-arrays as discussed offline.