Use the same registers for StoreIC and KeyedStoreIC.

Use the same registers for StoreIC and KeyedStoreIC.

The x64 and ia32 platforms already did this, port to arm and arm64.

R=mstarzinger@chromium.org

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

[mvstanton, 2014-07-10 15:45:48]

Hi Michael, here is the change we discussed.
It makes StoreIC and KeyedStoreIC follow the same register specification. Thanks
for the look!
--Michael

[Michael Starzinger, 2014-07-11 08:27:08]

LGTM, only nits about the parameter constants.

https://codereview.chromium.org/385553004/diff/1/src/code-stubs.h
File src/code-stubs.h (right):

https://codereview.chromium.org/385553004/diff/1/src/code-stubs.h#newcode2458
src/code-stubs.h:2458: enum RegisterInfo {
nit: Let's add a comment "// Parameters accessed via
CodeStubGraphBuilder::GetParameter()" similar to the other hydrogen stubs here.
Also does it really increase readability to have an enum for this instead of a
list of const ints? I would prefer the latter.

https://codereview.chromium.org/385553004/diff/1/src/code-stubs.h#newcode2463
src/code-stubs.h:2463: kRegisterArgumentCount
nit: The register count doesn't seem to be used. Can we drop it to avoid
confusion?

[mvstanton, 2014-07-11 08:43:09]

Thx Michael, I have a question back to you, see inline and tell me what you
think. Thx again,
--Michael

https://codereview.chromium.org/385553004/diff/1/src/code-stubs.h
File src/code-stubs.h (right):

https://codereview.chromium.org/385553004/diff/1/src/code-stubs.h#newcode2458
src/code-stubs.h:2458: enum RegisterInfo {
On 2014/07/11 08:27:08, Michael Starzinger wrote:
> nit: Let's add a comment "// Parameters accessed via
> CodeStubGraphBuilder::GetParameter()" similar to the other hydrogen stubs
here.
> Also does it really increase readability to have an enum for this instead of a
> list of const ints? I would prefer the latter.

Cool, I'll add the comment. As for the enum format, I'm following the convention
laid down in a few previous CLs in ic.h for Load/StoreKeyed. You get an enum
which gives you ordering for stack-based calls, then the registers. If it's cool
I'll keep that convention for now, perhaps later do a refactoring CL to turn
them into const ints monolithically.

https://codereview.chromium.org/385553004/diff/1/src/code-stubs.h#newcode2463
src/code-stubs.h:2463: kRegisterArgumentCount
On 2014/07/11 08:27:08, Michael Starzinger wrote:
> nit: The register count doesn't seem to be used. Can we drop it to avoid
> confusion?

It's part of the convention, and is used in some asserts for some ICs, but it's
true, I don't use this one. Actually, how about this idea: the bailout method
uses the constant in an assert around arguments length, the bailout method could
also use those indexes to extract arguments instead of 0, 1, 2, etc.

[Michael Starzinger, 2014-07-11 09:04:11]

Still LGTM.

https://codereview.chromium.org/385553004/diff/1/src/code-stubs.h
File src/code-stubs.h (right):

https://codereview.chromium.org/385553004/diff/1/src/code-stubs.h#newcode2458
src/code-stubs.h:2458: enum RegisterInfo {
On 2014/07/11 08:43:09, mvstanton wrote:
> On 2014/07/11 08:27:08, Michael Starzinger wrote:
> > nit: Let's add a comment "// Parameters accessed via
> > CodeStubGraphBuilder::GetParameter()" similar to the other hydrogen stubs
> here.
> > Also does it really increase readability to have an enum for this instead of
a
> > list of const ints? I would prefer the latter.
> 
> Cool, I'll add the comment. As for the enum format, I'm following the
convention
> laid down in a few previous CLs in ic.h for Load/StoreKeyed. You get an enum
> which gives you ordering for stack-based calls, then the registers. If it's
cool
> I'll keep that convention for now, perhaps later do a refactoring CL to turn
> them into const ints monolithically.

As discussed offline: In this case I am fine with keeping the enum as it is. But
please add the comment.

https://codereview.chromium.org/385553004/diff/1/src/code-stubs.h#newcode2463
src/code-stubs.h:2463: kRegisterArgumentCount
On 2014/07/11 08:43:09, mvstanton wrote:
> On 2014/07/11 08:27:08, Michael Starzinger wrote:
> > nit: The register count doesn't seem to be used. Can we drop it to avoid
> > confusion?
> 
> It's part of the convention, and is used in some asserts for some ICs, but
it's
> true, I don't use this one. Actually, how about this idea: the bailout method
> uses the constant in an assert around arguments length, the bailout method
could
> also use those indexes to extract arguments instead of 0, 1, 2, etc.

Acknowledged.

[mvstanton, 2014-07-11 13:53:35]

Committed patchset #1 manually as r22358 (tree was closed).