X64: Added inline keyed load/store and a bunch of other missing functions.

X64: Added inline keyed load/store and a bunch of other missing functions.

[Lasse Reichstein, 2009-07-28 14:57:38]

Largish review.

[William Hesse, 2009-07-29 09:01:37]

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.

[Lasse Reichstein, 2009-07-29 11:04:38]

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").

[William Hesse, 2009-07-29 12:55:34]

LGTM.

[Lasse Reichstein, 2009-07-30 08:10:07]

Had to adapt to the new (six-argument) way of calling load interceptor property
IC. Please check this only.

[William Hesse, 2009-07-30 08:18:52]

LGTM.