Port FastCloneShallowArrayStub to ARM and x64.

Port FastCloneShallowArrayStub to ARM and x64.


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

[Lasse Reichstein, 2010-02-10 17:41:27]

LGTM.

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

http://codereview.chromium.org/597022/diff/1/3#newcode4522
src/arm/codegen-arm.cc:4522: int size = JSArray::kSize + elements_size;
Pedantically, you should assert that either of the sizes is a multiple of
kObjectAlignment (otherwise the sum might be less than what is needed for two
aligned objects). A comment should suffice.

http://codereview.chromium.org/597022/diff/1/3#newcode4526
src/arm/codegen-arm.cc:4526: Label slow_case;
Please add a comment stating the order of arguments on the stack, or make
constants for their offsets.

http://codereview.chromium.org/597022/diff/1/3#newcode4548
src/arm/codegen-arm.cc:4548: __ str(r1, FieldMemOperand(r0, i));
There are only seven or eight fields to copy. If you unroll the loop manually,
you can interleave the loads and stores, putting an instruction between each
load and a dependent store.

http://codereview.chromium.org/597022/diff/1/3#newcode4562
src/arm/codegen-arm.cc:4562: __ str(r1, FieldMemOperand(r2, i));
A single unrolling (two loads followed by two stores in each iteration +
potentially a single copy at the end for parity) would reduce the wasted cycles.

If you have lots of free registers, you can consider using ldrm/strm to load and
store several in few instructions.

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

http://codereview.chromium.org/597022/diff/1/2#newcode6300
src/x64/codegen-x64.cc:6300: for (int i = 0; i < elements_size; i +=
kPointerSize) {
If elements_size gets big (for some value of "big"), it might be better to make
a computed loop in that case.
If that's not expected to happen, this should be fine.

[Mads Ager (chromium), 2010-02-11 12:24:02]

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

http://codereview.chromium.org/597022/diff/1/3#newcode4522
src/arm/codegen-arm.cc:4522: int size = JSArray::kSize + elements_size;
On 2010/02/10 17:41:27, Lasse Reichstein wrote:
> Pedantically, you should assert that either of the sizes is a multiple of
> kObjectAlignment (otherwise the sum might be less than what is needed for two
> aligned objects). A comment should suffice.

Added a comment stating that all sizes here are multiples of kPointerSize.

http://codereview.chromium.org/597022/diff/1/3#newcode4526
src/arm/codegen-arm.cc:4526: Label slow_case;
On 2010/02/10 17:41:27, Lasse Reichstein wrote:
> Please add a comment stating the order of arguments on the stack, or make
> constants for their offsets.

Done.

http://codereview.chromium.org/597022/diff/1/3#newcode4548
src/arm/codegen-arm.cc:4548: __ str(r1, FieldMemOperand(r0, i));
On 2010/02/10 17:41:27, Lasse Reichstein wrote:
> There are only seven or eight fields to copy. If you unroll the loop manually,
> you can interleave the loads and stores, putting an instruction between each
> load and a dependent store.

Good point.  Let's experiment with that on all platforms as a separate change.

http://codereview.chromium.org/597022/diff/1/3#newcode4562
src/arm/codegen-arm.cc:4562: __ str(r1, FieldMemOperand(r2, i));
On 2010/02/10 17:41:27, Lasse Reichstein wrote:
> A single unrolling (two loads followed by two stores in each iteration +
> potentially a single copy at the end for parity) would reduce the wasted
cycles.
> 
> If you have lots of free registers, you can consider using ldrm/strm to load
and
> store several in few instructions.

Good point.  Let's experiment with that on all platforms as a separate change.

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

http://codereview.chromium.org/597022/diff/1/2#newcode6300
src/x64/codegen-x64.cc:6300: for (int i = 0; i < elements_size; i +=
kPointerSize) {
On 2010/02/10 17:41:27, Lasse Reichstein wrote:
> If elements_size gets big (for some value of "big"), it might be better to
make
> a computed loop in that case.
> If that's not expected to happen, this should be fine.

Elements size is bounded because we check for
FastCloneShallowArrayStub::kMaximumLength before calling the stub.