Change keyed store IC interface on x64 to take value, key, and receiver in re...

Change keyed store IC interface on x64 to take value, key, and receiver in registers rather than on the stack.

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

[Søren Thygesen Gjesse, 2010-05-20 11:54:32]

LGTM

http://codereview.chromium.org/2111011/diff/10001/11002
File src/x64/codegen-x64.cc (right):

http://codereview.chromium.org/2111011/diff/10001/11002#newcode694
src/x64/codegen-x64.cc:694: } else if (receiver_.is(value_)) {
I don't get this, can receiver_.is(value_) ever be true?

http://codereview.chromium.org/2111011/diff/10001/11002#newcode699
src/x64/codegen-x64.cc:699: } else if (key_.is(value_)) {
Ditto.

http://codereview.chromium.org/2111011/diff/10001/11002#newcode704
src/x64/codegen-x64.cc:704: // Value is now in rax. Its original location is
remembered in value_.
Maybe extend comment to say that value_ is used later to store the result, and
that receiver_ and key_ might not have their original values.

Alternatively use two locals receiver and key to avoid overwriting receiver_ and
key_ when swapping with rax.

http://codereview.chromium.org/2111011/diff/10001/11002#newcode718
src/x64/codegen-x64.cc:718: }
How about

  } else 
    // receiver_ is neither rcx nor rdx.
    __ movq(rdx, receiver_);
    if (!key_.is(rcx)) {
    __ movq(rcx, key_);
    }
  }

instead of

  } else if (key_.is(rcx)) {
    __ movq(rdx, receiver_);
  } else {
    __ movq(rcx, key_);
    __ movq(rdx, receiver_);
  }

http://codereview.chromium.org/2111011/diff/10001/11005
File src/x64/ic-x64.cc (right):

http://codereview.chromium.org/2111011/diff/10001/11005#newcode900
src/x64/ic-x64.cc:900: // rdx: JSArray
JSArray -> receiver (a JSArray)

Like in the comment below.

http://codereview.chromium.org/2111011/diff/10001/11005#newcode1065
src/x64/ic-x64.cc:1065: __ pop(rdx);
Please add the comment on rbx and rdi content here as well.

http://codereview.chromium.org/2111011/diff/10001/11005#newcode1102
src/x64/ic-x64.cc:1102: __ bind(&is_nan);
Please add comment on register content as above.

http://codereview.chromium.org/2111011/diff/10001/11007
File src/x64/virtual-frame-x64.cc (right):

http://codereview.chromium.org/2111011/diff/10001/11007#newcode1035
src/x64/virtual-frame-x64.cc:1035: // are a and b.  Other results can be live,
but must not be in a_reg or b_reg.
Please assert these asumptions.

http://codereview.chromium.org/2111011/diff/10001/11007#newcode1046
src/x64/virtual-frame-x64.cc:1046: // a must be in b_reg, b in a_reg.
How about adding swapping of results to the virtual frame? Then you don´t need
to rely on a and b being invalidated and the caller can decide when to
invalidate.

[William Hesse, 2010-05-20 14:56:09]

http://codereview.chromium.org/2111011/diff/10001/11002
File src/x64/codegen-x64.cc (right):

http://codereview.chromium.org/2111011/diff/10001/11002#newcode694
src/x64/codegen-x64.cc:694: } else if (receiver_.is(value_)) {
On 2010/05/20 11:54:32, Søren Gjesse wrote:
> I don't get this, can receiver_.is(value_) ever be true?

Yes.  var x=new Object(); x[i] = x;

http://codereview.chromium.org/2111011/diff/10001/11002#newcode704
src/x64/codegen-x64.cc:704: // Value is now in rax. Its original location is
remembered in value_.
On 2010/05/20 11:54:32, Søren Gjesse wrote:
> Maybe extend comment to say that value_ is used later to store the result, and
> that receiver_ and key_ might not have their original values.
> 
> Alternatively use two locals receiver and key to avoid overwriting receiver_
and
> key_ when swapping with rax.

Done.

http://codereview.chromium.org/2111011/diff/10001/11002#newcode718
src/x64/codegen-x64.cc:718: }
If key_ is rdx and receiver_ is rbx, we need to move key_ first.

On 2010/05/20 11:54:32, Søren Gjesse wrote:
> How about
> 
>   } else 
>     // receiver_ is neither rcx nor rdx.
>     __ movq(rdx, receiver_);
>     if (!key_.is(rcx)) {
>     __ movq(rcx, key_);
>     }
>   }
> 
> instead of
> 
>   } else if (key_.is(rcx)) {
>     __ movq(rdx, receiver_);
>   } else {
>     __ movq(rcx, key_);
>     __ movq(rdx, receiver_);
>   }

http://codereview.chromium.org/2111011/diff/10001/11005
File src/x64/ic-x64.cc (right):

http://codereview.chromium.org/2111011/diff/10001/11005#newcode900
src/x64/ic-x64.cc:900: // rdx: JSArray
On 2010/05/20 11:54:32, Søren Gjesse wrote:
> JSArray -> receiver (a JSArray)
> 
> Like in the comment below.

Done.

http://codereview.chromium.org/2111011/diff/10001/11005#newcode1065
src/x64/ic-x64.cc:1065: __ pop(rdx);
On 2010/05/20 11:54:32, Søren Gjesse wrote:
> Please add the comment on rbx and rdi content here as well.

Done.

http://codereview.chromium.org/2111011/diff/10001/11005#newcode1102
src/x64/ic-x64.cc:1102: __ bind(&is_nan);
On 2010/05/20 11:54:32, Søren Gjesse wrote:
> Please add comment on register content as above.

Done.

http://codereview.chromium.org/2111011/diff/10001/11007
File src/x64/virtual-frame-x64.cc (right):

http://codereview.chromium.org/2111011/diff/10001/11007#newcode1035
src/x64/virtual-frame-x64.cc:1035: // are a and b.  Other results can be live,
but must not be in a_reg or b_reg.
On 2010/05/20 11:54:32, Søren Gjesse wrote:
> Please assert these asumptions.

Done.

http://codereview.chromium.org/2111011/diff/10001/11007#newcode1046
src/x64/virtual-frame-x64.cc:1046: // a must be in b_reg, b in a_reg.
This is only used in setting up arguments in registers for
an ic call, which relies on a spilled frame and does not preserve registers on
return, so we don't need this.
We could also just swap using a temporary variable.

It On 2010/05/20 11:54:32, Søren Gjesse wrote:
> How about adding swapping of results to the virtual frame? Then you don´t need
> to rely on a and b being invalidated and the caller can decide when to
> invalidate.