IonMonkey: Call uncompiled and native functions.

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: sstangl, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

6 years ago
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.
Depends on: 701958
(Reporter)

Comment 1

6 years ago
Nicolas: Why does this depend on JSOP_GETPROP? That doesn't seem right.
(Reporter)

Comment 2

6 years ago
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.
(Reporter)

Updated

6 years ago
Attachment #579865 - Attachment is obsolete: true
(Reporter)

Updated

6 years ago
Summary: IonMonkey: Call uncompiled functions. → IonMonkey: Call uncompiled and native functions.
(Reporter)

Comment 3

5 years ago
Created attachment 581070 [details] [diff] [review]
Support calling uncompiled and native functions on the slow path.

Lacks ARM support -- I will flag mjrosenb separately.
Attachment #581070 - Flags: review?(dvander)
(Reporter)

Updated

5 years ago
Attachment #580242 - Attachment is obsolete: true
(Reporter)

Comment 4

5 years ago
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.
(Reporter)

Updated

5 years ago
Blocks: 685097
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.
Attachment #581070 - Flags: review?(dvander) → review+
(Reporter)

Comment 6

5 years ago
(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.
> 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;
(Reporter)

Comment 8

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

Still needs an ARM port.
(Reporter)

Updated

5 years ago
Blocks: 710948
(Reporter)

Comment 9

5 years ago
ARM port is implicit in bug 710948.
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.