Review feedback addressed, final version committed.
http://codereview.chromium.org/7014033/diff/4001/test/mjsunit/external-array.js File test/mjsunit/external-array.js (right): http://codereview.chromium.org/7014033/diff/4001/test/mjsunit/external-array.js#newcode155 test/mjsunit/external-array.js:155: I'll write and submit the test in a separate CL. On 2011/05/13 13:17:24, Lasse Reichstein wrote:
Test 0.49999999999999994, please :).
And both 1.5 and 2.5.
http://codereview.chromium.org/7014033/diff/1025/src/arm/lithium-arm.cc File src/arm/lithium-arm.cc (right): http://codereview.chromium.org/7014033/diff/1025/src/arm/lithium-arm.cc#newcode1756 src/arm/lithium-arm.cc:1756: Representation rep = value->representation(); On 2011/05/16 07:06:35, Kevin Millikin wrote:
Can we rename this to input_rep and assert that instr->representation
is int32? Done. http://codereview.chromium.org/7014033/diff/1025/src/arm/lithium-arm.cc#newcode1759 src/arm/lithium-arm.cc:1759: return DefineAsRegister(new LClampDoubleToUint8(reg, FixedTemp(d1))); On 2011/05/16 07:06:35, Kevin Millikin wrote:
Probably needs a comment that the fixed register is because register
allocator
does not (yet) support allocation of double temporaries. I'd be
puzzled if I
read this code again in a month.
Done. http://codereview.chromium.org/7014033/diff/1025/src/arm/lithium-codegen-arm.cc File src/arm/lithium-codegen-arm.cc (right): http://codereview.chromium.org/7014033/diff/1025/src/arm/lithium-codegen-arm.cc#newcode3356 src/arm/lithium-codegen-arm.cc:3356: __ Usat(value, 8, Operand(value)); On 2011/05/16 07:06:35, Kevin Millikin wrote:
Should this be removed?
Done. http://codereview.chromium.org/7014033/diff/1025/src/arm/lithium-codegen-arm.cc#newcode4052 src/arm/lithium-codegen-arm.cc:4052: __ b(al, &done); On 2011/05/16 07:06:35, Kevin Millikin wrote:
There is __ jmp(&done) for ARM, which just emits __b(al, &done). I
find it
clearer when reading ARM code but I'm not sure if everyone agrees with
that. Done. http://codereview.chromium.org/7014033/diff/1025/src/arm/lithium-codegen-arm.cc#newcode4057 src/arm/lithium-codegen-arm.cc:4057: __ vldr(double_scratch0(), scratch0(), HeapNumber::kValueOffset); On 2011/05/16 07:06:35, Kevin Millikin wrote:
Someone has kindly implemented vldr to take a MemOperand! So you
should be able
to get rid of the sub and the scratch0 register use and write:
__ vldr(double_scratch0(), FieldMemOperand(input_reg, HeapNumber::kValueOffset));
I guess this code was copied from somewhere, so you could change it
there too. Done. http://codereview.chromium.org/7014033/diff/1025/src/hydrogen-instructions.h File src/hydrogen-instructions.h (right): http://codereview.chromium.org/7014033/diff/1025/src/hydrogen-instructions.h#newcode1061 src/hydrogen-instructions.h:1061: SetFlag(kFlexibleRepresentation); On 2011/05/16 07:06:35, Kevin Millikin wrote:
Everywhere else we have kFlexibleRepresentation, we also have set_representation(Representation::Tagged()) to establish a default representation. I don't know that the representation inference
algorithm
depends on it, but maybe it should be here for safety should somebody
later
assume it.
Done. http://codereview.chromium.org/7014033/diff/1025/src/x64/lithium-codegen-x64.cc File src/x64/lithium-codegen-x64.cc (right): http://codereview.chromium.org/7014033/diff/1025/src/x64/lithium-codegen-x64.cc#newcode3188 src/x64/lithium-codegen-x64.cc:3188: { // Clamp the value to [0..255]. On 2011/05/16 07:06:35, Kevin Millikin wrote:
Can we remove this clamping?
Done. http://codereview.chromium.org/7014033/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
