Closed Bug 708441 Opened 13 years ago Closed 12 years ago

IonMonkey: Call uncompiled and native functions.

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: sstangl, Unassigned)

References

Details

Attachments

(1 file, 2 obsolete files)

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
Nicolas: Why does this depend on JSOP_GETPROP? That doesn't seem right.
This handles calling native and uncompiled functions in visitCallGeneric() by routing both to js::Invoke().

Blocked on VMFunction changes, but otherwise ready.
Attachment #579865 - Attachment is obsolete: true
Summary: IonMonkey: Call uncompiled functions. → IonMonkey: Call uncompiled and native functions.
Lacks ARM support -- I will flag mjrosenb separately.
Attachment #581070 - Flags: review?(dvander)
Attachment #580242 - Attachment is obsolete: true
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.
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+
(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;
Blocks: 710948
ARM port is implicit in bug 710948.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.