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.cc#newcode2442 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.cc#newcode2456 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.cc#newcode2458 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.cc#newcode2469 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.cc#newcode3304 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.cc#newcode3306 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.cc#newcode3306 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.cc#newcode3308 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.cc#newcode3314 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#newcode3754 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-ia32.cc File src/ia32/lithium-codegen-ia32.cc (right): http://codereview.chromium.org/7350021/diff/16001/src/ia32/lithium-codegen-ia32.cc#newcode2236 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-ia32.cc#newcode2239 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-ia32.cc#newcode2241 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-ia32.cc#newcode3111 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.cc#newcode3102 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-arrays.js File test/mjsunit/unbox-double-arrays.js (right): http://codereview.chromium.org/7350021/diff/16001/test/mjsunit/unbox-double-arrays.js#newcode51 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. http://codereview.chromium.org/7350021/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
