Last Comment Bug 708441 - IonMonkey: Call uncompiled and native functions.
: IonMonkey: Call uncompiled and native functions.
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: general
:
Mentors:
Depends on: 694169 701958 707845
Blocks: 685097 710948
  Show dependency treegraph
 
Reported: 2011-12-07 15:17 PST by Sean Stangl [:sstangl]
Modified: 2011-12-14 18:04 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP: Handle uncompiled functions (interpret only). (4.11 KB, patch)
2011-12-07 15:17 PST, Sean Stangl [:sstangl]
no flags Details | Diff | Review
WIP: Call native/uncompiled functions {x86,x64}. (7.00 KB, patch)
2011-12-08 16:44 PST, Sean Stangl [:sstangl]
no flags Details | Diff | Review
Support calling uncompiled and native functions on the slow path. (14.37 KB, patch)
2011-12-12 15:11 PST, Sean Stangl [:sstangl]
dvander: review+
Details | Diff | Review

Description Sean Stangl [:sstangl] 2011-12-07 15:17:50 PST
Created attachment 579865 [details] [diff] [review]
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] 2011-12-08 16:42:49 PST
Nicolas: Why does this depend on JSOP_GETPROP? That doesn't seem right.
Comment 2 Sean Stangl [:sstangl] 2011-12-08 16:44:29 PST
Created attachment 580242 [details] [diff] [review]
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] 2011-12-12 15:11:08 PST
Created attachment 581070 [details] [diff] [review]
Support calling uncompiled and native functions on the slow path.

Lacks ARM support -- I will flag mjrosenb separately.
Comment 4 Sean Stangl [:sstangl] 2011-12-12 15:23:44 PST
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] 2011-12-12 18:37:57 PST
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] 2011-12-13 13:03:39 PST
(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] 2011-12-13 13:15:19 PST
> 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] 2011-12-13 13:41:54 PST
http://hg.mozilla.org/projects/ionmonkey/rev/5ee070c3f2d0

Still needs an ARM port.
Comment 9 Sean Stangl [:sstangl] 2011-12-14 18:04:06 PST
ARM port is implicit in bug 710948.

Note You need to log in before you can comment on or make changes to this bug.