Direct call C++ functions

The idea is to 3 categories:
1) That never fail and must not iterate through the stack (and consequently must depend on memory allocation success). May be called without constructing an exit frame and don't need checks the return value.
2) That may fail but does not access the stack frame (consequently don't perform GC). In cace of memory allocation fault may be recalled after GC. Don't need an exit frame but need error handling and recovery code branch.
3) That nees iterate through the stack or be able to perform GC. Called through an exit frame.

This CL adds funtions of the first category in ia32.

[Vitaly Repeshko, 2010-02-12 11:36:47]

FYI.

http://codereview.chromium.org/598072/diff/4001/5003
File src/ia32/macro-assembler-ia32.cc (right):

http://codereview.chromium.org/598072/diff/4001/5003#newcode1155
src/ia32/macro-assembler-ia32.cc:1155:
call(FUNCTION_ADDR(ExternalReference(f).address()), RelocInfo::RUNTIME_ENTRY);
Some ABIs (notably mac:
http://developer.apple.com/mac/library/DOCUMENTATION/DeveloperTools/Conceptua...)
require certain stack alignment before calling a C function. Exit frame does
align before calling. Technically it is not strictly required for all functions,
but in case a function uses SSE instructions or calls something from the
standard library the program will simply crash without proper alignment.

[Søren Thygesen Gjesse, 2010-02-12 12:32:24]

http://codereview.chromium.org/598072/diff/4001/5003
File src/ia32/macro-assembler-ia32.cc (right):

http://codereview.chromium.org/598072/diff/4001/5003#newcode1154
src/ia32/macro-assembler-ia32.cc:1154: // JS caller saved are: eax, ecx, edx,
ebx, edi (superset of C++)
On 64-bit we have the functions

  ArgumentStackSlotsForCFunctionCall
  PrepareCallCFunction
  CallCfunction

in the macro assembler, which abstracts the platform differences of calling a C
function. It is currently used by the RegExp (regexp-macro-assembler-x64.cc). It
might be an idea to introduce something like that on 32-bit as well. In
regexp-macro-assembler-ia32.cc there are local functions FrameAlign and
CallCFunction for handling that. This should probably also be used in
RegExpExecStub::Generate as this also makes a C call (but not to a real C
function, so the frame alignment here is not strictly required).

Maybe abstracting it even more by making a stack allocated structure, e.g.

  CFunctionCall ccall(argc);
  ccall.PrepareStack();
  ...
  ccall.Call(register);

maybe not...

[SeRya, 2010-02-18 16:27:37]

- Added stack alignment.
- Mode functions are called without CEntryStub.
- Added x64 implementation (arm is still not implemented).

[Mads Ager (chromium), 2010-02-19 10:49:57]

Please cut this down to the minimal possible change.  Do not attempt to handle
three different variants of runtime calls in one change.  Just split them into
two at first and then add another category later if needed.

Also, there seems to be a renaming change in here as well in a test.  Could you
split that out as a separate change?

First batch of comments below.  I'll have a look again when this has been
cleaned up to only contain a subset of this.

http://codereview.chromium.org/598072/diff/8001/8009
File src/ia32/ic-ia32.cc (right):

http://codereview.chromium.org/598072/diff/8001/8009#newcode628
src/ia32/ic-ia32.cc:628: IC_Utility(kKeyedLoadPropertyWithInterceptor)), 2, 1);
I think this is easier to read:

__ TailCallExternalReference(
    ExternalReference(IC_Utility(kKeyedLoadPropertyWithInterceptor)),
    2,
    1);

http://codereview.chromium.org/598072/diff/8001/8009#newcode1280
src/ia32/ic-ia32.cc:1280: 2, 1);
Align the arguments with the other argument to TailCallExternalReference:

  __ TailCallExternalReference(ExternalReference(IC_Utility(kLoadIC_Miss)),
                               2,
                               1);

http://codereview.chromium.org/598072/diff/8001/8009#newcode1395
src/ia32/ic-ia32.cc:1395: __
TailCallExternalReference(ExternalReference(IC_Utility(
Identation as above.

http://codereview.chromium.org/598072/diff/8001/8009#newcode1450
src/ia32/ic-ia32.cc:1450: __
TailCallExternalReference(ExternalReference(IC_Utility(kStoreIC_Miss)),
Identation.

http://codereview.chromium.org/598072/diff/8001/8009#newcode1492
src/ia32/ic-ia32.cc:1492: __
TailCallExternalReference(ExternalReference(IC_Utility(
Indentation.

http://codereview.chromium.org/598072/diff/8001/8003
File src/ia32/macro-assembler-ia32.cc (right):

http://codereview.chromium.org/598072/diff/8001/8003#newcode1163
src/ia32/macro-assembler-ia32.cc:1163: void
MacroAssembler::DirectCallRuntime(Runtime::Function* f, bool tail) {
When a method in the macro-assembler handles both calls and jumps (tail-calls),
we use the convention that it is called InvokeXYZ and that it takes an
InvokeFlag parameter which is either CALL_FUNCTION or JUMP_FUNCTION.

So, please call this one DirectInvokeRuntime or something like that and replace
the bool with an InvokeFlag parameter.

http://codereview.chromium.org/598072/diff/8001/8003#newcode1165
src/ia32/macro-assembler-ia32.cc:1165: //   ag1
ag1 -> arg1

http://codereview.chromium.org/598072/diff/8001/8003#newcode1267
src/ia32/macro-assembler-ia32.cc:1267: int num_arguments,
Indentation.

http://codereview.chromium.org/598072/diff/8001/8004
File src/ia32/macro-assembler-ia32.h (right):

http://codereview.chromium.org/598072/diff/8001/8004#newcode377
src/ia32/macro-assembler-ia32.h:377: int num_arguments,
Align with the other arguments.

http://codereview.chromium.org/598072/diff/8001/8005
File src/runtime.cc (right):

http://codereview.chromium.org/598072/diff/8001/8005#newcode52
src/runtime.cc:52: #if V8_TARGET_ARCH_IA32
Get rid of these defines.

http://codereview.chromium.org/598072/diff/8001/8005#newcode59
src/runtime.cc:59: class NotFailsTag {};
Why do you need this?

http://codereview.chromium.org/598072/diff/8001/8005#newcode8273
src/runtime.cc:8273: // Utilities for building the runtime function list
I would like to get rid of all of this.  Can we split up the C++ runtime
functions in a number of lists (macros) instead and get rid of all this?

http://codereview.chromium.org/598072/diff/8001/8006
File src/runtime.h (right):

http://codereview.chromium.org/598072/diff/8001/8006#newcode372
src/runtime.h:372: // and collect the garbage.
collect the garbage -> perform garbage collection.

http://codereview.chromium.org/598072/diff/8001/8006#newcode379
src/runtime.h:379: // The Callee always returns a valid object
Callee -> callee

Please end comment with period.

http://codereview.chromium.org/598072/diff/8001/8011
File src/x64/macro-assembler-x64.cc (right):

http://codereview.chromium.org/598072/diff/8001/8011#newcode384
src/x64/macro-assembler-x64.cc:384: == f->calling_convention) ? 0 :
num_arguments;
Can we indent this differently please?

[Søren Thygesen Gjesse, 2010-02-19 12:13:29]

http://codereview.chromium.org/598072/diff/8001/8003
File src/ia32/macro-assembler-ia32.cc (right):

http://codereview.chromium.org/598072/diff/8001/8003#newcode1168
src/ia32/macro-assembler-ia32.cc:1168: //   ret addr (if tail call)
I suggest encapsulating this in PrepareCallCFunction/CallCFunction in the macro
assembler as on x64 and use these in the RegExp macro assembler. That could be
done as a separate commit before this.

http://codereview.chromium.org/598072/diff/8001/8003#newcode1211
src/ia32/macro-assembler-ia32.cc:1211:
call(FUNCTION_ADDR(ExternalReference(f).address()), RelocInfo::RUNTIME_ENTRY);
Please add a comment saying that we are only performing a semi tail call as we
need to copy the arguments for alignment. On Windows 32-bit we could consider
cheating as the 8 byte alignment set in platform-win is not strictly required.
We could measure this after this is done.

http://codereview.chromium.org/598072/diff/8001/8004
File src/ia32/macro-assembler-ia32.h (right):

http://codereview.chromium.org/598072/diff/8001/8004#newcode369
src/ia32/macro-assembler-ia32.h:369: // Tail call of a runtime routine (jump).
This comment needs to be changed as well.

http://codereview.chromium.org/598072/diff/8001/8011
File src/x64/macro-assembler-x64.cc (right):

http://codereview.chromium.org/598072/diff/8001/8011#newcode405
src/x64/macro-assembler-x64.cc:405: // A expected to be put into the registers
in order of Register
This comment seems a bit broken.

http://codereview.chromium.org/598072/diff/8001/8013
File src/x64/macro-assembler-x64.h (right):

http://codereview.chromium.org/598072/diff/8001/8013#newcode44
src/x64/macro-assembler-x64.h:44: static const RegList kCCallerSaved =
Can't we use Register.bit here (e.g. rax.bit())?

http://codereview.chromium.org/598072/diff/8001/8012
File src/x64/virtual-frame-x64.cc (right):

http://codereview.chromium.org/598072/diff/8001/8012#newcode982
src/x64/virtual-frame-x64.cc:982: Forget(arg_count);
On ia32 this threw and exception?

http://codereview.chromium.org/598072/diff/8001/8012#newcode985
src/x64/virtual-frame-x64.cc:985: 
This is more or less a duplication of what is in macro assembler CallRuntime.
Why is that? I think you should start by just spilling all registers. And then
optimizing for calee saved registers later.

[SeRya, 2010-02-19 18:11:38]

http://codereview.chromium.org/598072/diff/8001/8009
File src/ia32/ic-ia32.cc (right):

http://codereview.chromium.org/598072/diff/8001/8009#newcode628
src/ia32/ic-ia32.cc:628: IC_Utility(kKeyedLoadPropertyWithInterceptor)), 2, 1);
On 2010/02/19 10:49:57, Mads Ager wrote:
> I think this is easier to read:
> 
> __ TailCallExternalReference(
>     ExternalReference(IC_Utility(kKeyedLoadPropertyWithInterceptor)),
>     2,
>     1);

I introduced a var to make this line shorter (like in some other places of
calling TailCallExternalReference).

http://codereview.chromium.org/598072/diff/8001/8009#newcode1280
src/ia32/ic-ia32.cc:1280: 2, 1);
On 2010/02/19 10:49:57, Mads Ager wrote:
> Align the arguments with the other argument to TailCallExternalReference:
> 
>   __ TailCallExternalReference(ExternalReference(IC_Utility(kLoadIC_Miss)),
>                                2,
>                                1);

As above

http://codereview.chromium.org/598072/diff/8001/8009#newcode1395
src/ia32/ic-ia32.cc:1395: __
TailCallExternalReference(ExternalReference(IC_Utility(
On 2010/02/19 10:49:57, Mads Ager wrote:
> Identation as above.

Done.

http://codereview.chromium.org/598072/diff/8001/8009#newcode1450
src/ia32/ic-ia32.cc:1450: __
TailCallExternalReference(ExternalReference(IC_Utility(kStoreIC_Miss)),
On 2010/02/19 10:49:57, Mads Ager wrote:
> Identation.

Done.

http://codereview.chromium.org/598072/diff/8001/8009#newcode1492
src/ia32/ic-ia32.cc:1492: __
TailCallExternalReference(ExternalReference(IC_Utility(
On 2010/02/19 10:49:57, Mads Ager wrote:
> Indentation.

Done.

http://codereview.chromium.org/598072/diff/8001/8003
File src/ia32/macro-assembler-ia32.cc (right):

http://codereview.chromium.org/598072/diff/8001/8003#newcode1163
src/ia32/macro-assembler-ia32.cc:1163: void
MacroAssembler::DirectCallRuntime(Runtime::Function* f, bool tail) {
On 2010/02/19 10:49:57, Mads Ager wrote:
> When a method in the macro-assembler handles both calls and jumps
(tail-calls),
> we use the convention that it is called InvokeXYZ and that it takes an
> InvokeFlag parameter which is either CALL_FUNCTION or JUMP_FUNCTION.
> 
> So, please call this one DirectInvokeRuntime or something like that and
replace
> the bool with an InvokeFlag parameter. 

Done.

http://codereview.chromium.org/598072/diff/8001/8003#newcode1165
src/ia32/macro-assembler-ia32.cc:1165: //   ag1
On 2010/02/19 10:49:57, Mads Ager wrote:
> ag1 -> arg1

Done.

http://codereview.chromium.org/598072/diff/8001/8003#newcode1168
src/ia32/macro-assembler-ia32.cc:1168: //   ret addr (if tail call)
On 2010/02/19 12:13:29, Søren Gjesse wrote:
> I suggest encapsulating this in PrepareCallCFunction/CallCFunction in the
macro
> assembler as on x64 and use these in the RegExp macro assembler. That could be
> done as a separate commit before this.

This code behaves differenly from x64's CallCFunction. However I'm planning
further optimization for ia32 what apparently will allow to extract common code
for CallCFunction.

http://codereview.chromium.org/598072/diff/8001/8003#newcode1211
src/ia32/macro-assembler-ia32.cc:1211:
call(FUNCTION_ADDR(ExternalReference(f).address()), RelocInfo::RUNTIME_ENTRY);
On 2010/02/19 12:13:29, Søren Gjesse wrote:
> Please add a comment saying that we are only performing a semi tail call as we
> need to copy the arguments for alignment. On Windows 32-bit we could consider
> cheating as the 8 byte alignment set in platform-win is not strictly required.
> We could measure this after this is done.

Done.

http://codereview.chromium.org/598072/diff/8001/8003#newcode1267
src/ia32/macro-assembler-ia32.cc:1267: int num_arguments,
On 2010/02/19 10:49:57, Mads Ager wrote:
> Indentation.

Done.

http://codereview.chromium.org/598072/diff/8001/8004
File src/ia32/macro-assembler-ia32.h (right):

http://codereview.chromium.org/598072/diff/8001/8004#newcode369
src/ia32/macro-assembler-ia32.h:369: // Tail call of a runtime routine (jump).
On 2010/02/19 12:13:29, Søren Gjesse wrote:
> This comment needs to be changed as well.

Done.

http://codereview.chromium.org/598072/diff/8001/8004#newcode377
src/ia32/macro-assembler-ia32.h:377: int num_arguments,
On 2010/02/19 10:49:57, Mads Ager wrote:
> Align with the other arguments.

Done.

http://codereview.chromium.org/598072/diff/8001/8005
File src/runtime.cc (right):

http://codereview.chromium.org/598072/diff/8001/8005#newcode52
src/runtime.cc:52: #if V8_TARGET_ARCH_IA32
On 2010/02/19 10:49:57, Mads Ager wrote:
> Get rid of these defines.

Changed to:
#if defined(V8_TARGET_ARCH_X64) || defined(V8_TARGET_ARCH_IA32)
#define DIRECT_CALL_SUPPORTED
#endif

Did you mean that?

http://codereview.chromium.org/598072/diff/8001/8005#newcode59
src/runtime.cc:59: class NotFailsTag {};
On 2010/02/19 10:49:57, Mads Ager wrote:
> Why do you need this?

I supposed to use it to differentiate function what never fails from that could
fail and need to be reties. Since code for function with retry has removed it's
not needed anymore. Deleted.

http://codereview.chromium.org/598072/diff/8001/8005#newcode8273
src/runtime.cc:8273: // Utilities for building the runtime function list
On 2010/02/19 10:49:57, Mads Ager wrote:
> I would like to get rid of all of this.  Can we split up the C++ runtime
> functions in a number of lists (macros) instead and get rid of all this? 

It make easy to switch calling convention for a specific function. I'd like to
keep it for now. But since calling convention support will be implemented on all
architectures it could be done.

Alternatively we could use it for derivation all the function traits (number of
arguments and number of return values) from their signatures simply making the
Argument class be a template parametrized by number of arguments (or even their
types).

http://codereview.chromium.org/598072/diff/8001/8006
File src/runtime.h (right):

http://codereview.chromium.org/598072/diff/8001/8006#newcode372
src/runtime.h:372: // and collect the garbage.
On 2010/02/19 10:49:57, Mads Ager wrote:
> collect the garbage -> perform garbage collection.

Done.

http://codereview.chromium.org/598072/diff/8001/8006#newcode379
src/runtime.h:379: // The Callee always returns a valid object
On 2010/02/19 10:49:57, Mads Ager wrote:
> Callee -> callee
> 
> Please end comment with period.

Done.

http://codereview.chromium.org/598072/diff/8001/8011
File src/x64/macro-assembler-x64.cc (right):

http://codereview.chromium.org/598072/diff/8001/8011#newcode384
src/x64/macro-assembler-x64.cc:384: == f->calling_convention) ? 0 :
num_arguments;
On 2010/02/19 10:49:57, Mads Ager wrote:
> Can we indent this differently please?

Done.

http://codereview.chromium.org/598072/diff/8001/8011#newcode405
src/x64/macro-assembler-x64.cc:405: // A expected to be put into the registers
in order of Register
On 2010/02/19 12:13:29, Søren Gjesse wrote:
> This comment seems a bit broken.

Done.

http://codereview.chromium.org/598072/diff/8001/8013
File src/x64/macro-assembler-x64.h (right):

http://codereview.chromium.org/598072/diff/8001/8013#newcode44
src/x64/macro-assembler-x64.h:44: static const RegList kCCallerSaved =
On 2010/02/19 12:13:29, Søren Gjesse wrote:
> Can't we use Register.bit here (e.g. rax.bit())?

Done.

http://codereview.chromium.org/598072/diff/8001/8012
File src/x64/virtual-frame-x64.cc (right):

http://codereview.chromium.org/598072/diff/8001/8012#newcode982
src/x64/virtual-frame-x64.cc:982: Forget(arg_count);
On 2010/02/19 12:13:29, Søren Gjesse wrote:
> On ia32 this threw and exception?

Not, just returns undefined value:
void MacroAssembler::IllegalOperation(int num_arguments) {
  // ...
  mov(eax, Immediate(Factory::undefined_value()));
}

http://codereview.chromium.org/598072/diff/8001/8012#newcode985
src/x64/virtual-frame-x64.cc:985: 
On 2010/02/19 12:13:29, Søren Gjesse wrote:
> This is more or less a duplication of what is in macro assembler CallRuntime.
> Why is that?

If you talk about checking parameters number it is duplication. I wouldn't like
to complicate the code supporting number of parameters not- fitting in registers
(any existing now candidate to be called directly has not more than 3
parameters). Limiting number of parameters by 4 would simplify code on ia32
(partially), x64 and arm.

May be we should just put ASSERT here?

> I think you should start by just spilling all registers. And then
> optimizing for calee saved registers later.

Actually I did it for ia32 (I home to come up with passing args in registers, at
least first 2). But in case of x64 this optimization looks straightforward. What
use of spilling all registers here?

[Søren Thygesen Gjesse, 2010-02-22 12:38:22]

We would like this to be split into smaller changes in these steps:

1. Changing TailCallRuntime to TailCallExternalReference can go in a separate
change

2. Add PrepareCallCFunction/CallCFunction to macro-assembler-ia32 and use it in
regexp-macro-assembler-ia32.cc

3. Direct call support on ia32 only and only enabled for one or two of the
hottest runtime functions suitable and make it just simply pass all arguments on
the stack (standard calling convention).

Don't use the SIGNATURE_XXX macros, instead add the calling convention to the
functions lists in runtime.h, e.g.

  F(GetProperty, 2, 1, EXIT_FRAME_CALL) \
  F(KeyedGetProperty, 2, 1, EXIT_FRAME_CALL) \
  ...

Then add a the list RUNTIME_FUNCTION_LIST_DIRECT_CALL:

#ifdef V8_TARGET_ARCH_IA32
#define RUNTIME_FUNCTION_LIST_DIRECT_CALL(F) \
  F(XXX, 2, 1, DIRECT_CALL)
#endif

#ifdef V8_TARGET_ARCH_X64
#define RUNTIME_FUNCTION_LIST_DIRECT_CALL(F) \
  F(XXX, 2, 1, EXIT_FRAME_CALL)
#endif

#ifdef V8_TARGET_ARCH_ARM
#define RUNTIME_FUNCTION_LIST_DIRECT_CALL(F) \
  F(XXX, 2, 1, EXIT_FRAME_CALL)
#endif

and remove XXX from RUNTIME_FUNCTION_LIST_ALWAYS_X. Add the new list:

#define RUNTIME_FUNCTION_LIST(F) \
  RUNTIME_FUNCTION_LIST_ALWAYS_1(F) \
  RUNTIME_FUNCTION_LIST_ALWAYS_2(F) \
  RUNTIME_FUNCTION_LIST_DIRECT_CALL(F) \
  RUNTIME_FUNCTION_LIST_DEBUG(F) \
  RUNTIME_FUNCTION_LIST_DEBUGGER_SUPPORT(F) \
  RUNTIME_FUNCTION_LIST_PROFILER_SUPPORT(F)

In runtime.cc have two "duplicate" versions of Runtime_XXX:

#ifdef DIRECT_CALL_SUPPORTED

static Object* Runtime_XXX(Object* x, Object* y) {
  // Full body.
}

#else

static Object* Runtime_XXX(Arguments args) {
  // Full body.
}

#endif


4. Port direct call to x64

5. Port direct call to ARM

6. Extend the number of runtime functions for which direct call is supported.

7. Add more advanced optimizations.

8. Goto 7 :-)

http://codereview.chromium.org/598072/diff/11034/10043
File src/ia32/macro-assembler-ia32.cc (right):

http://codereview.chromium.org/598072/diff/11034/10043#newcode1171
src/ia32/macro-assembler-ia32.cc:1171: 
I still think we should have the handling of calling a C function directly
abstracted into a PrepareCallCFunction function and retire FrameAlign etc. in
regexp-macro-assembler-ia32.cc.

As far as I can see you optimize the removal of the existing arguments into
restoring esp, and use push instead of
  mov(xxx, Operand(eax, Y * kPointerSize);
  mov(Operand(esp, X * kPointerSize), xxx);
I don't know whether it makes a performance difference, but if it does the
PrepareCallCFunction could take parameters to indicate whether the stack should
be prepared for push or storing into and number additional stack slots to get
rid of when restoring after alignment.

http://codereview.chromium.org/598072/diff/11034/10043#newcode1218
src/ia32/macro-assembler-ia32.cc:1218: }
Maybe just adjust the stack by f->nargs in an else to keep things simpler.
Otherwise please comment on then mov(esp, ... removes the arguments.

http://codereview.chromium.org/598072/diff/11034/10048
File src/runtime.h (right):

http://codereview.chromium.org/598072/diff/11034/10048#newcode376
src/runtime.h:376: DIRECT_CALL_NOT_FAILS
NOT -> NEVER.