Good comments.
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); I'm guessing it is mainly a matter of avoiding interceptors (and because the most traditional use of numeric indices are plain JS arrays which all have the same map). Adding named properties to a JSObject with fast elements shouldn't affect whether this works, so checking the elements FixedArray's map (to see if it's a plain FixedArray and not a Dictionary) might be enough to test for fast case, but it won't know if there are interceptors. http://codereview.chromium.org/160272/diff/1/3#newcode5476 Line 5476: // is not a dictionary. Not really necessary. The elements array of an object can only ever be a FixedArray (fast elements) or a HashTable (slow elements). However, it seems safer and more to test for equality to fixed-array anyway. http://codereview.chromium.org/160272/diff/1/3#newcode5487 Line 5487: __ movsxlq(index.reg(), key.reg()); It's used as an index below (as you comment), so we need the correct value in the entire 64-bit register. However, if it's always positive (as suggested by the unsigned comparison below), there is no need for the sign extension (or arithmetic shift). I'll make the smi-test above test only allow non-negative smis and avoid the sign extension. http://codereview.chromium.org/160272/diff/1/3#newcode5492 Line 5492: Done. http://codereview.chromium.org/160272/diff/1/3#newcode5508 Line 5508: __ movq(kScratchRegister, Factory::the_hole_value(), Fixed http://codereview.chromium.org/160272/diff/1/3#newcode5639 Line 5639: // Check that the key is a non-negative smi. In the load code we also have a separate smi-check. We need to do that before doing smi computations anyway. http://codereview.chromium.org/160272/diff/1/3#newcode5648 Line 5648: __ movq(kScratchRegister, Fixed. http://codereview.chromium.org/160272/diff/1/3#newcode5662 Line 5662: // is not a dictionary. Done. http://codereview.chromium.org/160272/diff/1/3#newcode5671 Line 5671: __ movq(kScratchRegister, Factory::fixed_array_map(), Done. Not necessary on __ bind, since coverage code needs to go before the actual code (by the way the macro is constructed, it must end in "masm->"). http://codereview.chromium.org/160272/diff/1/3#newcode5677 Line 5677: // Store the value. Done. 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) { Added in header. http://codereview.chromium.org/160272/diff/1/4#newcode167 Line 167: Address test_instruction_address = address + 4; // 4 = stub address Nor me. It turned out to be the value of kTargetAddrToReturnAddrDist in ia32. Changed to that in both x64 and ia32. http://codereview.chromium.org/160272/diff/1/4#newcode640 Line 640: if (*test_instruction_address != kTestEaxByte) return false; It only appears twice, and in technically unrelated ICs (i.e., one could change its implementation without the other doing so), so I guess it wasn't found worthy of abstraction. http://codereview.chromium.org/160272/diff/1/4#newcode647 Line 647: // operand-immediate compare instruction, so we add 3 to get the It is indeed wrong. Fixed. http://codereview.chromium.org/160272/diff/1/4#newcode654 Line 654: // offset to the last 4 bytes. It's wrong too (there's a REX prefix now, so the constant is "3"). http://codereview.chromium.org/160272 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
