Change KeyedLoadIC interface on ia32 to take receiver and name in registers.

Change KeyedLoadIC interface on ia32 to take receiver and name in registers.

Committed: http://code.google.com/p/v8/source/detail?r=3895

[Kevin Millikin (Chromium), 2010-02-17 12:50:52]

I'm not sure I like the (eax=receiver,ecx=key) calling convention.

I definitely don't like changing the terminology "key" ==> "name" in the keyed
ICs.  I think it just invites confusion.

If you spot cleanups in assembly code while making these changes (newer idioms
like CmpInstanceType or SmiTag), go ahead and put them in.

http://codereview.chromium.org/601080/diff/11/1014
File src/ia32/builtins-ia32.cc (right):

http://codereview.chromium.org/601080/diff/11/1014#newcode647
src/ia32/builtins-ia32.cc:647: __ mov(ecx, Operand(ebp, kIndexOffset));
So far, if we pass one register argument it is eax, and if we pass two they are
(edx,eax).  Based on that I would expect the index in eax and the arguments
object in edx.

We should adopt something that's not arbitrary.

http://codereview.chromium.org/601080/diff/11/1014#newcode660
src/ia32/builtins-ia32.cc:660: // Remove IC arguments from the stack and push
the nth argument.
Please change this comment.

http://codereview.chromium.org/601080/diff/11/1014#newcode663
src/ia32/builtins-ia32.cc:663: // Update the index on the stack and in register
eax.
Please change this comment (if you use ecx).

http://codereview.chromium.org/601080/diff/11/1010
File src/ia32/codegen-ia32.cc (right):

http://codereview.chromium.org/601080/diff/11/1010#newcode6794
src/ia32/codegen-ia32.cc:6794: Result name = cgen_->frame()->Pop();
Doesn't

cgen_->frame()->PushElementAt(1);
cgen_->frame()->PushElementAt(1);

work?  If not we should make it so.

http://codereview.chromium.org/601080/diff/11/1015
File src/ia32/ic-ia32.cc (right):

http://codereview.chromium.org/601080/diff/11/1015#newcode223
src/ia32/ic-ia32.cc:223: //  -- ecx    : name
Not really a "name", more like a "key".

http://codereview.chromium.org/601080/diff/11/1015#newcode245
src/ia32/ic-ia32.cc:245: __ movzx_b(edx, FieldOperand(edx,
Map::kInstanceTypeOffset));
These two instructions are

__ CmpInstanceType(edx, JS_OBJECT_TYPE);

http://codereview.chromium.org/601080/diff/11/1015#newcode252
src/ia32/ic-ia32.cc:252: __ sar(ebx, kSmiTagSize);
__ SmiUntag(ebx);

http://codereview.chromium.org/601080/diff/11/1015#newcode257
src/ia32/ic-ia32.cc:257: __ cmp(FieldOperand(edx, HeapObject::kMapOffset),
__ CheckMap(edx, Factory::fixed_array_map(), &check_pixel_array, true)

http://codereview.chromium.org/601080/diff/11/1015#newcode265
src/ia32/ic-ia32.cc:265: Operand(edx, ebx, times_4, FixedArray::kHeaderSize -
kHeapObjectTag));
FieldOperand(edx, ebx, times_4, FixedArray::kHeaderSize)

http://codereview.chromium.org/601080/diff/11/1015#newcode280
src/ia32/ic-ia32.cc:280: __ cmp(FieldOperand(edx, HeapObject::kMapOffset),
__ CheckMap(edx, Factory::pixel_array_map(), &slow, true);

http://codereview.chromium.org/601080/diff/11/1015#newcode318
src/ia32/ic-ia32.cc:318: __ cmp(FieldOperand(ebx, HeapObject::kMapOffset),
CheckMap if you wish.

http://codereview.chromium.org/601080/diff/11/1015#newcode396
src/ia32/ic-ia32.cc:396: //  -- ecx    : name
key?

http://codereview.chromium.org/601080/diff/11/1015#newcode474
src/ia32/ic-ia32.cc:474: __ sar(edx, kSmiTagSize);  // Untag the index.
__ SmiUntag(edx)

http://codereview.chromium.org/601080/diff/11/1015#newcode530
src/ia32/ic-ia32.cc:530: __ shl(eax, kSmiTagSize);
__ SmiTag(eax);

http://codereview.chromium.org/601080/diff/11/1015#newcode568
src/ia32/ic-ia32.cc:568: __ shl(eax, kSmiTagSize);
SmiTag.

http://codereview.chromium.org/601080/diff/11/1013
File src/ia32/virtual-frame-ia32.cc (right):

http://codereview.chromium.org/601080/diff/11/1013#newcode927
src/ia32/virtual-frame-ia32.cc:927: // Key and receiver are on top of the frame.
 The IC expects them on
Comment is now wrong.

http://codereview.chromium.org/601080/diff/11/1013#newcode931
src/ia32/virtual-frame-ia32.cc:931: // Put arguments in registers, and on stack.
Comment is wrong (no args are on the stack).

http://codereview.chromium.org/601080/diff/11/1013#newcode934
src/ia32/virtual-frame-ia32.cc:934: PrepareForCall(0, 0);  // Two stack args,
neither callee-dropped.
Comment is wrong.

[William Hesse, 2010-02-17 17:01:09]

All recommended changes made.  Interface changed to:
key: eax
receiver: edx

Some uses of SmiTag, SmiUntag, MapCheck added as suggested.

http://codereview.chromium.org/601080/diff/11/1014
File src/ia32/builtins-ia32.cc (right):

http://codereview.chromium.org/601080/diff/11/1014#newcode647
src/ia32/builtins-ia32.cc:647: __ mov(ecx, Operand(ebp, kIndexOffset));
On 2010/02/17 12:50:52, Kevin Millikin wrote:
> So far, if we pass one register argument it is eax, and if we pass two they
are
> (edx,eax).  Based on that I would expect the index in eax and the arguments
> object in edx.
> 
> We should adopt something that's not arbitrary.

OK: receiver in edx, index in eax.

http://codereview.chromium.org/601080/diff/11/1010
File src/ia32/codegen-ia32.cc (right):

http://codereview.chromium.org/601080/diff/11/1010#newcode6794
src/ia32/codegen-ia32.cc:6794: Result name = cgen_->frame()->Pop();
On 2010/02/17 12:50:52, Kevin Millikin wrote:
> Doesn't
> 
> cgen_->frame()->PushElementAt(1);
> cgen_->frame()->PushElementAt(1);
> 
> work?  If not we should make it so.

Done.

http://codereview.chromium.org/601080/diff/11/1015
File src/ia32/ic-ia32.cc (right):

http://codereview.chromium.org/601080/diff/11/1015#newcode223
src/ia32/ic-ia32.cc:223: //  -- ecx    : name
On 2010/02/17 12:50:52, Kevin Millikin wrote:
> Not really a "name", more like a "key".

Done.

http://codereview.chromium.org/601080/diff/11/1015#newcode245
src/ia32/ic-ia32.cc:245: __ movzx_b(edx, FieldOperand(edx,
Map::kInstanceTypeOffset));
On 2010/02/17 12:50:52, Kevin Millikin wrote:
> These two instructions are
> 
> __ CmpInstanceType(edx, JS_OBJECT_TYPE);

Done.

http://codereview.chromium.org/601080/diff/11/1015#newcode252
src/ia32/ic-ia32.cc:252: __ sar(ebx, kSmiTagSize);
On 2010/02/17 12:50:52, Kevin Millikin wrote:
> __ SmiUntag(ebx);

Done.

http://codereview.chromium.org/601080/diff/11/1015#newcode257
src/ia32/ic-ia32.cc:257: __ cmp(FieldOperand(edx, HeapObject::kMapOffset),
On 2010/02/17 12:50:52, Kevin Millikin wrote:
> __ CheckMap(edx, Factory::fixed_array_map(), &check_pixel_array, true)

Done.

http://codereview.chromium.org/601080/diff/11/1015#newcode265
src/ia32/ic-ia32.cc:265: Operand(edx, ebx, times_4, FixedArray::kHeaderSize -
kHeapObjectTag));
On 2010/02/17 12:50:52, Kevin Millikin wrote:
> FieldOperand(edx, ebx, times_4, FixedArray::kHeaderSize)

Done.

[Kevin Millikin (Chromium), 2010-02-18 09:46:25]

LGTM.

http://codereview.chromium.org/601080/diff/8001/9001
File src/ia32/codegen-ia32.cc (right):

http://codereview.chromium.org/601080/diff/8001/9001#newcode6478
src/ia32/codegen-ia32.cc:6478: if (!receiver_.is(eax)) {
These are pretty hairy to reverse engineer.  I think two simple comments help:

if (!receiver_.is(eax)) {
  // Register eax is available for key.
  ...
} else if (!key_.is(edx)) {
  // Register edx is available for receiver.
  ...
} ...

http://codereview.chromium.org/601080/diff/8001/9001#newcode6798
src/ia32/codegen-ia32.cc:6798: Variable* var =
expression_->AsVariableProxy()->AsVariable();
It's a separate change, but var is always NULL and is_global is always false.

http://codereview.chromium.org/601080/diff/8001/9006
File src/ia32/ic-ia32.cc (right):

http://codereview.chromium.org/601080/diff/8001/9006#newcode266
src/ia32/ic-ia32.cc:266: __ mov(eax, ecx);
I think this code can be streamlined a bit (as a TODO) if you allow eax to hold
values other than the key (obviously preserving the key in a known place).

For example, you might eliminate some moves on the fast path like the one just
above.

http://codereview.chromium.org/601080/diff/8001/9004
File src/ia32/virtual-frame-ia32.cc (right):

http://codereview.chromium.org/601080/diff/8001/9004#newcode932
src/ia32/virtual-frame-ia32.cc:932: if (!key.is_register() ||
!key.reg().is(edx)) {
// Register edx is available for receiver.

http://codereview.chromium.org/601080/diff/8001/9004#newcode935
src/ia32/virtual-frame-ia32.cc:935: } else if (!receiver.is_register() ||
!receiver.reg().is(eax)) {
// Register eax is available for key.