Handle array construction on native code

Handle array construction on native code.

The construction of arrays when using the the Array function either as a constructor or a normal function is now handled fully in generated code in most cases. Only when Array is called with one argument which is either negative or abowe JSObject::kInitialMaxFastElementArray (which is currently 1000) or if the allocated object cannot fit in the room left in new space is the runtime system entered.

Two new native code built-in functions are added one for normal invocation and one for the construct call. The existing C++ builtin is renamed, but kept. When the normal invocation cannot be handled in generated code the C++ builtin is called. When the construct invocation cannot be handled in native code the generic construct stub is called (which will end up in the C++ builtin through a construct trampoline).

One thing that might be changed is preserving esi (constructor function) during the handling of a construct call. We know precisily what function we where calling anyway and can just reload it. This could remove the parameter construct_call to ArrayNativeCode and remove the handling of this from that function.

The X64 and ARM implementations are not part of this changelist.

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

[Lasse Reichstein, 2009-09-16 08:03:01]

Drive by comment.

http://codereview.chromium.org/193125/diff/1/3
File src/ia32/builtins-ia32.cc (right):

http://codereview.chromium.org/193125/diff/1/3#newcode940
Line 940: __ test(Operand(esp, (push_count + 1) * kPointerSize),
Immediate(0xc0000000));
You can combine the test for a non-negative smi into a single test:
 test(Operand(esp, (push_count + 1) * kPointerSize),
      Immediate(0x80000001));
 j(not_zero, &prepeare_generic_code_call);

[Mads Ager (chromium), 2009-09-16 10:46:13]

LGTM

http://codereview.chromium.org/193125/diff/1/5
File src/builtins.cc (left):

http://codereview.chromium.org/193125/diff/1/5#oldcode169
Line 169: if (args.length() == 2) return array->SetElementsLength(args[1]);
Good catch. :)

http://codereview.chromium.org/193125/diff/1/3
File src/ia32/builtins-ia32.cc (right):

http://codereview.chromium.org/193125/diff/1/3#newcode677
Line 677: // register. If the parameter holes larger than zero an elements
backing store
holes larger -> holes is larger

http://codereview.chromium.org/193125/diff/1/3#newcode698
Line 698: size += FixedArray::kHeaderSize + holes * kPointerSize;
You could use FixedArray::SizeFor(holes) here instead.

http://codereview.chromium.org/193125/diff/1/3#newcode713
Line 713: __ mov(scratch1, Factory::empty_fixed_array());
You don't need to use the scratch register here.  You should be able to use __
mov(FieldOperand(...), Factory::empty_fixed_array())?

http://codereview.chromium.org/193125/diff/1/3#newcode718
Line 718: // If no storage is requested for the the elements array just set the
empty
the the -> the

http://codereview.chromium.org/193125/diff/1/3#newcode721
Line 721: __ mov(scratch3, Factory::empty_fixed_array());
No need for using the scratch register.

http://codereview.chromium.org/193125/diff/1/3#newcode738
Line 738: __ mov(scratch3, Factory::fixed_array_map());
Remove?

http://codereview.chromium.org/193125/diff/1/3#newcode743
Line 743: __ mov(scratch3, Factory::the_hole_value());
Push this into only the unfolded loop to avoid the extra reloc info?  Use it
directly in the loop that is not unfolded (since there will be only one
occurrence)?

http://codereview.chromium.org/193125/diff/1/3#newcode795
Line 795: FixedArray::kHeaderSize +
FixedArray::SizeFor(kPreallocatedArrayElements)?

http://codereview.chromium.org/193125/diff/1/3#newcode847
Line 847: __ mov(scratch, Factory::fixed_array_map());
Remove?

http://codereview.chromium.org/193125/diff/1/3#newcode869
Line 869: __ mov(scratch, Factory::the_hole_value());
Remove.  

Do you want to unfold this loop as well if it is small?

http://codereview.chromium.org/193125/diff/1/3#newcode1041
Line 1041: // REstore argc and constructor before running the generic code.
REstore -> Restore.

http://codereview.chromium.org/193125/diff/1/3#newcode1062
Line 1062: #ifdef DEBUG
if (FLAG_debug_code) {

?

http://codereview.chromium.org/193125/diff/1/3#newcode1093
Line 1093: #ifdef DEBUG
if (FLAG_debug_code) {

?

[Søren Thygesen Gjesse, 2009-09-16 11:17:21]

http://codereview.chromium.org/193125/diff/1/3
File src/ia32/builtins-ia32.cc (right):

http://codereview.chromium.org/193125/diff/1/3#newcode677
Line 677: // register. If the parameter holes larger than zero an elements
backing store
On 2009/09/16 10:46:13, Mads Ager wrote:
> holes larger -> holes is larger

Done.

http://codereview.chromium.org/193125/diff/1/3#newcode698
Line 698: size += FixedArray::kHeaderSize + holes * kPointerSize;
On 2009/09/16 10:46:13, Mads Ager wrote:
> You could use FixedArray::SizeFor(holes) here instead.

Done.

http://codereview.chromium.org/193125/diff/1/3#newcode713
Line 713: __ mov(scratch1, Factory::empty_fixed_array());
On 2009/09/16 10:46:13, Mads Ager wrote:
> You don't need to use the scratch register here.  You should be able to use __
> mov(FieldOperand(...), Factory::empty_fixed_array())?

Done - I guess I was being ARM'ish.

http://codereview.chromium.org/193125/diff/1/3#newcode718
Line 718: // If no storage is requested for the the elements array just set the
empty
On 2009/09/16 10:46:13, Mads Ager wrote:
> the the -> the

Done.

http://codereview.chromium.org/193125/diff/1/3#newcode721
Line 721: __ mov(scratch3, Factory::empty_fixed_array());
On 2009/09/16 10:46:13, Mads Ager wrote:
> No need for using the scratch register.

Done.

http://codereview.chromium.org/193125/diff/1/3#newcode738
Line 738: __ mov(scratch3, Factory::fixed_array_map());
On 2009/09/16 10:46:13, Mads Ager wrote:
> Remove?

Done.

http://codereview.chromium.org/193125/diff/1/3#newcode743
Line 743: __ mov(scratch3, Factory::the_hole_value());
On 2009/09/16 10:46:13, Mads Ager wrote:
> Push this into only the unfolded loop to avoid the extra reloc info?  Use it
> directly in the loop that is not unfolded (since there will be only one
> occurrence)?

Done.

http://codereview.chromium.org/193125/diff/1/3#newcode795
Line 795: FixedArray::kHeaderSize +
On 2009/09/16 10:46:13, Mads Ager wrote:
> FixedArray::SizeFor(kPreallocatedArrayElements)?

Done.

http://codereview.chromium.org/193125/diff/1/3#newcode847
Line 847: __ mov(scratch, Factory::fixed_array_map());
On 2009/09/16 10:46:13, Mads Ager wrote:
> Remove?

Done.

http://codereview.chromium.org/193125/diff/1/3#newcode869
Line 869: __ mov(scratch, Factory::the_hole_value());
On 2009/09/16 10:46:13, Mads Ager wrote:
> Remove.  
> 
> Do you want to unfold this loop as well if it is small?

Can't do that as the size is dynamic and in a register.

http://codereview.chromium.org/193125/diff/1/3#newcode940
Line 940: __ test(Operand(esp, (push_count + 1) * kPointerSize),
Immediate(0xc0000000));
On 2009/09/16 08:03:01, Lasse Reichstein wrote:
> You can combine the test for a non-negative smi into a single test:
>  test(Operand(esp, (push_count + 1) * kPointerSize),
>       Immediate(0x80000001));
>  j(not_zero, &prepeare_generic_code_call);

Done. Used kIntprtSignBit | kSmiTagMask instead of 0x80000001. The 0xc0000000
was also wrong as the value was a smi.

http://codereview.chromium.org/193125/diff/1/3#newcode1041
Line 1041: // REstore argc and constructor before running the generic code.
On 2009/09/16 10:46:13, Mads Ager wrote:
> REstore -> Restore.

Done.

http://codereview.chromium.org/193125/diff/1/3#newcode1062
Line 1062: #ifdef DEBUG
On 2009/09/16 10:46:13, Mads Ager wrote:
> if (FLAG_debug_code) {
> 
> ?

Done. Changed Check to Assert. We have much inconsistency in the code of whether
to use Assert or Check when checking for  FLAG_debug_code.

http://codereview.chromium.org/193125/diff/1/3#newcode1093
Line 1093: #ifdef DEBUG
On 2009/09/16 10:46:13, Mads Ager wrote:
> if (FLAG_debug_code) {
> 
> ?

Done.