I have some naming suggestions, and beefed up comments/asserts.
Suggestions in
the ARM files apply sometimes to all platforms.
LGTM with comments adressed.
http://codereview.chromium.org/7014033/diff/4001/src/arm/lithium-arm.h
File src/arm/lithium-arm.h (right):
http://codereview.chromium.org/7014033/diff/4001/src/arm/lithium-arm.h#newcode77
src/arm/lithium-arm.h:77: V(ClampIToUint8)
\
It's strange that there is "Double", "I", and "Tagged" :) I suggest
"D", "I", and "T" so the class names match the printed instruction
names.
http://codereview.chromium.org/7014033/diff/4001/src/arm/lithium-arm.h#newcode1935
src/arm/lithium-arm.h:1935: explicit LClampDoubleToUint8(LOperand*
value, LOperand* temp) {
No need for explicit here.
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();
Can we rename this to input_rep and assert that instr->representation is
int32?
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)));
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.
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));
Should this be removed?
http://codereview.chromium.org/7014033/diff/1025/src/arm/lithium-codegen-arm.cc#newcode4052
src/arm/lithium-codegen-arm.cc:4052: __ b(al, &done);
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.
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);
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.
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);
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.
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].
Can we remove this clamping?
http://codereview.chromium.org/7014033/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
