Crankshaft support for FixedDoubleArrays

Crankshaft support for FixedDoubleArrays

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

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

[danno, 2011-07-18 14:29:40]

please review.

[Alexandre, 2011-07-18 16:22:14]

Three possible minor possible optimizations on ARM.

Alexandre

http://codereview.chromium.org/7350021/diff/16001/src/arm/lithium-codegen-arm.cc
File src/arm/lithium-codegen-arm.cc (right):

http://codereview.chromium.org/7350021/diff/16001/src/arm/lithium-codegen-arm...
src/arm/lithium-codegen-arm.cc:2458: __ add(elements, elements, operand);
If key is constant this add instruction can be merged with the one below.

http://codereview.chromium.org/7350021/diff/16001/src/arm/lithium-codegen-arm...
src/arm/lithium-codegen-arm.cc:3306: __ add(elements, elements, operand);
If the key is constant, these two add instructions can be merged into one.

http://codereview.chromium.org/7350021/diff/16001/src/arm/lithium-codegen-arm...
src/arm/lithium-codegen-arm.cc:3314: __ Vmov(value,
FixedDoubleArray::canonical_not_the_hole_nan_as_double());
You can spare the previous branch by making this vmov conditional.

[Mads Ager (chromium), 2011-07-19 08:03:51]

LGTM with a number of nits.

The nits for ia32 applies to x64 as well (and the other way around). :-)

http://codereview.chromium.org/7350021/diff/16001/src/arm/lithium-codegen-arm.cc
File src/arm/lithium-codegen-arm.cc (right):

http://codereview.chromium.org/7350021/diff/16001/src/arm/lithium-codegen-arm...
src/arm/lithium-codegen-arm.cc:2442: Register scratch1 = scratch0();
scratch1 -> scratch?

http://codereview.chromium.org/7350021/diff/16001/src/arm/lithium-codegen-arm...
src/arm/lithium-codegen-arm.cc:2456: Operand operand(key_is_constant ?
Operand(constant_key * (1 << shift_size))
Use "operand = expression" instead of "operand(expression)" here?

http://codereview.chromium.org/7350021/diff/16001/src/arm/lithium-codegen-arm...
src/arm/lithium-codegen-arm.cc:2458: __ add(elements, elements, operand);
On 2011/07/18 16:22:14, Alexandre wrote:
> If key is constant this add instruction can be merged with the one below.

I'm fine with tweaking stuff like this later. I think if the instruction does
not require hole checks and the key is a constant, the element register does not
need to be writable and we can use UseRegisterAtStart. Starting with the current
structure is fine to keep it simple. Then we can measure the impact of these
tweaks once this lands.

http://codereview.chromium.org/7350021/diff/16001/src/arm/lithium-codegen-arm...
src/arm/lithium-codegen-arm.cc:2469: Operand(FixedDoubleArray::kHeaderSize -
kSmiTagMask));
kSmiTagMask -> kHeapObjectTag?

http://codereview.chromium.org/7350021/diff/16001/src/arm/lithium-codegen-arm...
src/arm/lithium-codegen-arm.cc:3304: Operand operand(key_is_constant ?
Operand(constant_key * (1 << shift_size))
I would use '=' here.

http://codereview.chromium.org/7350021/diff/16001/src/arm/lithium-codegen-arm...
src/arm/lithium-codegen-arm.cc:3306: __ add(elements, elements, operand);
Instead of writing to elements here, can you use the scratch register? That way
i think you can avoid writing to elements and you can get away with using
UseRegisterAtStart instead of UseTempRegister?

http://codereview.chromium.org/7350021/diff/16001/src/arm/lithium-codegen-arm...
src/arm/lithium-codegen-arm.cc:3308: Operand(FixedDoubleArray::kHeaderSize -
kSmiTagMask));
kHeapObjectTag

http://codereview.chromium.org/7350021/diff/16001/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

http://codereview.chromium.org/7350021/diff/16001/src/hydrogen-instructions.h...
src/hydrogen-instructions.h:3754: ? Representation::Tagged()
I would avoid the nested ?: here.

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

http://codereview.chromium.org/7350021/diff/16001/src/ia32/lithium-codegen-ia...
src/ia32/lithium-codegen-ia32.cc:2236: XMMRegister
result(ToDoubleRegister(instr->result()));
I would use = as for elements.

http://codereview.chromium.org/7350021/diff/16001/src/ia32/lithium-codegen-ia...
src/ia32/lithium-codegen-ia32.cc:2239: Operand
hole_check_operand(BuildFastArrayOperand(
For these operand creations I would use = instead of constructor syntax. Here
and multiple places below.

http://codereview.chromium.org/7350021/diff/16001/src/ia32/lithium-codegen-ia...
src/ia32/lithium-codegen-ia32.cc:2241: FixedDoubleArray::kHeaderSize -
kSmiTagMask + sizeof(kHoleNanLower32)));
kHeapObjectTag instead of kSmiTagMask?

http://codereview.chromium.org/7350021/diff/16001/src/ia32/lithium-codegen-ia...
src/ia32/lithium-codegen-ia32.cc:3111: FixedDoubleArray::kHeaderSize -
kSmiTagMask));
kHeapObjectTag

http://codereview.chromium.org/7350021/diff/16001/src/x64/lithium-codegen-x64.cc
File src/x64/lithium-codegen-x64.cc (right):

http://codereview.chromium.org/7350021/diff/16001/src/x64/lithium-codegen-x64...
src/x64/lithium-codegen-x64.cc:3102: Register key = instr->key()->IsRegister() ?
ToRegister(instr->key()) : no_reg;
I don't think you ever use this one?

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

http://codereview.chromium.org/7350021/diff/16001/test/mjsunit/unbox-double-a...
test/mjsunit/unbox-double-arrays.js:51: function flush_compiled_code() {
Do you need both flush_compiled_code and multiple copies of the test function?

[danno, 2011-07-19 12:50:16]

addressed feedback, landing...

http://codereview.chromium.org/7350021/diff/16001/src/arm/lithium-codegen-arm.cc
File src/arm/lithium-codegen-arm.cc (right):

http://codereview.chromium.org/7350021/diff/16001/src/arm/lithium-codegen-arm...
src/arm/lithium-codegen-arm.cc:2442: Register scratch1 = scratch0();
On 2011/07/19 08:03:51, Mads Ager wrote:
> scratch1 -> scratch?

Done.

http://codereview.chromium.org/7350021/diff/16001/src/arm/lithium-codegen-arm...
src/arm/lithium-codegen-arm.cc:2456: Operand operand(key_is_constant ?
Operand(constant_key * (1 << shift_size))
On 2011/07/19 08:03:51, Mads Ager wrote:
> Use "operand = expression" instead of "operand(expression)" here?

Done.

http://codereview.chromium.org/7350021/diff/16001/src/arm/lithium-codegen-arm...
src/arm/lithium-codegen-arm.cc:2458: __ add(elements, elements, operand);
On 2011/07/18 16:22:14, Alexandre wrote:
> If key is constant this add instruction can be merged with the one below.

Done.

http://codereview.chromium.org/7350021/diff/16001/src/arm/lithium-codegen-arm...
src/arm/lithium-codegen-arm.cc:2469: Operand(FixedDoubleArray::kHeaderSize -
kSmiTagMask));
On 2011/07/19 08:03:51, Mads Ager wrote:
> kSmiTagMask -> kHeapObjectTag?

Done.

http://codereview.chromium.org/7350021/diff/16001/src/arm/lithium-codegen-arm...
src/arm/lithium-codegen-arm.cc:3304: Operand operand(key_is_constant ?
Operand(constant_key * (1 << shift_size))
On 2011/07/19 08:03:51, Mads Ager wrote:
> I would use '=' here.

Done.

http://codereview.chromium.org/7350021/diff/16001/src/arm/lithium-codegen-arm...
src/arm/lithium-codegen-arm.cc:3306: __ add(elements, elements, operand);
On 2011/07/18 16:22:14, Alexandre wrote:
> If the key is constant, these two add instructions can be merged into one.

Done.

http://codereview.chromium.org/7350021/diff/16001/src/arm/lithium-codegen-arm...
src/arm/lithium-codegen-arm.cc:3306: __ add(elements, elements, operand);
On 2011/07/19 08:03:51, Mads Ager wrote:
> Instead of writing to elements here, can you use the scratch register? That
way
> i think you can avoid writing to elements and you can get away with using
> UseRegisterAtStart instead of UseTempRegister?

Done.

http://codereview.chromium.org/7350021/diff/16001/src/arm/lithium-codegen-arm...
src/arm/lithium-codegen-arm.cc:3308: Operand(FixedDoubleArray::kHeaderSize -
kSmiTagMask));
On 2011/07/19 08:03:51, Mads Ager wrote:
> kHeapObjectTag

Done.

http://codereview.chromium.org/7350021/diff/16001/src/arm/lithium-codegen-arm...
src/arm/lithium-codegen-arm.cc:3314: __ Vmov(value,
FixedDoubleArray::canonical_not_the_hole_nan_as_double());
On 2011/07/18 16:22:14, Alexandre wrote:
> You can spare the previous branch by making this vmov conditional.

Done.

http://codereview.chromium.org/7350021/diff/16001/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

http://codereview.chromium.org/7350021/diff/16001/src/hydrogen-instructions.h...
src/hydrogen-instructions.h:3754: ? Representation::Tagged()
On 2011/07/19 08:03:51, Mads Ager wrote:
> I would avoid the nested ?: here.

Done.

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

http://codereview.chromium.org/7350021/diff/16001/src/ia32/lithium-codegen-ia...
src/ia32/lithium-codegen-ia32.cc:2236: XMMRegister
result(ToDoubleRegister(instr->result()));
On 2011/07/19 08:03:51, Mads Ager wrote:
> I would use = as for elements.

Done.

http://codereview.chromium.org/7350021/diff/16001/src/ia32/lithium-codegen-ia...
src/ia32/lithium-codegen-ia32.cc:2239: Operand
hole_check_operand(BuildFastArrayOperand(
On 2011/07/19 08:03:51, Mads Ager wrote:
> For these operand creations I would use = instead of constructor syntax. Here
> and multiple places below.

Done.

http://codereview.chromium.org/7350021/diff/16001/src/ia32/lithium-codegen-ia...
src/ia32/lithium-codegen-ia32.cc:2241: FixedDoubleArray::kHeaderSize -
kSmiTagMask + sizeof(kHoleNanLower32)));
On 2011/07/19 08:03:51, Mads Ager wrote:
> kHeapObjectTag instead of kSmiTagMask?

Done.

http://codereview.chromium.org/7350021/diff/16001/src/ia32/lithium-codegen-ia...
src/ia32/lithium-codegen-ia32.cc:3111: FixedDoubleArray::kHeaderSize -
kSmiTagMask));
On 2011/07/19 08:03:51, Mads Ager wrote:
> kHeapObjectTag

Done.

http://codereview.chromium.org/7350021/diff/16001/src/x64/lithium-codegen-x64.cc
File src/x64/lithium-codegen-x64.cc (right):

http://codereview.chromium.org/7350021/diff/16001/src/x64/lithium-codegen-x64...
src/x64/lithium-codegen-x64.cc:3102: Register key = instr->key()->IsRegister() ?
ToRegister(instr->key()) : no_reg;
On 2011/07/19 08:03:51, Mads Ager wrote:
> I don't think you ever use this one?

Done.

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

http://codereview.chromium.org/7350021/diff/16001/test/mjsunit/unbox-double-a...
test/mjsunit/unbox-double-arrays.js:51: function flush_compiled_code() {
On 2011/07/19 08:03:51, Mads Ager wrote:
> Do you need both flush_compiled_code and multiple copies of the test function?

Done.