Refactor fast API call.

Refactor fast API call.

Make it use custom call generator infrastructure.

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

[antonm, 2011-03-11 20:43:14]

Vitaly,

may you have a look?

I think this unification is a good idea, but I by no means insist.  If you're
fine with general approach, I'll finish ports to other platforms.

[Vitaly Repeshko, 2011-03-14 15:33:10]

The approach looks good. Waiting for the next review round...


Thanks,
Vitaly

http://codereview.chromium.org/6686003/diff/1/src/hydrogen.cc
File src/hydrogen.cc (right):

http://codereview.chromium.org/6686003/diff/1/src/hydrogen.cc#newcode4265
src/hydrogen.cc:4265: // because it is likely to generate better code. 
Similarly, we
The comment should be updated.

http://codereview.chromium.org/6686003/diff/1/src/ia32/stub-cache-ia32.cc
File src/ia32/stub-cache-ia32.cc (right):

http://codereview.chromium.org/6686003/diff/1/src/ia32/stub-cache-ia32.cc#new...
src/ia32/stub-cache-ia32.cc:2138: if (object->IsGlobalObject()) return
Heap::undefined_value();
Please document the checks.

http://codereview.chromium.org/6686003/diff/1/src/ia32/stub-cache-ia32.cc#new...
src/ia32/stub-cache-ia32.cc:2159: // before to call any runtime function.
"to call" -> "calling"?

http://codereview.chromium.org/6686003/diff/1/src/ia32/stub-cache-ia32.cc#new...
src/ia32/stub-cache-ia32.cc:2167: __ mov(eax, Operand(esp, 3 * kPointerSize));
I think this could be combined with the code that reserves the space above.
Maybe it could become the new ReserveSpaceForFastApiCall? Also, it'd be nice to
get rid of the magic 3.

http://codereview.chromium.org/6686003/diff/1/src/ia32/stub-cache-ia32.cc#new...
src/ia32/stub-cache-ia32.cc:2170: // esp[2 * kPointerSize] is uninitialized,
esp[3 * kPointerSize] contains
Nice! But if manage to switch to using helper functions here, we can get rid of
the comment.

http://codereview.chromium.org/6686003/diff/1/src/ia32/stub-cache-ia32.cc#new...
src/ia32/stub-cache-ia32.cc:2176: __ add(Operand(esp),
Immediate(kFastApiCallArguments * kPointerSize));
New FreeSpace...?

http://codereview.chromium.org/6686003/diff/1/src/ia32/stub-cache-ia32.cc#new...
src/ia32/stub-cache-ia32.cc:2187: 
nit: Too many blank lines.

http://codereview.chromium.org/6686003/diff/1/src/stub-cache.h
File src/stub-cache.h (right):

http://codereview.chromium.org/6686003/diff/1/src/stub-cache.h#newcode733
src/stub-cache.h:733: CallOptimization* optimization,
Should work with "const CallOptimization&".

[antonm, 2011-03-15 08:57:37]

http://codereview.chromium.org/6686003/diff/1/src/hydrogen.cc
File src/hydrogen.cc (right):

http://codereview.chromium.org/6686003/diff/1/src/hydrogen.cc#newcode4265
src/hydrogen.cc:4265: // because it is likely to generate better code. 
Similarly, we
On 2011/03/14 15:33:10, Vitaly Repeshko wrote:
> The comment should be updated.

Done.

http://codereview.chromium.org/6686003/diff/1/src/ia32/stub-cache-ia32.cc
File src/ia32/stub-cache-ia32.cc (right):

http://codereview.chromium.org/6686003/diff/1/src/ia32/stub-cache-ia32.cc#new...
src/ia32/stub-cache-ia32.cc:2159: // before to call any runtime function.
On 2011/03/14 15:33:10, Vitaly Repeshko wrote:
> "to call" -> "calling"?

Done.

http://codereview.chromium.org/6686003/diff/1/src/ia32/stub-cache-ia32.cc#new...
src/ia32/stub-cache-ia32.cc:2167: __ mov(eax, Operand(esp, 3 * kPointerSize));
On 2011/03/14 15:33:10, Vitaly Repeshko wrote:
> I think this could be combined with the code that reserves the space above.
> Maybe it could become the new ReserveSpaceForFastApiCall? Also, it'd be nice
to
> get rid of the magic 3.

I am not sure as eax is used as a scratch in checkprototypes.

http://codereview.chromium.org/6686003/diff/1/src/ia32/stub-cache-ia32.cc#new...
src/ia32/stub-cache-ia32.cc:2170: // esp[2 * kPointerSize] is uninitialized,
esp[3 * kPointerSize] contains
On 2011/03/14 15:33:10, Vitaly Repeshko wrote:
> Nice! But if manage to switch to using helper functions here, we can get rid
of
> the comment.

See above

http://codereview.chromium.org/6686003/diff/1/src/ia32/stub-cache-ia32.cc#new...
src/ia32/stub-cache-ia32.cc:2176: __ add(Operand(esp),
Immediate(kFastApiCallArguments * kPointerSize));
On 2011/03/14 15:33:10, Vitaly Repeshko wrote:
> New FreeSpace...?

Done.

http://codereview.chromium.org/6686003/diff/1/src/ia32/stub-cache-ia32.cc#new...
src/ia32/stub-cache-ia32.cc:2187: 
On 2011/03/14 15:33:10, Vitaly Repeshko wrote:
> nit: Too many blank lines.

Done.

http://codereview.chromium.org/6686003/diff/1/src/stub-cache.h
File src/stub-cache.h (right):

http://codereview.chromium.org/6686003/diff/1/src/stub-cache.h#newcode733
src/stub-cache.h:733: CallOptimization* optimization,
On 2011/03/14 15:33:10, Vitaly Repeshko wrote:
> Should work with "const CallOptimization&".

Well, it looks like typically v8 uses pointers, not references.  I'll switch if
you insist.

[Vitaly Repeshko, 2011-03-17 15:30:04]

LGTM

http://codereview.chromium.org/6686003/diff/1/src/ia32/stub-cache-ia32.cc
File src/ia32/stub-cache-ia32.cc (right):

http://codereview.chromium.org/6686003/diff/1/src/ia32/stub-cache-ia32.cc#new...
src/ia32/stub-cache-ia32.cc:2167: __ mov(eax, Operand(esp, 3 * kPointerSize));
On 2011/03/15 08:57:37, antonm wrote:
> On 2011/03/14 15:33:10, Vitaly Repeshko wrote:
> > I think this could be combined with the code that reserves the space above.
> > Maybe it could become the new ReserveSpaceForFastApiCall? Also, it'd be nice
> to
> > get rid of the magic 3.
> 
> I am not sure as eax is used as a scratch in checkprototypes.

If it's a scratch, then it can be used in the reserve code. It'll be a bit nicer
this way. But making a new helper from it is probably not worth it, because we
can't use it in the interceptor code.

http://codereview.chromium.org/6686003/diff/1/src/stub-cache.h
File src/stub-cache.h (right):

http://codereview.chromium.org/6686003/diff/1/src/stub-cache.h#newcode733
src/stub-cache.h:733: CallOptimization* optimization,
On 2011/03/15 08:57:37, antonm wrote:
> On 2011/03/14 15:33:10, Vitaly Repeshko wrote:
> > Should work with "const CallOptimization&".
> 
> Well, it looks like typically v8 uses pointers, not references.  I'll switch
if
> you insist.

I insist on every comment I write, unless marked otherwise :) I really think we
should try to use C++ features when they provide more safety and don't get in
the way.

[antonm, 2011-03-17 17:24:55]

http://codereview.chromium.org/6686003/diff/1/src/stub-cache.h
File src/stub-cache.h (right):

http://codereview.chromium.org/6686003/diff/1/src/stub-cache.h#newcode733
src/stub-cache.h:733: CallOptimization* optimization,
On 2011/03/17 15:30:04, Vitaly Repeshko wrote:
> On 2011/03/15 08:57:37, antonm wrote:
> > On 2011/03/14 15:33:10, Vitaly Repeshko wrote:
> > > Should work with "const CallOptimization&".
> > 
> > Well, it looks like typically v8 uses pointers, not references.  I'll switch
> if
> > you insist.
> 
> I insist on every comment I write, unless marked otherwise :) I really think
we
> should try to use C++ features when they provide more safety and don't get in
> the way.

Done.