Overall LGTM, but I have comments on the ARM changes.


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

http://codereview.chromium.org/10831049/diff/1/src/arm/lithium-codegen-arm.cc#newcode2793
src/arm/lithium-codegen-arm.cc:2793: if
(instr->hydrogen()->key()->representation().IsTagged()) {
I don't see how this condition can ever be true. HLoadKeyedFastElements
forces Integer32 representation for its |key| input.

http://codereview.chromium.org/10831049/diff/1/src/arm/lithium-codegen-arm.cc#newcode2802
src/arm/lithium-codegen-arm.cc:2802: Operand(instr->additional_index()
<< kPointerSizeLog2));
What's the benefit of having this addition in generated code here?
instr->additional_index() is a constant; adding it to
FixedArray::kHeaderSize at Hydrogen runtime seems fine to me.

http://codereview.chromium.org/10831049/

--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev

Reply via email to