Check the inlined code for loads.
http://codereview.chromium.org/160272/diff/1/3 File src/x64/codegen-x64.cc (right): http://codereview.chromium.org/160272/diff/1/3#newcode5466 Line 5466: masm->movq(kScratchRegister, Factory::null_value(), RelocInfo::EMBEDDED_OBJECT); Why are we testing the receiver against a particular map here? Doesn't the following code work for any JSObject with fast elements, and the index in range? (Maybe this is to exclude cases with an interceptor, or accessors?) Why is this even much faster than the generic stub? http://codereview.chromium.org/160272/diff/1/3#newcode5476 Line 5476: // is not a dictionary. Change the comment (and the code) to ensure that the elements array is a FixedArray, now that we have PixelArrays. Check equal to FixedArray, not not equal to HashTable. http://codereview.chromium.org/160272/diff/1/3#newcode5487 Line 5487: __ movsxlq(index.reg(), key.reg()); Why not movl? http://codereview.chromium.org/160272/diff/1/3#newcode5492 Line 5492: Comment here that only positive indices in range reach here, they are zero-extended, and so the full 64-bit index_reg can be used as an index in an operand. http://codereview.chromium.org/160272/diff/1/3#newcode5508 Line 5508: __ movq(kScratchRegister, Factory::the_hole_value(), This two-instruction sequence is MacroAssembler::Cmp(Reg, Handle) http://codereview.chromium.org/160272/diff/1/3#newcode5632 Line 5632: // Check that the value is a smi if it is not a constant. We Move "We" to the next line? http://codereview.chromium.org/160272/diff/1/3#newcode5639 Line 5639: // Check that the key is a non-negative smi. Why not combine this test with the test against the array limit, as in the load code? cmpl(key.reg(), FieldOperand..), then Branch(above_equal). http://codereview.chromium.org/160272/diff/1/3#newcode5648 Line 5648: __ movq(kScratchRegister, This is MacroAssembler::CmpObjectType(receiver.reg(), JS_ARRAY_TYPE, kScratchRegister) http://codereview.chromium.org/160272/diff/1/3#newcode5662 Line 5662: // is not a dictionary. is a FixedArray. http://codereview.chromium.org/160272/diff/1/3#newcode5671 Line 5671: __ movq(kScratchRegister, Factory::fixed_array_map(), masm->bind( masm->movq( To avoid extra code. http://codereview.chromium.org/160272/diff/1/3#newcode5677 Line 5677: // Store the value. I suppose you should assert that SmiTag == 0 here. We do it most other places where it matters. Do we have a static assert (compile-time assert)? http://codereview.chromium.org/160272/diff/1/4 File src/x64/ic-x64.cc (right): http://codereview.chromium.org/160272/diff/1/4#newcode166 Line 166: static bool PatchInlinedMapCheck(Address address, Object* map) { Comment with argument meanings, preconditions, and effect. Unless this is in the header file. http://codereview.chromium.org/160272/diff/1/4#newcode167 Line 167: Address test_instruction_address = address + 4; // 4 = stub address 4 = stub address makes no sense to me. http://codereview.chromium.org/160272/diff/1/4#newcode640 Line 640: if (*test_instruction_address != kTestEaxByte) return false; I thought this test was abstracted to a function call - HasInlineCode, or something like that. http://codereview.chromium.org/160272/diff/1/4#newcode647 Line 647: // operand-immediate compare instruction, so we add 3 to get the This is definitely wrong. The map address is in a 64-bit immediate load to the scratch register. http://codereview.chromium.org/160272/diff/1/4#newcode654 Line 654: // offset to the last 4 bytes. Check that this is right. http://codereview.chromium.org/160272/diff/1/5 File src/x64/macro-assembler-x64.cc (right): http://codereview.chromium.org/160272/diff/1/5#newcode327 Line 327: push(Immediate(smi)); This will be sign-extended, not zero-extended, but that should be safe. http://codereview.chromium.org/160272 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
