KeyedLoadIC should have same register spec as LoadIC.

KeyedLoadIC should have same register spec as LoadIC.

On arm, arm64 and x64 there is a different register specification between LoadIC and KeyedLoadIC.

It would be nicer if these are the same, allowing some key optimizations.

R=jkummerow@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=22103

[mvstanton, 2014-06-26 13:18:06]

Hi Jakob,
Here is the CL discussed, for the moment I only have x64.
PTAL, thx!
--Michael

[Jakob Kummerow, 2014-06-26 16:17:10]

Looks good.

I would have suggested to land what you have, and then port to the other
platforms in another CL, but the platform-independent changes are making that
difficult...

https://codereview.chromium.org/338963003/diff/30001/src/ia32/builtins-ia32.cc
File src/ia32/builtins-ia32.cc (right):

https://codereview.chromium.org/338963003/diff/30001/src/ia32/builtins-ia32.c...
src/ia32/builtins-ia32.cc:1016: // Update the index on the stack and in register
eax.
nit: s/eax/|key|/

https://codereview.chromium.org/338963003/diff/30001/src/ia32/builtins-ia32.c...
src/ia32/builtins-ia32.cc:1028: ASSERT(!key.is(eax));
Here, too, consider using Move() instead.

https://codereview.chromium.org/338963003/diff/30001/src/x64/builtins-x64.cc
File src/x64/builtins-x64.cc (right):

https://codereview.chromium.org/338963003/diff/30001/src/x64/builtins-x64.cc#...
src/x64/builtins-x64.cc:1100: ASSERT(!key.is(rax));
why does this matter? AFAICS violating the ASSERT wouldn't hurt, so we can just
skip it.

https://codereview.chromium.org/338963003/diff/30001/src/x64/full-codegen-x64.cc
File src/x64/full-codegen-x64.cc (right):

https://codereview.chromium.org/338963003/diff/30001/src/x64/full-codegen-x64...
src/x64/full-codegen-x64.cc:1847: // We need the receiver both on the stack and
in the accumulator.
s/accumulator/register/

https://codereview.chromium.org/338963003/diff/30001/src/x64/full-codegen-x64...
src/x64/full-codegen-x64.cc:2069: ASSERT(!load_receiver.is(rax));
suggestion: in all the cases where you're ASSERTing that we're not moving
unnecessarily, you could simply use Move() instead of ASSERT+movp(). Move() 
does a src != dst check.

https://codereview.chromium.org/338963003/diff/30001/src/x64/ic-x64.cc
File src/x64/ic-x64.cc (right):

https://codereview.chromium.org/338963003/diff/30001/src/x64/ic-x64.cc#newcod...
src/x64/ic-x64.cc:368: // receiver: receiver
Yay for high-entropy comments! :-P

AFAICS, the |receiver| register always contains the receiver, and the |key|
register always contains the key (it is only overwritten with the "new"  key in
case of heapnumber->smi conversion). So you could drop the comments about those
two.

[mvstanton, 2014-06-30 13:19:09]

Hi Jakob,
Thanks for the look thus far. I uploaded a patch that responds to comments, then
an arm/arm64 patch for ease of reading.

Yes, if I have some trouble I'll divide the CL up by platform.  Until then
blithe confidence...:)

Thanks again,
--Michael

https://codereview.chromium.org/338963003/diff/30001/src/ia32/builtins-ia32.cc
File src/ia32/builtins-ia32.cc (right):

https://codereview.chromium.org/338963003/diff/30001/src/ia32/builtins-ia32.c...
src/ia32/builtins-ia32.cc:1016: // Update the index on the stack and in register
eax.
On 2014/06/26 16:17:10, Jakob wrote:
> nit: s/eax/|key|/

Done.

https://codereview.chromium.org/338963003/diff/30001/src/ia32/builtins-ia32.c...
src/ia32/builtins-ia32.cc:1028: ASSERT(!key.is(eax));
On 2014/06/26 16:17:10, Jakob wrote:
> Here, too, consider using Move() instead.

Done.

https://codereview.chromium.org/338963003/diff/30001/src/x64/builtins-x64.cc
File src/x64/builtins-x64.cc (right):

https://codereview.chromium.org/338963003/diff/30001/src/x64/builtins-x64.cc#...
src/x64/builtins-x64.cc:1100: ASSERT(!key.is(rax));
On 2014/06/26 16:17:10, Jakob wrote:
> why does this matter? AFAICS violating the ASSERT wouldn't hurt, so we can
just
> skip it.

Done.

https://codereview.chromium.org/338963003/diff/30001/src/x64/full-codegen-x64.cc
File src/x64/full-codegen-x64.cc (right):

https://codereview.chromium.org/338963003/diff/30001/src/x64/full-codegen-x64...
src/x64/full-codegen-x64.cc:2069: ASSERT(!load_receiver.is(rax));
On 2014/06/26 16:17:10, Jakob wrote:
> suggestion: in all the cases where you're ASSERTing that we're not moving
> unnecessarily, you could simply use Move() instead of ASSERT+movp(). Move() 
> does a src != dst check.

Awesome, didn't realize that, thx.

https://codereview.chromium.org/338963003/diff/30001/src/x64/ic-x64.cc
File src/x64/ic-x64.cc (right):

https://codereview.chromium.org/338963003/diff/30001/src/x64/ic-x64.cc#newcod...
src/x64/ic-x64.cc:368: // receiver: receiver
On 2014/06/26 16:17:10, Jakob wrote:
> Yay for high-entropy comments! :-P
> 
> AFAICS, the |receiver| register always contains the receiver, and the |key|
> register always contains the key (it is only overwritten with the "new"  key
in
> case of heapnumber->smi conversion). So you could drop the comments about
those
> two.

Will do, thanks.

[Jakob Kummerow, 2014-06-30 14:12:46]

LGTM with a bunch of nits.

Great work!

https://codereview.chromium.org/338963003/diff/110001/src/arm/full-codegen-ar...
File src/arm/full-codegen-arm.cc (right):

https://codereview.chromium.org/338963003/diff/110001/src/arm/full-codegen-ar...
src/arm/full-codegen-arm.cc:2631: // Move the key into the right register for
the keyed load IC.
nit: unnecessary comment (doesn't say anything that couldn't be seen in the
code)

https://codereview.chromium.org/338963003/diff/110001/src/arm/full-codegen-ar...
src/arm/full-codegen-arm.cc:4228: // Put the object both on the stack and in the
accumulator.
nit: s/accumulator/register/

https://codereview.chromium.org/338963003/diff/110001/src/arm/ic-arm.cc
File src/arm/ic-arm.cc (right):

https://codereview.chromium.org/338963003/diff/110001/src/arm/ic-arm.cc#newco...
src/arm/ic-arm.cc:317: //  -- r1    : receiver
Why doesn't this get the same treatment as e.g.
KeyedLoadIC::GenerateSloppyArguments below?

https://codereview.chromium.org/338963003/diff/110001/src/arm/ic-arm.cc#newco...
src/arm/ic-arm.cc:475: // The return address is on the stack.
actually, it's in |lr|

https://codereview.chromium.org/338963003/diff/110001/src/arm/stub-cache-arm.cc
File src/arm/stub-cache-arm.cc (right):

https://codereview.chromium.org/338963003/diff/110001/src/arm/stub-cache-arm....
src/arm/stub-cache-arm.cc:1478: //  -- lr     : return address
another comment that we an trim to just this line.

https://codereview.chromium.org/338963003/diff/110001/src/arm/stub-cache-arm....
src/arm/stub-cache-arm.cc:1501: //  -- r0     : key
outdated. Just drop the entire comment.

https://codereview.chromium.org/338963003/diff/110001/src/arm/stub-cache-arm....
src/arm/stub-cache-arm.cc:1511: //  -- r0     : key
again

https://codereview.chromium.org/338963003/diff/110001/src/arm64/full-codegen-...
File src/arm64/full-codegen-arm64.cc (right):

https://codereview.chromium.org/338963003/diff/110001/src/arm64/full-codegen-...
src/arm64/full-codegen-arm64.cc:2330: // Move the key into the right register
for the keyed load IC call.
don't need this comment.

https://codereview.chromium.org/338963003/diff/110001/src/arm64/ic-arm64.cc
File src/arm64/ic-arm64.cc (right):

https://codereview.chromium.org/338963003/diff/110001/src/arm64/ic-arm64.cc#n...
src/arm64/ic-arm64.cc:412: //  -- lr    : return address
another comment that can be trimmed to "The return address is in lr.", at least
if you use named registers below, possibly instead of the ASSERTs.

https://codereview.chromium.org/338963003/diff/110001/src/arm64/ic-arm64.cc#n...
src/arm64/ic-arm64.cc:496: __ Ldr(x0, unmapped_location);
I'd s/x0/result/ here (and in the next line)

https://codereview.chromium.org/338963003/diff/110001/src/arm64/ic-arm64.cc#n...
src/arm64/ic-arm64.cc:762: // Slow case
nit: trailing full stop please.

https://codereview.chromium.org/338963003/diff/110001/src/arm64/ic-arm64.cc#n...
src/arm64/ic-arm64.cc:780: void KeyedLoadIC::GenerateString(MacroAssembler*
masm) {
Wohoo! Register refactoring nirvana -- nothing to do here :-)

https://codereview.chromium.org/338963003/diff/110001/src/arm64/stub-cache-ar...
File src/arm64/stub-cache-arm64.cc (right):

https://codereview.chromium.org/338963003/diff/110001/src/arm64/stub-cache-ar...
src/arm64/stub-cache-arm64.cc:1453: //  -- lr     : return address
one more comment to trim.

[mvstanton, 2014-06-30 14:59:00]

Thanks Jakob! Okay rebasing/submitting asap.
--Michael

https://codereview.chromium.org/338963003/diff/110001/src/arm/full-codegen-ar...
File src/arm/full-codegen-arm.cc (right):

https://codereview.chromium.org/338963003/diff/110001/src/arm/full-codegen-ar...
src/arm/full-codegen-arm.cc:2631: // Move the key into the right register for
the keyed load IC.
On 2014/06/30 14:12:45, Jakob wrote:
> nit: unnecessary comment (doesn't say anything that couldn't be seen in the
> code)
True, deletin'.

https://codereview.chromium.org/338963003/diff/110001/src/arm/full-codegen-ar...
src/arm/full-codegen-arm.cc:4228: // Put the object both on the stack and in the
accumulator.
On 2014/06/30 14:12:45, Jakob wrote:
> nit: s/accumulator/register/

Done.

https://codereview.chromium.org/338963003/diff/110001/src/arm/ic-arm.cc
File src/arm/ic-arm.cc (right):

https://codereview.chromium.org/338963003/diff/110001/src/arm/ic-arm.cc#newco...
src/arm/ic-arm.cc:317: //  -- r1    : receiver
On 2014/06/30 14:12:45, Jakob wrote:
> Why doesn't this get the same treatment as e.g.
> KeyedLoadIC::GenerateSloppyArguments below?

Yep, I can do that.

https://codereview.chromium.org/338963003/diff/110001/src/arm/ic-arm.cc#newco...
src/arm/ic-arm.cc:475: // The return address is on the stack.
On 2014/06/30 14:12:45, Jakob wrote:
> actually, it's in |lr|

Done.

https://codereview.chromium.org/338963003/diff/110001/src/arm/stub-cache-arm.cc
File src/arm/stub-cache-arm.cc (right):

https://codereview.chromium.org/338963003/diff/110001/src/arm/stub-cache-arm....
src/arm/stub-cache-arm.cc:1478: //  -- lr     : return address
On 2014/06/30 14:12:45, Jakob wrote:
> another comment that we an trim to just this line.

Done.

https://codereview.chromium.org/338963003/diff/110001/src/arm/stub-cache-arm....
src/arm/stub-cache-arm.cc:1501: //  -- r0     : key
On 2014/06/30 14:12:45, Jakob wrote:
> outdated. Just drop the entire comment.

Done.

https://codereview.chromium.org/338963003/diff/110001/src/arm/stub-cache-arm....
src/arm/stub-cache-arm.cc:1511: //  -- r0     : key
On 2014/06/30 14:12:45, Jakob wrote:
> again

Done.

https://codereview.chromium.org/338963003/diff/110001/src/arm64/ic-arm64.cc
File src/arm64/ic-arm64.cc (right):

https://codereview.chromium.org/338963003/diff/110001/src/arm64/ic-arm64.cc#n...
src/arm64/ic-arm64.cc:412: //  -- lr    : return address
On 2014/06/30 14:12:45, Jakob wrote:
> another comment that can be trimmed to "The return address is in lr.", at
least
> if you use named registers below, possibly instead of the ASSERTs.

Done.

https://codereview.chromium.org/338963003/diff/110001/src/arm64/ic-arm64.cc#n...
src/arm64/ic-arm64.cc:496: __ Ldr(x0, unmapped_location);
On 2014/06/30 14:12:45, Jakob wrote:
> I'd s/x0/result/ here (and in the next line)

Done.

https://codereview.chromium.org/338963003/diff/110001/src/arm64/ic-arm64.cc#n...
src/arm64/ic-arm64.cc:762: // Slow case
On 2014/06/30 14:12:45, Jakob wrote:
> nit: trailing full stop please.

Done.

https://codereview.chromium.org/338963003/diff/110001/src/arm64/ic-arm64.cc#n...
src/arm64/ic-arm64.cc:780: void KeyedLoadIC::GenerateString(MacroAssembler*
masm) {
On 2014/06/30 14:12:45, Jakob wrote:
> Wohoo! Register refactoring nirvana -- nothing to do here :-)

:D

https://codereview.chromium.org/338963003/diff/110001/src/arm64/stub-cache-ar...
File src/arm64/stub-cache-arm64.cc (right):

https://codereview.chromium.org/338963003/diff/110001/src/arm64/stub-cache-ar...
src/arm64/stub-cache-arm64.cc:1453: //  -- lr     : return address
On 2014/06/30 14:12:45, Jakob wrote:
> one more comment to trim.

Done.

[mvstanton, 2014-06-30 15:58:34]

Committed patchset #5 manually as r22103 (presubmit successful).