Port KeyedCallIC implementation to x64 and ARM....

Port KeyedCallIC implementation to x64 and ARM.

Also edited ic-ia32.cc for clarity and better formatting.


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

[Mads Ager (chromium), 2010-06-15 16:14:58]

LGTM with a couple of nits.

http://codereview.chromium.org/2853003/diff/1/13
File src/arm/ic-arm.cc (right):

http://codereview.chromium.org/2853003/diff/1/13#newcode178
src/arm/ic-arm.cc:178: // key      - holds the smi key on entry and is
unchanged.
The comments here are a bit confusing since |key| is not unchanged in one of the
usages of this because |key| and |result| are the same register at that use
site.

http://codereview.chromium.org/2853003/diff/1/13#newcode335
src/arm/ic-arm.cc:335: Register result,
The comment says that receiver and key are both unchanged. However, it seems
that this is sometimes used where receiver or key is the same register as
result. In that case, receiver or key is changed.

Can we remove the result register argument and make it clear in the comment on
register usage that the receiver is overwritten with the result if the load
succeeds and is unchanged otherwise?

Probably similar for the other platforms.

http://codereview.chromium.org/2853003/diff/1/13#newcode355
src/arm/ic-arm.cc:355: // t2 - used for the index into the dictionary.
This line doesn't seem to belong here? :)

http://codereview.chromium.org/2853003/diff/1/13#newcode371
src/arm/ic-arm.cc:371: __ ldr(scratch2,
Why do you need |scratch2| here? Based on the comment in this function it would
make sense to load directly to |result| here. Remove the |result| argument to
make the overwriting clear? If that is not possible, at least update the
comments to state that result is unchanged unless the load succeeds.

http://codereview.chromium.org/2853003/diff/1/13#newcode392
src/arm/ic-arm.cc:392: // r0: key
Remove this line. |key| is passed as an argument.

http://codereview.chromium.org/2853003/diff/1/13#newcode704
src/arm/ic-arm.cc:704: // Check that the value n r1 is a JSFunction.
n -> in

http://codereview.chromium.org/2853003/diff/1/2
File test/mjsunit/keyed-call-generic.js (right):

http://codereview.chromium.org/2853003/diff/1/2#newcode31
test/mjsunit/keyed-call-generic.js:31: //  for(var i = 0; i != 10; i++ ) {
Code in comment.

[Vladislav Kaznacheev, 2010-06-16 09:08:53]

http://codereview.chromium.org/2853003/diff/1/13
File src/arm/ic-arm.cc (right):

http://codereview.chromium.org/2853003/diff/1/13#newcode178
src/arm/ic-arm.cc:178: // key      - holds the smi key on entry and is
unchanged.
Added clearer comments.
On 2010/06/15 16:14:58, Mads Ager wrote:
> The comments here are a bit confusing since |key| is not unchanged in one of
the
> usages of this because |key| and |result| are the same register at that use
> site.

http://codereview.chromium.org/2853003/diff/1/13#newcode335
src/arm/ic-arm.cc:335: Register result,
The function is called from two different places which expect results in
different registers (only one of them conflicting with the receiver). Removing
the result would require an extra register move in KeyedCallIC usage.
I have added comments clarifying what changes and when.
On 2010/06/15 16:14:58, Mads Ager wrote:
> The comment says that receiver and key are both unchanged. However, it seems
> that this is sometimes used where receiver or key is the same register as
> result. In that case, receiver or key is changed.
> 
> Can we remove the result register argument and make it clear in the comment on
> register usage that the receiver is overwritten with the result if the load
> succeeds and is unchanged otherwise?
> 
> Probably similar for the other platforms.

http://codereview.chromium.org/2853003/diff/1/13#newcode355
src/arm/ic-arm.cc:355: // t2 - used for the index into the dictionary.
On 2010/06/15 16:14:58, Mads Ager wrote:
> This line doesn't seem to belong here? :)

Removed

http://codereview.chromium.org/2853003/diff/1/13#newcode371
src/arm/ic-arm.cc:371: __ ldr(scratch2,
At one of the two call sites |result| is the same as |key| which is needed on
bailout, so overwrite is allowed only if the load succeeds. Added a comment to
that effect.

On 2010/06/15 16:14:58, Mads Ager wrote:
> Why do you need |scratch2| here? Based on the comment in this function it
would
> make sense to load directly to |result| here. Remove the |result| argument to
> make the overwriting clear? If that is not possible, at least update the
> comments to state that result is unchanged unless the load succeeds.

http://codereview.chromium.org/2853003/diff/1/13#newcode392
src/arm/ic-arm.cc:392: // r0: key
On 2010/06/15 16:14:58, Mads Ager wrote:
> Remove this line. |key| is passed as an argument.

Done.

http://codereview.chromium.org/2853003/diff/1/13#newcode704
src/arm/ic-arm.cc:704: // Check that the value n r1 is a JSFunction.
On 2010/06/15 16:14:58, Mads Ager wrote:
> n -> in

Done.

http://codereview.chromium.org/2853003/diff/1/2
File test/mjsunit/keyed-call-generic.js (right):

http://codereview.chromium.org/2853003/diff/1/2#newcode31
test/mjsunit/keyed-call-generic.js:31: //  for(var i = 0; i != 10; i++ ) {
On 2010/06/15 16:14:58, Mads Ager wrote:
> Code in comment.

Done.

[Vladislav Kaznacheev, 2010-06-16 10:03:01]

Fixed an assertion in arm debug mode. Submitting.