Implement ICs for FastDoubleArray loads and stores

Implement ICs for FastDoubleArray loads and stores

Implemented on ia32, x64, ARM. Stubbed out with UNIMPLEMENTED on MIPS.

BUG=none
TEST=unbox-double-arrays.js

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

[danno, 2011-07-12 08:20:09]

Please review.

[Mads Ager (chromium), 2011-07-12 12:03:26]

http://codereview.chromium.org/7307030/diff/12005/src/apiutils.h
File src/apiutils.h (right):

http://codereview.chromium.org/7307030/diff/12005/src/apiutils.h#newcode31
src/apiutils.h:31: #include "objects.h"
Why do you need an extra include here when the rest of the file has not changed?

http://codereview.chromium.org/7307030/diff/12005/src/arm/stub-cache-arm.cc
File src/arm/stub-cache-arm.cc (right):

http://codereview.chromium.org/7307030/diff/12005/src/arm/stub-cache-arm.cc#n...
src/arm/stub-cache-arm.cc:4196: __ ldr(r3, FieldMemOperand(elements_reg,
FixedArray::kLengthOffset));
In the store stub you have named the scratch registers as well. I think you
should be consistent and either name them here as well or remove the naming in
the store stub.

http://codereview.chromium.org/7307030/diff/12005/src/arm/stub-cache-arm.cc#n...
src/arm/stub-cache-arm.cc:4203: uint32_t upper_32_offset =
FixedArray::kHeaderSize + Register::kSizeInBytes;
This doesn't really matter, but I like sizeof(kHoleNanLower32) which you are
using in the 32-bit version better. :-)

http://codereview.chromium.org/7307030/diff/12005/src/arm/stub-cache-arm.cc#n...
src/arm/stub-cache-arm.cc:4211: // Non Nan. Allocate a new heap number and copy
the double value into it.
NaN

http://codereview.chromium.org/7307030/diff/12005/src/factory.h
File src/factory.h (right):

http://codereview.chromium.org/7307030/diff/12005/src/factory.h#newcode34
src/factory.h:34: #include "isolate.h"
Is isolate.h needed here? There are no other changes to this file?

http://codereview.chromium.org/7307030/diff/12005/src/ia32/lithium-codegen-ia...
File src/ia32/lithium-codegen-ia32.cc (right):

http://codereview.chromium.org/7307030/diff/12005/src/ia32/lithium-codegen-ia...
src/ia32/lithium-codegen-ia32.cc:2793:
ExternalReference::address_of_canonical_non_hole_nan();
Do you only need this change in ia32 lithium codegen and not in any of the
others?

http://codereview.chromium.org/7307030/diff/12005/src/ia32/stub-cache-ia32.cc
File src/ia32/stub-cache-ia32.cc (right):

http://codereview.chromium.org/7307030/diff/12005/src/ia32/stub-cache-ia32.cc...
src/ia32/stub-cache-ia32.cc:3822: __ fld_d(FieldOperand(ecx, eax, times_4,
FixedDoubleArray::kHeaderSize));
This looks dangerous. Can't this lead to an unbalanced FPU stack? You are
loading here and then the allocation can fail and you don't pop the value if it
does. Either you should allocate earlier or you should pop from the FPU stack
(ffree(); fincstp()) before going into the runtime system to allocate the heap
number.

http://codereview.chromium.org/7307030/diff/12005/src/ia32/stub-cache-ia32.cc...
src/ia32/stub-cache-ia32.cc:3914: __ j(above_equal, &miss_force_generic);
Move the identical branch instructions out of the if?

http://codereview.chromium.org/7307030/diff/12005/src/objects.cc
File src/objects.cc (right):

http://codereview.chromium.org/7307030/diff/12005/src/objects.cc#newcode31
src/objects.cc:31: #include "assembler.h"
objects.cc needs the assembler? Is this for some constants? Maybe we should move
those constants to something like v8.h in that case? Seems a bit strange that
the assembler is needed here?

http://codereview.chromium.org/7307030/diff/12005/src/runtime.h
File src/runtime.h (right):

http://codereview.chromium.org/7307030/diff/12005/src/runtime.h#newcode32
src/runtime.h:32: #include "objects.h"
Is this addition needed?

http://codereview.chromium.org/7307030/diff/12005/src/x64/stub-cache-x64.cc
File src/x64/stub-cache-x64.cc (right):

http://codereview.chromium.org/7307030/diff/12005/src/x64/stub-cache-x64.cc#n...
src/x64/stub-cache-x64.cc:3618: __ fld_d(FieldOperand(rcx, kScratchRegister,
times_8,
Same as on ia32 this can lead to unbalanced push/pop on the FPU register stack.

http://codereview.chromium.org/7307030/diff/12005/src/x64/stub-cache-x64.cc#n...
src/x64/stub-cache-x64.cc:3712: __ j(above_equal, &miss_force_generic);
Move the branch out of the conditional?

http://codereview.chromium.org/7307030/diff/12005/test/mjsunit/unbox-double-a...
File test/mjsunit/unbox-double-arrays.js (right):

http://codereview.chromium.org/7307030/diff/12005/test/mjsunit/unbox-double-a...
test/mjsunit/unbox-double-arrays.js:81: // (premonomorphic and monomorphic) of
KeyedLoad access works in various cases.
Long line. :-)

http://codereview.chromium.org/7307030/diff/12005/test/mjsunit/unbox-double-a...
test/mjsunit/unbox-double-arrays.js:117: test_various_stores(large_array,
Infinity, -Infinity, expected_array_value(7));
Long line.

[Rodolph Perfetta, 2011-07-12 14:00:30]

drive by comments.

http://codereview.chromium.org/7307030/diff/12005/src/arm/stub-cache-arm.cc
File src/arm/stub-cache-arm.cc (right):

http://codereview.chromium.org/7307030/diff/12005/src/arm/stub-cache-arm.cc#n...
src/arm/stub-cache-arm.cc:4208: __ add(r5, r4, Operand(1), SetCC);
0x7FFFFFFF + 1 will not set the overflow flag, but the negative flag. Some
negative values are valid for the upper part of a double so maybe a cmp could be
used instead. The V8 assembler will replace cmp with 0x7FFFFFFF by cmn with
0x80000001 and 0x80000001 is a valid ARM immediate (no need for extra
instructions).

[danno, 2011-07-13 08:59:52]

Thanks for the helpful comments. Should be much better now, please take another
look.

http://codereview.chromium.org/7307030/diff/12005/src/apiutils.h
File src/apiutils.h (right):

http://codereview.chromium.org/7307030/diff/12005/src/apiutils.h#newcode31
src/apiutils.h:31: #include "objects.h"
On 2011/07/12 12:03:26, Mads Ager wrote:
> Why do you need an extra include here when the rest of the file has not
changed?

Done.

http://codereview.chromium.org/7307030/diff/12005/src/arm/stub-cache-arm.cc
File src/arm/stub-cache-arm.cc (right):

http://codereview.chromium.org/7307030/diff/12005/src/arm/stub-cache-arm.cc#n...
src/arm/stub-cache-arm.cc:4196: __ ldr(r3, FieldMemOperand(elements_reg,
FixedArray::kLengthOffset));
On 2011/07/12 12:03:26, Mads Ager wrote:
> In the store stub you have named the scratch registers as well. I think you
> should be consistent and either name them here as well or remove the naming in
> the store stub.

Done.

http://codereview.chromium.org/7307030/diff/12005/src/arm/stub-cache-arm.cc#n...
src/arm/stub-cache-arm.cc:4203: uint32_t upper_32_offset =
FixedArray::kHeaderSize + Register::kSizeInBytes;
On 2011/07/12 12:03:26, Mads Ager wrote:
> This doesn't really matter, but I like sizeof(kHoleNanLower32) which you are
> using in the 32-bit version better. :-)

Done.

http://codereview.chromium.org/7307030/diff/12005/src/arm/stub-cache-arm.cc#n...
src/arm/stub-cache-arm.cc:4208: __ add(r5, r4, Operand(1), SetCC);
On 2011/07/12 14:00:30, Rodolph Perfetta wrote:
> 0x7FFFFFFF + 1 will not set the overflow flag, but the negative flag. Some
> negative values are valid for the upper part of a double so maybe a cmp could
be
> used instead. The V8 assembler will replace cmp with 0x7FFFFFFF by cmn with
> 0x80000001 and 0x80000001 is a valid ARM immediate (no need for extra
> instructions).

Done.

http://codereview.chromium.org/7307030/diff/12005/src/arm/stub-cache-arm.cc#n...
src/arm/stub-cache-arm.cc:4211: // Non Nan. Allocate a new heap number and copy
the double value into it.
On 2011/07/12 12:03:26, Mads Ager wrote:
> NaN

Done.

http://codereview.chromium.org/7307030/diff/12005/src/factory.h
File src/factory.h (right):

http://codereview.chromium.org/7307030/diff/12005/src/factory.h#newcode34
src/factory.h:34: #include "isolate.h"
On 2011/07/12 12:03:26, Mads Ager wrote:
> Is isolate.h needed here? There are no other changes to this file?

Done.

http://codereview.chromium.org/7307030/diff/12005/src/ia32/lithium-codegen-ia...
File src/ia32/lithium-codegen-ia32.cc (right):

http://codereview.chromium.org/7307030/diff/12005/src/ia32/lithium-codegen-ia...
src/ia32/lithium-codegen-ia32.cc:2793:
ExternalReference::address_of_canonical_non_hole_nan();
On 2011/07/12 12:03:26, Mads Ager wrote:
> Do you only need this change in ia32 lithium codegen and not in any of the
> others?

No, only the ia32 implementation uses this constant from assembler.cc in this
way. I was surprised, too.

http://codereview.chromium.org/7307030/diff/12005/src/ia32/stub-cache-ia32.cc
File src/ia32/stub-cache-ia32.cc (right):

http://codereview.chromium.org/7307030/diff/12005/src/ia32/stub-cache-ia32.cc...
src/ia32/stub-cache-ia32.cc:3822: __ fld_d(FieldOperand(ecx, eax, times_4,
FixedDoubleArray::kHeaderSize));
On 2011/07/12 12:03:26, Mads Ager wrote:
> This looks dangerous. Can't this lead to an unbalanced FPU stack? You are
> loading here and then the allocation can fail and you don't pop the value if
it
> does. Either you should allocate earlier or you should pop from the FPU stack
> (ffree(); fincstp()) before going into the runtime system to allocate the heap
> number.

Done.

http://codereview.chromium.org/7307030/diff/12005/src/ia32/stub-cache-ia32.cc...
src/ia32/stub-cache-ia32.cc:3914: __ j(above_equal, &miss_force_generic);
On 2011/07/12 12:03:26, Mads Ager wrote:
> Move the identical branch instructions out of the if?

Done.

http://codereview.chromium.org/7307030/diff/12005/src/objects.cc
File src/objects.cc (right):

http://codereview.chromium.org/7307030/diff/12005/src/objects.cc#newcode31
src/objects.cc:31: #include "assembler.h"
On 2011/07/12 12:03:26, Mads Ager wrote:
> objects.cc needs the assembler? Is this for some constants? Maybe we should
move
> those constants to something like v8.h in that case? Seems a bit strange that
> the assembler is needed here?

Done.

http://codereview.chromium.org/7307030/diff/12005/src/runtime.h
File src/runtime.h (right):

http://codereview.chromium.org/7307030/diff/12005/src/runtime.h#newcode32
src/runtime.h:32: #include "objects.h"
On 2011/07/12 12:03:26, Mads Ager wrote:
> Is this addition needed?

Done.

http://codereview.chromium.org/7307030/diff/12005/src/x64/stub-cache-x64.cc
File src/x64/stub-cache-x64.cc (right):

http://codereview.chromium.org/7307030/diff/12005/src/x64/stub-cache-x64.cc#n...
src/x64/stub-cache-x64.cc:3618: __ fld_d(FieldOperand(rcx, kScratchRegister,
times_8,
On 2011/07/12 12:03:26, Mads Ager wrote:
> Same as on ia32 this can lead to unbalanced push/pop on the FPU register
stack.

Done.

http://codereview.chromium.org/7307030/diff/12005/src/x64/stub-cache-x64.cc#n...
src/x64/stub-cache-x64.cc:3712: __ j(above_equal, &miss_force_generic);
On 2011/07/12 12:03:26, Mads Ager wrote:
> Move the branch out of the conditional?

Done.

http://codereview.chromium.org/7307030/diff/12005/test/mjsunit/unbox-double-a...
File test/mjsunit/unbox-double-arrays.js (right):

http://codereview.chromium.org/7307030/diff/12005/test/mjsunit/unbox-double-a...
test/mjsunit/unbox-double-arrays.js:81: // (premonomorphic and monomorphic) of
KeyedLoad access works in various cases.
On 2011/07/12 12:03:26, Mads Ager wrote:
> Long line. :-)

Done.

http://codereview.chromium.org/7307030/diff/12005/test/mjsunit/unbox-double-a...
test/mjsunit/unbox-double-arrays.js:117: test_various_stores(large_array,
Infinity, -Infinity, expected_array_value(7));
On 2011/07/12 12:03:26, Mads Ager wrote:
> Long line. 

Done.

[Mads Ager (chromium), 2011-07-13 09:43:09]

LGTM with a couple of nits.

http://codereview.chromium.org/7307030/diff/21002/src/arm/stub-cache-arm.cc
File src/arm/stub-cache-arm.cc (right):

http://codereview.chromium.org/7307030/diff/21002/src/arm/stub-cache-arm.cc#n...
src/arm/stub-cache-arm.cc:4210: __ cmp(scratch, Operand(0x7FFFFFFF));
Can you use kHoleNaNUpper32 here?

http://codereview.chromium.org/7307030/diff/21002/src/arm/stub-cache-arm.cc#n...
src/arm/stub-cache-arm.cc:4360: __ ldr(scratch3, FieldMemOperand(value_reg,
HeapNumber::kExponentOffset));
Are scratch2 and scratch3 used for anything but exponent and mantissa? If not,
those would be better names for them.

http://codereview.chromium.org/7307030/diff/21002/src/assembler.cc
File src/assembler.cc (right):

http://codereview.chromium.org/7307030/diff/21002/src/assembler.cc#newcode50
src/assembler.cc:50: #include "v8.h"
This is already included above.

http://codereview.chromium.org/7307030/diff/21002/src/ia32/stub-cache-ia32.cc
File src/ia32/stub-cache-ia32.cc (right):

http://codereview.chromium.org/7307030/diff/21002/src/ia32/stub-cache-ia32.cc...
src/ia32/stub-cache-ia32.cc:3822: __ fld_d(FieldOperand(ecx, eax, times_4,
FixedDoubleArray::kHeaderSize));
Would it be worth it to check for SSE2 here as well as in the stores and use xmm
registers? We should probably see if it makes a difference.

http://codereview.chromium.org/7307030/diff/21002/src/ia32/stub-cache-ia32.cc...
src/ia32/stub-cache-ia32.cc:3830: __ ffree();
Maybe add a short comment like: Pop double from FPU stack and enter the runtime
system to allocate result.

http://codereview.chromium.org/7307030/diff/21002/src/ia32/stub-cache-ia32.cc...
src/ia32/stub-cache-ia32.cc:3931: __ cmp(FieldOperand(eax, offset),
Immediate(0x7ff00000));
You had a comment about this constant in the ARM code. We should probably name
this constant as well.

[danno, 2011-07-13 14:11:03]

feedback addressed and committed.

http://codereview.chromium.org/7307030/diff/21002/src/arm/stub-cache-arm.cc
File src/arm/stub-cache-arm.cc (right):

http://codereview.chromium.org/7307030/diff/21002/src/arm/stub-cache-arm.cc#n...
src/arm/stub-cache-arm.cc:4210: __ cmp(scratch, Operand(0x7FFFFFFF));
On 2011/07/13 09:43:09, Mads Ager wrote:
> Can you use kHoleNaNUpper32 here?

Done.

http://codereview.chromium.org/7307030/diff/21002/src/arm/stub-cache-arm.cc#n...
src/arm/stub-cache-arm.cc:4360: __ ldr(scratch3, FieldMemOperand(value_reg,
HeapNumber::kExponentOffset));
On 2011/07/13 09:43:09, Mads Ager wrote:
> Are scratch2 and scratch3 used for anything but exponent and mantissa? If not,
> those would be better names for them.

Done.

http://codereview.chromium.org/7307030/diff/21002/src/assembler.cc
File src/assembler.cc (right):

http://codereview.chromium.org/7307030/diff/21002/src/assembler.cc#newcode50
src/assembler.cc:50: #include "v8.h"
On 2011/07/13 09:43:09, Mads Ager wrote:
> This is already included above.

Done.

http://codereview.chromium.org/7307030/diff/21002/src/ia32/stub-cache-ia32.cc
File src/ia32/stub-cache-ia32.cc (right):

http://codereview.chromium.org/7307030/diff/21002/src/ia32/stub-cache-ia32.cc...
src/ia32/stub-cache-ia32.cc:3822: __ fld_d(FieldOperand(ecx, eax, times_4,
FixedDoubleArray::kHeaderSize));
On 2011/07/13 09:43:09, Mads Ager wrote:
> Would it be worth it to check for SSE2 here as well as in the stores and use
xmm
> registers? We should probably see if it makes a difference.

Done.

http://codereview.chromium.org/7307030/diff/21002/src/ia32/stub-cache-ia32.cc...
src/ia32/stub-cache-ia32.cc:3830: __ ffree();
On 2011/07/13 09:43:09, Mads Ager wrote:
> Maybe add a short comment like: Pop double from FPU stack and enter the
runtime
> system to allocate result.

Done.

http://codereview.chromium.org/7307030/diff/21002/src/ia32/stub-cache-ia32.cc...
src/ia32/stub-cache-ia32.cc:3931: __ cmp(FieldOperand(eax, offset),
Immediate(0x7ff00000));
On 2011/07/13 09:43:09, Mads Ager wrote:
> You had a comment about this constant in the ARM code. We should probably name
> this constant as well.

Done.