LGTM with a couple of comments.
http://codereview.chromium.org/660184/diff/1/2 File src/arm/codegen-arm.cc (right): http://codereview.chromium.org/660184/diff/1/2#newcode3439 src/arm/codegen-arm.cc:3439: __ add(r1, r1, Operand(r0)); I would prefer to use __ add(r1, r1, Operand(r0, LSL, kPointerSizeLog2 - kSmiTagSize)) instead of these two instructions. Also makes it more clear to me that you are adding ((untagged r0) * kPointerSize) to r1. http://codereview.chromium.org/660184/diff/1/2#newcode3442 src/arm/codegen-arm.cc:3442: __ cmp(r1, Operand(Factory::undefined_value())); You should use LoadRoot to ip followed by cmp here to use the RootArray instead of embedding the object in the code. http://codereview.chromium.org/660184/diff/1/6 File src/ia32/codegen-ia32.cc (right): http://codereview.chromium.org/660184/diff/1/6#newcode5605 src/ia32/codegen-ia32.cc:5605: frame_->Dup(); Do you need the Dup here? You never modify code as far as I can tell, so I guess you could just Pop it from the frame and push it back if you need it on the frame. Then you can just push the result instead of overwriting element zero on the frame with the result. http://codereview.chromium.org/660184/diff/1/6#newcode5625 src/ia32/codegen-ia32.cc:5625: __ Set(temp.reg(), Immediate(Factory::single_character_string_cache())); Adding a comment here stating that code contains a smi tagged ascii char code might be good to explain the times_hafl_pointer_size in the Operand below. http://codereview.chromium.org/660184/diff/1/6#newcode5629 src/ia32/codegen-ia32.cc:5629: __ mov(temp.reg(), Operand(temp.reg(), Use FieldOperand and drop the "- kHeapObjectTag" on the offset? http://codereview.chromium.org/660184/diff/1/12 File src/x64/codegen-x64.cc (right): http://codereview.chromium.org/660184/diff/1/12#newcode3906 src/x64/codegen-x64.cc:3906: __ movq(temp.reg(), Operand(temp.reg(), FieldOperand to get rid of the "- kHeapObjectTag" in the offset. http://codereview.chromium.org/660184/diff/1/12#newcode3909 src/x64/codegen-x64.cc:3909: __ Cmp(temp.reg(), Factory::undefined_value()); You should use CompareRoot here to use the root array. http://codereview.chromium.org/660184 -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
