Refactored custom call IC generators:

Refactored custom call IC generators:

 * All generators are listed in a single place.
 * Generators are installed as a separate pass in the bootstrapper.
 * Replaced pointers to generator functions with integer ids.

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

[Vitaly Repeshko, 2010-05-05 22:31:22]

Anton,

Here's a small refactoring for you. I'm going to use this for
string.char(Code)?At and some other builtin functions.

Thanks,
Vitaly

[antonm, 2010-05-06 05:54:04]

LGTM.

Mostly minor nits in comments.

http://codereview.chromium.org/1981002/diff/3001/4001
File src/arm/stub-cache-arm.cc (right):

http://codereview.chromium.org/1981002/diff/3001/4001#newcode1189
src/arm/stub-cache-arm.cc:1189: const int id =
function_info->custom_call_generator_id();
I am not sure it's V8's style to declare locals as consts unless it's
performance critical.  If you agree, all three platforms would require a change

http://codereview.chromium.org/1981002/diff/3001/4004
File src/bootstrapper.cc (right):

http://codereview.chromium.org/1981002/diff/3001/4004#newcode1348
src/bootstrapper.cc:1348: const int id =
CallStubCompiler::k##name##CallGenerator;              \
ditto for const

http://codereview.chromium.org/1981002/diff/3001/4007
File src/objects.h (right):

http://codereview.chromium.org/1981002/diff/3001/4007#newcode3203
src/objects.h:3203: // or Smi identifying a CustomCallGenerator.
you have removed CustomCallGenerator typedef, correct?  if yes, you might want
to change CustomCallGenerator into custom call generator.

http://codereview.chromium.org/1981002/diff/3001/4010
File src/stub-cache.cc (right):

http://codereview.chromium.org/1981002/diff/3001/4010#newcode1152
src/stub-cache.cc:1152: switch (generator_id) {
I guess technically we can get rid of generator ids completely as we could fetch
functions which should have custom call generator and just compare if |function|
passed is identical to one of those.  But solutions I'd investigate (prefetching
handles or looking up objects on runtime) apparently have performance
implications.  Another approach would be add those functions into context.

http://codereview.chromium.org/1981002/diff/3001/4010#newcode1164
src/stub-cache.cc:1164: return NULL;
maybe return undefined instead of NULL---in the worst case we would bail out to
regular cal constant stub generator.

http://codereview.chromium.org/1981002/diff/3001/4011
File src/stub-cache.h (right):

http://codereview.chromium.org/1981002/diff/3001/4011#newcode564
src/stub-cache.h:564: // The list is used by the bootstrapper to install custom
call
you might want to omit this para as there might be various things we could do
with this list.

http://codereview.chromium.org/1981002/diff/3001/4011#newcode583
src/stub-cache.h:583: #undef DECLARE_CALL_GENERATOR_ID
by no means insisting, but for me it's slightly easier to read if #undef is one
line higher---it's more obvious that kNumCallGenerators is not in the 'scope' of
DECLARE_CALL_GENERATOR_ID

[Vitaly Repeshko, 2010-05-06 13:04:17]

http://codereview.chromium.org/1981002/diff/3001/4001
File src/arm/stub-cache-arm.cc (right):

http://codereview.chromium.org/1981002/diff/3001/4001#newcode1189
src/arm/stub-cache-arm.cc:1189: const int id =
function_info->custom_call_generator_id();
On 2010/05/06 05:54:04, antonm wrote:
> I am not sure it's V8's style to declare locals as consts unless it's
> performance critical.  If you agree, all three platforms would require a
change

C'mon... const is a good thing.

http://codereview.chromium.org/1981002/diff/3001/4007
File src/objects.h (right):

http://codereview.chromium.org/1981002/diff/3001/4007#newcode3203
src/objects.h:3203: // or Smi identifying a CustomCallGenerator.
On 2010/05/06 05:54:04, antonm wrote:
> you have removed CustomCallGenerator typedef, correct?  if yes, you might want
> to change CustomCallGenerator into custom call generator.

Done.

http://codereview.chromium.org/1981002/diff/3001/4010
File src/stub-cache.cc (right):

http://codereview.chromium.org/1981002/diff/3001/4010#newcode1152
src/stub-cache.cc:1152: switch (generator_id) {
On 2010/05/06 05:54:04, antonm wrote:
> I guess technically we can get rid of generator ids completely as we could
fetch
> functions which should have custom call generator and just compare if
|function|
> passed is identical to one of those.  But solutions I'd investigate
(prefetching
> handles or looking up objects on runtime) apparently have performance
> implications.  Another approach would be add those functions into context.

I think fetching these functions from the context won't work, because it will
fetch the current (possibly modified) function as opposed to the builtin one.
This could be worked around, but I prefer the current approach because it's more
explicit.

http://codereview.chromium.org/1981002/diff/3001/4010#newcode1164
src/stub-cache.cc:1164: return NULL;
On 2010/05/06 05:54:04, antonm wrote:
> maybe return undefined instead of NULL---in the worst case we would bail out
to
> regular cal constant stub generator.

Good idea. Done.

http://codereview.chromium.org/1981002/diff/3001/4011
File src/stub-cache.h (right):

http://codereview.chromium.org/1981002/diff/3001/4011#newcode564
src/stub-cache.h:564: // The list is used by the bootstrapper to install custom
call
On 2010/05/06 05:54:04, antonm wrote:
> you might want to omit this para as there might be various things we could do
> with this list.

I like the fact it at least gives a hint on how this stuff is setup. I changed
the comment a bit.

http://codereview.chromium.org/1981002/diff/3001/4011#newcode583
src/stub-cache.h:583: #undef DECLARE_CALL_GENERATOR_ID
On 2010/05/06 05:54:04, antonm wrote:
> by no means insisting, but for me it's slightly easier to read if #undef is
one
> line higher---it's more obvious that kNumCallGenerators is not in the 'scope'
of
> DECLARE_CALL_GENERATOR_ID

Made the scope more clear :)