The IC exposes a register definition.

The IC exposes a register definition.

Centralize a register definition in an IC that provides:
1) symbolic names for the register (like, edx == receiver)
2) defines ordering when passed on the stack

Code that implements or uses the IC should use this definition instead of "knowing" what the registers are. Or at least have the definition to validate it's assumptions.

As a side effect of avoiding runtime static initializers (enforced by tools/check-static-initializers.sh, neat), I gave ownership of the registers array to CodeStubInterfaceDescriptor. This prompted a cleanup of that struct.

R=jkummerow@chromium.org

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

[mvstanton, 2014-06-24 16:20:24]

Hi Jakob, thx for the advice thus far. As discussed, this version manages to
avoid any function local or global runtime static initialization, not only for
the new IC register definitions, but it also gets the
CodeStubInterfaceDescriptor initialization to quit using the
race-condition-prone static initializers of register arrays.
Thx,
--Michael

[Jakob Kummerow, 2014-06-24 17:23:25]

Nice. High-level comments; I'm too tired right now to study the details for
correctness.

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

https://codereview.chromium.org/352583002/diff/20001/src/code-stubs.h#newcode351
src/code-stubs.h:351: // if hint_stack_parameter_count_ > 0, the code stub can
optimize the
nit: s/if/If/

https://codereview.chromium.org/352583002/diff/20001/src/code-stubs.h#newcode363
src/code-stubs.h:363: // assumed to be Tagged()
nit: trailing full stop

https://codereview.chromium.org/352583002/diff/20001/src/code-stubs.h#newcode364
src/code-stubs.h:364: Representation* register_param_representations_;
This should probably be a SmartArrayPointer too, but feel free to punt on that.

https://codereview.chromium.org/352583002/diff/20001/src/ia32/code-stubs-ia32.cc
File src/ia32/code-stubs-ia32.cc (right):

https://codereview.chromium.org/352583002/diff/20001/src/ia32/code-stubs-ia32...
src/ia32/code-stubs-ia32.cc:27: register_params_ = SmartArrayPointer<Register>(
As discussed:
(1) this can (hopefully!) go into the platform-independent code-stubs.cc
(2) let's use .Reset() instead of operator=.

https://codereview.chromium.org/352583002/diff/20001/src/ia32/code-stubs-ia32...
src/ia32/code-stubs-ia32.cc:39: 1, registers,
How about s/1/ARRAY_SIZE(registers)/ ? Less likely to be wrong, but more verbose
-- I'm not sure it'd be an improvement.

[Sven Panne, 2014-06-25 06:24:06]

https://codereview.chromium.org/352583002/diff/20001/src/ia32/code-stubs-ia32.cc
File src/ia32/code-stubs-ia32.cc (right):

https://codereview.chromium.org/352583002/diff/20001/src/ia32/code-stubs-ia32...
src/ia32/code-stubs-ia32.cc:39: 1, registers,
On 2014/06/24 17:23:24, Jakob wrote:
> How about s/1/ARRAY_SIZE(registers)/ ? Less likely to be wrong, but more
verbose
> -- I'm not sure it'd be an improvement.

We discussed similar things yesterday, another (IMHO slightly nicer) approach
would be adding a NULL sentinel to registers and let Initialize count the stuff.
Old skool... ;-)

The *right* approach (locally) would of course be using a std::vector with a
brace initializer. Perhaps in 2017...

[mvstanton, 2014-06-25 06:43:14]

Thanks for comments thus far, updated to "state of the art" for your
eye-straining pleasure. :p
--Michael

ps - Sven - 2017, only three years right? :D

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

https://codereview.chromium.org/352583002/diff/20001/src/code-stubs.h#newcode351
src/code-stubs.h:351: // if hint_stack_parameter_count_ > 0, the code stub can
optimize the
On 2014/06/24 17:23:24, Jakob wrote:
> nit: s/if/If/

Done.

https://codereview.chromium.org/352583002/diff/20001/src/code-stubs.h#newcode363
src/code-stubs.h:363: // assumed to be Tagged()
On 2014/06/24 17:23:24, Jakob wrote:
> nit: trailing full stop

Done.

https://codereview.chromium.org/352583002/diff/20001/src/code-stubs.h#newcode364
src/code-stubs.h:364: Representation* register_param_representations_;
On 2014/06/24 17:23:24, Jakob wrote:
> This should probably be a SmartArrayPointer too, but feel free to punt on
that.

Yep, I did this because otherwise the asymmetry is jarring.

https://codereview.chromium.org/352583002/diff/20001/src/ia32/code-stubs-ia32.cc
File src/ia32/code-stubs-ia32.cc (right):

https://codereview.chromium.org/352583002/diff/20001/src/ia32/code-stubs-ia32...
src/ia32/code-stubs-ia32.cc:27: register_params_ = SmartArrayPointer<Register>(
On 2014/06/24 17:23:24, Jakob wrote:
> As discussed:
> (1) this can (hopefully!) go into the platform-independent code-stubs.cc
> (2) let's use .Reset() instead of operator=.

Done.

https://codereview.chromium.org/352583002/diff/20001/src/ia32/code-stubs-ia32...
src/ia32/code-stubs-ia32.cc:39: 1, registers,
On 2014/06/24 17:23:24, Jakob wrote:
> How about s/1/ARRAY_SIZE(registers)/ ? Less likely to be wrong, but more
verbose
> -- I'm not sure it'd be an improvement.

Let's do it, because already arm64 made that change on it's own.

[Jakob Kummerow, 2014-06-25 10:31:56]

LGTM with comments.

https://codereview.chromium.org/352583002/diff/80001/src/code-stubs.cc
File src/code-stubs.cc (right):

https://codereview.chromium.org/352583002/diff/80001/src/code-stubs.cc#newcode44
src/code-stubs.cc:44: // Also the register parameter representations array if
one is specified.
nit: this sentence no verb.

https://codereview.chromium.org/352583002/diff/80001/src/code-stubs.cc#newcod...
src/code-stubs.cc:566: ASSERT_EQ(KeyedLoadIC::kRegisterArgumentCount, 2);
nit: not sure I see the value of this ASSERT... but if we go on refactoring this
code, it'll most likely be temporary anyway, so I don't care much.
Can you make it a STATIC_ASSERT though?

https://codereview.chromium.org/352583002/diff/80001/src/ia32/code-stubs-ia32.cc
File src/ia32/code-stubs-ia32.cc (right):

https://codereview.chromium.org/352583002/diff/80001/src/ia32/code-stubs-ia32...
src/ia32/code-stubs-ia32.cc:98: Register registers[] = { edx };
I think this could use LoadIC::ReceiverRegister() already... but since we have
to do more cleanup anyway, feel free to punt.

[mvstanton, 2014-06-25 12:29:00]

Right on, thanks! Will submit...
--Michael

https://codereview.chromium.org/352583002/diff/80001/src/code-stubs.cc
File src/code-stubs.cc (right):

https://codereview.chromium.org/352583002/diff/80001/src/code-stubs.cc#newcode44
src/code-stubs.cc:44: // Also the register parameter representations array if
one is specified.
On 2014/06/25 10:31:56, Jakob wrote:
> nit: this sentence no verb.

Done.

https://codereview.chromium.org/352583002/diff/80001/src/code-stubs.cc#newcod...
src/code-stubs.cc:566: ASSERT_EQ(KeyedLoadIC::kRegisterArgumentCount, 2);
On 2014/06/25 10:31:56, Jakob wrote:
> nit: not sure I see the value of this ASSERT... but if we go on refactoring
this
> code, it'll most likely be temporary anyway, so I don't care much.
> Can you make it a STATIC_ASSERT though?

Done (static assert). The reason for it is that this system is vulnerable to a
change in the number of arguments for KeyedLoadIC. As we've discussed there are
ideas going forward to possibly avoid creating an array of registers here, but
make the KeyedLoadIC more directly responsible.

https://codereview.chromium.org/352583002/diff/80001/src/ia32/code-stubs-ia32.cc
File src/ia32/code-stubs-ia32.cc (right):

https://codereview.chromium.org/352583002/diff/80001/src/ia32/code-stubs-ia32...
src/ia32/code-stubs-ia32.cc:98: Register registers[] = { edx };
On 2014/06/25 10:31:56, Jakob wrote:
> I think this could use LoadIC::ReceiverRegister() already... but since we have
> to do more cleanup anyway, feel free to punt.

Good catch, it's in the next CL!

[mvstanton, 2014-06-25 12:32:32]

Committed patchset #6 manually as r22011 (presubmit successful).