First take on custom call generators.

First take on custom call generators.

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

[antonm, 2010-03-04 21:53:53]

Mads,

may you have a look?

http://codereview.chromium.org/669061/diff/1009/19
File src/ia32/macro-assembler-ia32.cc (right):

http://codereview.chromium.org/669061/diff/1009/19#newcode171
src/ia32/macro-assembler-ia32.cc:171: lea(dst, Operand(object, dst,
times_half_pointer_size,
btw, this dst (in Operand) seems mildly suspicious to me as it's not
initialized.

[Mads Ager (chromium), 2010-03-08 12:39:28]

http://codereview.chromium.org/669061/diff/1009/16
File src/bootstrapper.cc (right):

http://codereview.chromium.org/669061/diff/1009/16#newcode1465
src/bootstrapper.cc:1465: if (function_data->IsUndefined()) {
I wonder if this can have interesting side-effects?

Using function template info for this new pointer means that this function will
be seen as an API function (because the function_data field will be different
from undefined).  We need to make sure that this is ok.  Even if it is ok I
think it would be nice to find a less intrusive way to do this.  It is confusing
that function_data can be different from undefined for two very different
reasons now.

http://codereview.chromium.org/669061/diff/1009/19
File src/ia32/macro-assembler-ia32.cc (right):

http://codereview.chromium.org/669061/diff/1009/19#newcode171
src/ia32/macro-assembler-ia32.cc:171: lea(dst, Operand(object, dst,
times_half_pointer_size,
On 2010/03/04 21:53:53, antonm wrote:
> btw, this dst (in Operand) seems mildly suspicious to me as it's not
> initialized.

As discussed offline, this is indeed a bit strange. However, if |offset| is zero
|dst| is the array index so this is the intended behavior.

http://codereview.chromium.org/669061/diff/1009/20
File src/ia32/macro-assembler-ia32.h (right):

http://codereview.chromium.org/669061/diff/1009/20#newcode57
src/ia32/macro-assembler-ia32.h:57: // scratch can be object itself, but it'd
get clobbered.
it'd get -> it will be?

http://codereview.chromium.org/669061/diff/1009/21
File src/ia32/stub-cache-ia32.cc (right):

http://codereview.chromium.org/669061/diff/1009/21#newcode1268
src/ia32/stub-cache-ia32.cc:1268: for (int i = 0; i < argc; i++) {
We should put a limit on the argc for which we unroll the loop here to control
code size.

http://codereview.chromium.org/669061/diff/1009/21#newcode1286
src/ia32/stub-cache-ia32.cc:1286: for (int i = 0; i < argc; i++) {
We should put a limit on argc for which we unroll the loop here to control code
size.  So, if argc is less than say 4 we unroll otherwise we generate code for a
loop to do this.

http://codereview.chromium.org/669061/diff/1009/21#newcode1291
src/ia32/stub-cache-ia32.cc:1291: __ push(edi);
This works well for one argument, but introduces a lot of pushes and pops for
more arguments.  We should be able to improve that by freeing some more
registers before entering the loop so the write barrier code clobbers other
registers instead.

It is not clear to me why we need to preserve ebx here?

[antonm, 2010-03-09 10:39:11]

Thanks a lot for comments, Mads.

http://codereview.chromium.org/669061/diff/1009/16
File src/bootstrapper.cc (right):

http://codereview.chromium.org/669061/diff/1009/16#newcode1465
src/bootstrapper.cc:1465: if (function_data->IsUndefined()) {
On 2010/03/08 12:39:28, Mads Ager wrote:
> I wonder if this can have interesting side-effects?
> 
> Using function template info for this new pointer means that this function
will
> be seen as an API function (because the function_data field will be different
> from undefined).  We need to make sure that this is ok.  Even if it is ok I
> think it would be nice to find a less intrusive way to do this.  It is
confusing
> that function_data can be different from undefined for two very different
> reasons now.

Mads, any recommendations are most appreciated.  I put it into the template for
the only reason---I didn't want to make all functions to pay additional pointer
overhead.  If you could recommend me some other place, that'd be cool.

One, but somewhat nasty approach, might be to overload function_data to hold
either API stuff or custom call generator, but that is somewhat smelly.

http://codereview.chromium.org/669061/diff/1009/19
File src/ia32/macro-assembler-ia32.cc (right):

http://codereview.chromium.org/669061/diff/1009/19#newcode171
src/ia32/macro-assembler-ia32.cc:171: lea(dst, Operand(object, dst,
times_half_pointer_size,
On 2010/03/08 12:39:28, Mads Ager wrote:
> On 2010/03/04 21:53:53, antonm wrote:
> > btw, this dst (in Operand) seems mildly suspicious to me as it's not
> > initialized.
> 
> As discussed offline, this is indeed a bit strange. However, if |offset| is
zero
> |dst| is the array index so this is the intended behavior.
> 
> 

Thanks a lot, Mads.

http://codereview.chromium.org/669061/diff/1009/20
File src/ia32/macro-assembler-ia32.h (right):

http://codereview.chromium.org/669061/diff/1009/20#newcode57
src/ia32/macro-assembler-ia32.h:57: // scratch can be object itself, but it'd
get clobbered.
On 2010/03/08 12:39:28, Mads Ager wrote:
> it'd get -> it will be?

Done.

http://codereview.chromium.org/669061/diff/1009/21
File src/ia32/stub-cache-ia32.cc (right):

http://codereview.chromium.org/669061/diff/1009/21#newcode1268
src/ia32/stub-cache-ia32.cc:1268: for (int i = 0; i < argc; i++) {
On 2010/03/08 12:39:28, Mads Ager wrote:
> We should put a limit on the argc for which we unroll the loop here to control
> code size.

Sure.  For now I am just calling a builtin if there are more than 5 args,
hopefully it's fine.

I would add a test for this case when we're closer to finalizing this CL.

http://codereview.chromium.org/669061/diff/1009/21#newcode1286
src/ia32/stub-cache-ia32.cc:1286: for (int i = 0; i < argc; i++) {
On 2010/03/08 12:39:28, Mads Ager wrote:
> We should put a limit on argc for which we unroll the loop here to control
code
> size.  So, if argc is less than say 4 we unroll otherwise we generate code for
a
> loop to do this.

Hopefully answered before.

http://codereview.chromium.org/669061/diff/1009/21#newcode1291
src/ia32/stub-cache-ia32.cc:1291: __ push(edi);
On 2010/03/08 12:39:28, Mads Ager wrote:
> This works well for one argument, but introduces a lot of pushes and pops for
> more arguments.  We should be able to improve that by freeing some more
> registers before entering the loop so the write barrier code clobbers other
> registers instead.

I'll try to do it.  Mads, is there somewhere a list of registers I can use at
this point without saving them?

Overall, though, it might be more robust to go into builtin for this case to
keep generated code as simple as possible.

> 
> It is not clear to me why we need to preserve ebx here?

Alas, currently RecordWrite clobbers object (it stores page into it, see ln. 58
(57 of reworked) macro-assembler-ia32.cc

[Mads Ager (chromium), 2010-03-09 11:51:00]

I see why you need to preserved ebx, sorry about that.  Playing with eliminating
some of the pushes and pops by freeing registers earlier still might make sense.

http://codereview.chromium.org/669061/diff/1009/16
File src/bootstrapper.cc (right):

http://codereview.chromium.org/669061/diff/1009/16#newcode1465
src/bootstrapper.cc:1465: if (function_data->IsUndefined()) {
On 2010/03/09 10:39:12, antonm wrote:
> On 2010/03/08 12:39:28, Mads Ager wrote:
> > I wonder if this can have interesting side-effects?
> > 
> > Using function template info for this new pointer means that this function
> will
> > be seen as an API function (because the function_data field will be
different
> > from undefined).  We need to make sure that this is ok.  Even if it is ok I
> > think it would be nice to find a less intrusive way to do this.  It is
> confusing
> > that function_data can be different from undefined for two very different
> > reasons now.
> 
> Mads, any recommendations are most appreciated.  I put it into the template
for
> the only reason---I didn't want to make all functions to pay additional
pointer
> overhead.  If you could recommend me some other place, that'd be cool.

You are definitely right that we should not waste a word extra for all functions
for this.

> One, but somewhat nasty approach, might be to overload function_data to hold
> either API stuff or custom call generator, but that is somewhat smelly.

It does smell, but I think it is better than the current approach because with
the current approach we cannot distinguish API functions from functions that
just have a custom call generator.  If we change the name of the field to
something like template_info_or_call_generator and add has_template_info and
has_call_generator methods to test which one is present (if any) it will not be
so bad and we should be able to avoid nasty side-effects.

[Mads Ager (chromium), 2010-03-10 08:06:21]

LGTM

http://codereview.chromium.org/669061/diff/12001/13001
File src/array.js (right):

http://codereview.chromium.org/669061/diff/12001/13001#newcode1110
src/array.js:1110: if (arguments.length == 3) {
We usually do:

if (!IS_UNDEFINED(len))

http://codereview.chromium.org/669061/diff/12001/13009
File src/ia32/stub-cache-ia32.cc (right):

http://codereview.chromium.org/669061/diff/12001/13009#newcode1248
src/ia32/stub-cache-ia32.cc:1248: Immediate(Factory::fixed_array_map()));
Identation one space off?

http://codereview.chromium.org/669061/diff/12001/13009#newcode1276
src/ia32/stub-cache-ia32.cc:1276: // Check if wrote not a smi.
wrote -> value is