708441 - IonMonkey: Call uncompiled and native functions.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Sean Stangl [:sstangl]]

Attachment: WIP: Handle uncompiled functions (interpret only).

With VMFunctions implemented, we can call out to C++ helper routines. This lets us finally augment visitCallGeneric() to call out to the interpreter for handling functions that are not yet JITted, which are probably the vast majority.

The attached patch just requests interpretation in the case of uncompiled functions. This will be shortly changed to attempt compilation, then run the Ion code if compilation succeeded, with interpretation only occurring upon compilation failure.

Passes all tests, but isn't pretty.

Comment 1

[Sean Stangl [:sstangl]]

Nicolas: Why does this depend on JSOP_GETPROP? That doesn't seem right.

Comment 2

[Sean Stangl [:sstangl]]

Attachment: WIP: Call native/uncompiled functions {x86,x64}.

This handles calling native and uncompiled functions in visitCallGeneric() by routing both to js::Invoke().

Blocked on VMFunction changes, but otherwise ready.

Comment 3

[Sean Stangl [:sstangl]]

Attachment: Support calling uncompiled and native functions on the slow path.

Lacks ARM support -- I will flag mjrosenb separately.

Comment 4

[Sean Stangl [:sstangl]]

As an extra note, I'll rename IonVMFunctions.h -> VMFunctions.h along with this patch. The renaming isn't included in the patch because of local idiosyncrasies involving git.

Comment 5

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 581070 [details] [diff] [review]
Support calling uncompiled and native functions on the slow path.

Review of attachment 581070 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/ion/IonVMFunctions.h
@@ +140,5 @@
> +    static inline DataType outParam() {
> +        return OutParamToDataType<A4>::result;
> +    }
> +    static inline size_t explicitArgs() {
> +        return 4 - (outParam() != Type_Void ? 1 : 0);

Nicolas pointed out that explicitArgs() should just return 4 here (and 2 earlier).

::: js/src/ion/shared/CodeGenerator-x86-shared.cpp
@@ +766,4 @@
>  CodeGeneratorX86Shared::visitNewArray(LNewArray *ins)
>  {
>      static const VMFunction NewInitArrayInfo =
> +        FunctionInfo3<JSObject *, uint32, types::TypeObject *, NewInitArray>();

Why do we need the "3" here? I think the compiler should be able to pick the right specialization.

@@ +827,5 @@
> +    masm.branchPtr(Assembler::BelowOrEqual, objreg, ImmWord(ION_DISABLED_SCRIPT), &invoke);
> +    // Forward unconditional (likely) branch to handle compiled code.
> +    masm.jmp(&compiled);
> +    {
> +        masm.bind(&invoke);

An out-of-line path would probably be nicer here but it's fine as-is.

::: js/src/ion/x64/Trampoline-x64.cpp
@@ +461,4 @@
>      if (f.outParam == Type_Value) {
>          outReg = regs.takeAny();
>          masm.reserveStack(sizeof(Value));
> +        masm.movq(rsp, outReg);

Heh. Ouch. Maybe we should encourage movl/movq(reg, reg) in favor of just mov.

Comment 6

[Sean Stangl [:sstangl]]

(In reply to David Anderson [:dvander] from comment #5)
> Nicolas pointed out that explicitArgs() should just return 4 here (and 2
> earlier).

That will cause a valid assertion. (I checked; it does.) "explicitArgs()" is meant to return the number of arguments that are not-JSContext and not an outparam. The code only worked previously because we never tested CallVM using an outparam, and the old calculation (2 + outparam?1) just happened by chance to produce a valid result.
 
> ::: js/src/ion/shared/CodeGenerator-x86-shared.cpp
> @@ +766,4 @@
> >  CodeGeneratorX86Shared::visitNewArray(LNewArray *ins)
> >  {
> >      static const VMFunction NewInitArrayInfo =
> > +        FunctionInfo3<JSObject *, uint32, types::TypeObject *, NewInitArray>();
> 
> Why do we need the "3" here? I think the compiler should be able to pick the
> right specialization.

I think so too! Unfortunately, the compiler does not concur, and doesn't want to overload templates in that manner, considering the second definition an invalid redeclaration.

I'll leave FunctionInfoN for the moment -- if we're feeling particularly clever in the future, perhaps nesting templates would work.

> Heh. Ouch. Maybe we should encourage movl/movq(reg, reg) in favor of just
> mov.

Yeah. In AT&T style, "mov" implies 16-bit anyway.

Comment 7

[David Anderson [:dvander] - inactive, e-mail if emergency]

> That will cause a valid assertion. (I checked; it does.) "explicitArgs()" is
> meant to return the number of arguments that are not-JSContext and not an
> outparam. The code only worked previously because we never tested CallVM
> using an outparam, and the old calculation (2 + outparam?1) just happened by
> chance to produce a valid result.

Ah, right - it should really be (outParam() == Type_Void ? 1 : 0) - it's the number of arguments you must explicitly push in the code generator.

> I think so too! Unfortunately, the compiler does not concur, and doesn't
> want to overload templates in that manner, considering the second definition
> an invalid redeclaration.

Do we need a base version? Something like...

template <class> struct FunctionInfo;

Comment 8

[Sean Stangl [:sstangl]]

http://hg.mozilla.org/projects/ionmonkey/rev/5ee070c3f2d0

Still needs an ARM port.

Comment 9

[Sean Stangl [:sstangl]]

ARM port is implicit in bug 710948.