Move inlined function declarations and support from codegen.* to runtime.*.

Move inlined function declarations and support from codegen.* to runtime.*.

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

[William Hesse, 2010-09-10 13:47:31]

Still needs to be ported to all platforms.  Just looking for general comments at
this point, about the approach.

[William Hesse, 2010-09-10 13:47:34]

Still needs to be ported to all platforms.  Just looking for general comments at
this point, about the approach.

[William Hesse, 2010-09-13 14:36:15]

A slight strangeness: The distinction between INLINE and INLINE_RUNTIME is not
used anywhere.  We could merge them.
 
Also, no constant of the form Intrinsics::kInline<name> is ever used explicitly
(they are used implicitly in the definition of the static array of function
information).

Tested using shell tests, and also built as part of Chromium.

[Kasper Lund, 2010-09-14 06:07:14]

There are definitely a lot of good stuff in this change, but I would probably
have preferred the change without changing the Runtime prefix for runtime
functions to Intrinsics.

First round of comments:

http://codereview.chromium.org/3293002/diff/53002/51040
File src/arm/builtins-arm.cc (right):

http://codereview.chromium.org/3293002/diff/53002/51040#newcode706
src/arm/builtins-arm.cc:706: __ push(r1);  // argument for Runtime_NewObject
You should probably grep for Runtime and see if commments (like this one) needs
updating too.

http://codereview.chromium.org/3293002/diff/53002/51040#newcode707
src/arm/builtins-arm.cc:707: __ CallRuntime(Intrinsics::kNewObject, 1);
Somehow it feels incomplete that you use CallRuntime to call something in the
Intrinsics namespace.

http://codereview.chromium.org/3293002/diff/53002/51042
File src/arm/code-stubs-arm.h (right):

http://codereview.chromium.org/3293002/diff/53002/51042#newcode48
src/arm/code-stubs-arm.h:48: Intrinsics::FunctionId RuntimeFunction();
Again, the RuntimeFunction name that returns an Intrinsics::FunctionId seems
incomplete.

http://codereview.chromium.org/3293002/diff/53002/51044
File src/arm/codegen-arm.h (right):

http://codereview.chromium.org/3293002/diff/53002/51044#newcode446
src/arm/codegen-arm.h:446: typedef void
(CodeGenerator::*InlineFunctionGenerator)
Type definitions should go before all the methods.

http://codereview.chromium.org/3293002/diff/53002/51044#newcode449
src/arm/codegen-arm.h:449: static InlineFunctionGenerator
FindInlineFunctionGenerator(int function_id);
Why is this id and int and not some enum?

http://codereview.chromium.org/3293002/diff/53002/51053
File src/ast.h (right):

http://codereview.chromium.org/3293002/diff/53002/51053#newcode1172
src/ast.h:1172: Intrinsics::FunctionData* function,
I would expect a thing called function to be of kind Intrinsics::Function* not
FunctionData. I think this might need a bit more renaming.

http://codereview.chromium.org/3293002/diff/53002/51054
File src/codegen.cc (right):

http://codereview.chromium.org/3293002/diff/53002/51054#newcode347
src/codegen.cc:347: // Lookup table for code generators for  special runtime
calls which are
Double space between for and special ("for  special").

http://codereview.chromium.org/3293002/diff/53002/51054#newcode371
src/codegen.cc:371: if (function_data != NULL &&
How about doing an early bailout (return false) if function_data == NULL? After
that you can add a local with the intrinsic_type and make the code easier to
read.

http://codereview.chromium.org/3293002/diff/53002/51065
File src/ia32/codegen-ia32.h (right):

http://codereview.chromium.org/3293002/diff/53002/51065#newcode623
src/ia32/codegen-ia32.h:623: typedef void
(CodeGenerator::*InlineFunctionGenerator)
See comments on other platform.

http://codereview.chromium.org/3293002/diff/53002/51072
File src/parser.cc (right):

http://codereview.chromium.org/3293002/diff/53002/51072#newcode4247
src/parser.cc:4247: if (!is_pre_parsing_) {
Doesn't NEW in the presence of preparsing just return NULL? Maybe we could do an
early bailout here - if (is_preparsing) return NULL - and avoid some of the
nesting here. Nit.

http://codereview.chromium.org/3293002/diff/53002/51072#newcode4260
src/parser.cc:4260: // %IS_VAR(x) evaluates to x if x is a variable,
Maybe we should double check that we're still using this nasty %IS_VAR business.
If we could get rid of this it would be nice.

http://codereview.chromium.org/3293002/diff/53002/51072#newcode4272
src/parser.cc:4272: && function_data->number_of_args != -1
Here && is at the start of the line. Above it's at the start of the line. Maybe
make this consistent.

http://codereview.chromium.org/3293002/diff/53002/51073
File src/runtime.cc (right):

http://codereview.chromium.org/3293002/diff/53002/51073#newcode10110
src/runtime.cc:10110: const int Intrinsics::kNotFound = -1;
Why not put this in the header file?

http://codereview.chromium.org/3293002/diff/53002/51073#newcode10114
src/runtime.cc:10114: ASSERT(dictionary != NULL);
Maybe you should be asserting that we're bootstrapping?

http://codereview.chromium.org/3293002/diff/53002/51073#newcode10118
src/runtime.cc:10118: if (name_symbol->IsFailure()) return name_symbol;
This seems very, very broken. This code cannot deal with failures (it isn't
retried) so the return value should probably be true/false instead.

http://codereview.chromium.org/3293002/diff/53002/51073#newcode10141
src/runtime.cc:10141: Intrinsics::FunctionId fid) {
I prefer id over fid.

http://codereview.chromium.org/3293002/diff/53002/51074
File src/runtime.h (right):

http://codereview.chromium.org/3293002/diff/53002/51074#newcode500
src/runtime.h:500: 
Extra newline here.

http://codereview.chromium.org/3293002/diff/53002/51074#newcode503
src/runtime.h:503: 
Remove newline here.

[William Hesse, 2010-09-14 13:19:12]

All comments addressed.  Change list is much smaller and cleaner now. 
Initialization code does not assert that bootstrapper is active because all the
surrounding similar initialization code does not.  Initialization code returns
an object, or failure, and requirements for running again after failure are
documented.

[Kasper Lund, 2010-09-14 13:45:36]

LGTM with a few comments:

http://codereview.chromium.org/3293002/diff/108002/109005
File src/full-codegen.h (right):

http://codereview.chromium.org/3293002/diff/108002/109005#newcode250
src/full-codegen.h:250: static InlineFunctionGenerator
kInlineFunctionGenerators[];
Strictly speaking the data members should be after all methods (cross platform).

http://codereview.chromium.org/3293002/diff/108002/109011
File src/runtime.cc (right):

http://codereview.chromium.org/3293002/diff/108002/109011#newcode10109
src/runtime.cc:10109: const int Runtime::kNotFound;
I'm not sure you really need this. We don't usually have them from static consts
defined in the header.

http://codereview.chromium.org/3293002/diff/108002/109012
File src/runtime.h (right):

http://codereview.chromium.org/3293002/diff/108002/109012#newcode476
src/runtime.h:476: // The number of arguments expected. nargs < 0 if the
function takes
nargs < 0 => Negative?

[William Hesse, 2010-09-20 10:49:28]

The definition of the static const int member is removed from the implementation
file.

http://codereview.chromium.org/3293002/diff/108002/109004
File src/full-codegen.cc (right):

http://codereview.chromium.org/3293002/diff/108002/109004#newcode513
src/full-codegen.cc:513: FullCodeGenerator::InlineFunctionGenerator
Added const qualifier.

http://codereview.chromium.org/3293002/diff/108002/109005
File src/full-codegen.h (right):

http://codereview.chromium.org/3293002/diff/108002/109005#newcode250
src/full-codegen.h:250: static InlineFunctionGenerator
kInlineFunctionGenerators[];
On 2010/09/14 13:45:36, Kasper Lund wrote:
> Strictly speaking the data members should be after all methods (cross
platform).

This is a static const array, so it should be at the top of the section, after
typedefs.  Added "const" to the declaration.

http://codereview.chromium.org/3293002/diff/108002/109011
File src/runtime.cc (right):

http://codereview.chromium.org/3293002/diff/108002/109011#newcode10109
src/runtime.cc:10109: const int Runtime::kNotFound;
On 2010/09/14 13:45:36, Kasper Lund wrote:
> I'm not sure you really need this. We don't usually have them from static
consts
> defined in the header.

The C++ standard seems to say this is needed unless every use of the constant is
in a place _requiring_ a compile-time constant.

http://codereview.chromium.org/3293002/diff/108002/109012
File src/runtime.h (right):

http://codereview.chromium.org/3293002/diff/108002/109012#newcode476
src/runtime.h:476: // The number of arguments expected. nargs < 0 if the
function takes
On 2010/09/14 13:45:36, Kasper Lund wrote:
> nargs < 0 => Negative?

Done.