Handle array construction in native code (ARM version)

Handle array construction in native code (ARM version).

Ported the handle array construction in native code to ARM. See http://codereview.chromium.org/193125 for details. 

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

[Erik Corry, 2009-09-22 12:48:53]

Are you sure this runs the fast case?

http://codereview.chromium.org/217014/diff/1/2
File src/arm/builtins-arm.cc (right):

http://codereview.chromium.org/217014/diff/1/2#newcode69
Line 69: static const int kPreallocatedArrayElements = 4;
This should probably be moved out of the architecture independent files.

http://codereview.chromium.org/217014/diff/1/2#newcode73
Line 73: // register. If the parameter holes is larger than zero an elements
backing
holes -> 'holes'
zero -> zero,

http://codereview.chromium.org/217014/diff/1/2#newcode82
Line 82: int holes,
I would prefer a more descriptive name like preallocated_elements

http://codereview.chromium.org/217014/diff/1/2#newcode92
Line 92: if (holes > 0) {
I think we should just assert that holes is > 0 and remove this unrunnable code.

http://codereview.chromium.org/217014/diff/1/2#newcode116
Line 116: if (holes == 0) {
And here.

http://codereview.chromium.org/217014/diff/1/2#newcode149
Line 149: if (holes <= kLoopUnfoldLimit) {
And here assert that holes is less than kLoopUnfoldLimit.

http://codereview.chromium.org/217014/diff/1/2#newcode213
Line 213: __ AllocateObjectInNewSpace(scratch,
It doesn't look to me like AllocateObjectInNewSpace works if the first and 4th
parameters are the same register.

[Søren Thygesen Gjesse, 2009-09-23 07:03:52]

Fixed the bug you discovered, please take another look.

http://codereview.chromium.org/217014/diff/1/2
File src/arm/builtins-arm.cc (right):

http://codereview.chromium.org/217014/diff/1/2#newcode69
Line 69: static const int kPreallocatedArrayElements = 4;
On 2009/09/22 12:48:54, Erik Corry wrote:
> This should probably be moved out of the architecture independent files.

Done.

http://codereview.chromium.org/217014/diff/1/2#newcode73
Line 73: // register. If the parameter holes is larger than zero an elements
backing
On 2009/09/22 12:48:54, Erik Corry wrote:
> holes -> 'holes'
> zero -> zero,

Done.

http://codereview.chromium.org/217014/diff/1/2#newcode82
Line 82: int holes,
On 2009/09/22 12:48:54, Erik Corry wrote:
> I would prefer a more descriptive name like preallocated_elements

Changed to initial_capacity.

http://codereview.chromium.org/217014/diff/1/2#newcode92
Line 92: if (holes > 0) {
On 2009/09/22 12:48:54, Erik Corry wrote:
> I think we should just assert that holes is > 0 and remove this unrunnable
code.

Done.

http://codereview.chromium.org/217014/diff/1/2#newcode116
Line 116: if (holes == 0) {
On 2009/09/22 12:48:54, Erik Corry wrote:
> And here.

Done.

http://codereview.chromium.org/217014/diff/1/2#newcode149
Line 149: if (holes <= kLoopUnfoldLimit) {
On 2009/09/22 12:48:54, Erik Corry wrote:
> And here assert that holes is less than kLoopUnfoldLimit.

Removed kLoopUnfoldLimit (constant only used in an ASSERT requires #ifdef
DEBUG/#endif) and put in the constant 10.

http://codereview.chromium.org/217014/diff/1/2#newcode213
Line 213: __ AllocateObjectInNewSpace(scratch,
On 2009/09/22 12:48:54, Erik Corry wrote:
> It doesn't look to me like AllocateObjectInNewSpace works if the first and 4th
> parameters are the same register.

Good catch - the code from here and forward did not work at all. It was pure
luck that the tests passed. Just starting the ARMû shell crashed.

[Erik Corry, 2009-09-23 08:26:28]

LGTM.  Consider combining the size-from-register and size-in-constant cases to
reduce code size.

http://codereview.chromium.org/217014/diff/1/2
File src/arm/builtins-arm.cc (right):

http://codereview.chromium.org/217014/diff/1/2#newcode149
Line 149: if (holes <= kLoopUnfoldLimit) {
On 2009/09/23 07:03:52, Søren Gjesse wrote:
> On 2009/09/22 12:48:54, Erik Corry wrote:
> > And here assert that holes is less than kLoopUnfoldLimit.
> 
> Removed kLoopUnfoldLimit (constant only used in an ASSERT requires #ifdef
> DEBUG/#endif) and put in the constant 10.

That must be fixable.

http://codereview.chromium.org/217014/diff/4001/4002#newcode314
Line 314: // Handle construction of an empty array.
to large -> too large

http://codereview.chromium.org/217014/diff/4001/4002#newcode361
Line 361: __ add(sp, sp, Operand(2 * kPointerSize));
Note sentence starting "Note" therefore the use of grammar.

http://codereview.chromium.org/217014/diff/4001/4002#newcode407
Line 407: __ bind(&entry);
as -> as a

http://codereview.chromium.org/217014/diff/4001/4002#newcode410
Line 410: 
in case -> if

http://codereview.chromium.org/217014/diff/4001/4002#newcode430
Line 430: 
does always have -> always has

http://codereview.chromium.org/217014/diff/4001/4002#newcode442
Line 442: 
as -> as a

[Søren Thygesen Gjesse, 2009-09-23 08:55:26]

I will try combining the size-from-register and size-in-constant cases as a
separate change.

http://codereview.chromium.org/217014/diff/1/2
File src/arm/builtins-arm.cc (right):

http://codereview.chromium.org/217014/diff/1/2#newcode149
Line 149: if (holes <= kLoopUnfoldLimit) {
On 2009/09/23 08:26:28, Erik Corry wrote:
> On 2009/09/23 07:03:52, Søren Gjesse wrote:
> > On 2009/09/22 12:48:54, Erik Corry wrote:
> > > And here assert that holes is less than kLoopUnfoldLimit.
> > 
> > Removed kLoopUnfoldLimit (constant only used in an ASSERT requires #ifdef
> > DEBUG/#endif) and put in the constant 10.
> 
> That must be fixable.
> 
> 

It was. Done.

http://codereview.chromium.org/217014/diff/4001/4002#newcode314
Line 314: // Handle construction of an empty array.
On 2009/09/23 08:26:28, Erik Corry wrote:
> to large -> too large

Done.

http://codereview.chromium.org/217014/diff/4001/4002#newcode361
Line 361: __ add(sp, sp, Operand(2 * kPointerSize));
On 2009/09/23 08:26:28, Erik Corry wrote:
> Note sentence starting "Note" therefore the use of grammar. 

Done.

http://codereview.chromium.org/217014/diff/4001/4002#newcode407
Line 407: __ bind(&entry);
On 2009/09/23 08:26:28, Erik Corry wrote:
> as -> as a

Done.

http://codereview.chromium.org/217014/diff/4001/4002#newcode410
Line 410: 
On 2009/09/23 08:26:28, Erik Corry wrote:
> in case -> if

Done.

http://codereview.chromium.org/217014/diff/4001/4002#newcode430
Line 430: 
On 2009/09/23 08:26:28, Erik Corry wrote:
> does always have -> always has

Done.

http://codereview.chromium.org/217014/diff/4001/4002#newcode442
Line 442: 
On 2009/09/23 08:26:28, Erik Corry wrote:
> as -> as a

Done.