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. http://codereview.chromium.org/2853003/show -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev
