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.cc#newcode2442 src/arm/lithium-codegen-arm.cc:2442: Register scratch1 = scratch0(); scratch1 -> scratch? 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)) Use "operand = expression" instead of "operand(expression)" here? 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. 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.cc#newcode2469 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.cc#newcode3304 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.cc#newcode3306 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.cc#newcode3308 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#newcode3754 src/hydrogen-instructions.h:3754: ? Representation::Tagged() I would avoid the nested ?: here. 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())); I would use = as for elements. 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( 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-ia32.cc#newcode2241 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-ia32.cc#newcode3111 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.cc#newcode3102 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-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() { Do you need both flush_compiled_code and multiple copies of the test function? http://codereview.chromium.org/7350021/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
