692838 - Factor stub calls with the interpreter.

Status: RESOLVED WONTFIX

(Core :: JavaScript Engine, defect)

Description

[Nicolas B. Pierron [:nbp]]

The interpreter currently implements many operations which are duplicated inside the JITs as stub calls.  We should factor the interpreter body and the stub calls.

The plan is to base IonMonkey on stub calls extracted from the interpreter.  Examples of similarities have been highlighted by Bug 685838 (JSOP_NEWARRAY) and Bug 691340 (JSOP_INITELEM).

(In reply to David Anderson [:dvander] from Bug 685838 comment #5)
> Review of attachment 563376 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/ion/IonStubCalls.cpp
> @@ +59,5 @@
> > +    if (!obj)
> > +        return MagicValue(JS_GENERIC_ERROR);
> > +
> > +    return ObjectValue(*obj);
> > +}
> 
> It's exciting to finally have JIT C++ calls that don't obey some extremely
> weird convention. But we've already run into the problem that has plagued
> JM: stub calls often don't have parity with the interpreter (here, we need
> to get a TypeObject), and in general as the interpreter evolves on m-c the
> ionmonkey branch will get left behind.
> 
> > // :TODO: (Bug ???) Merge this file with jsinterp.cpp and
> 
> After talking with Luke, this is definitely the right idea. Let's just do it
> now :) and in fact, let's do it on m-c! In jsinterp.h/cpp we could have:
> 
> JSObject *NewInitArray(JSContext *cx, uint32 count, types::TypeSet *types);
> 
> In the interpreter, JSOP_NEWARRAY would become:
>     TypeObject *type = TypeScript::InitObject(cx, script, regs.pc,
> JSProto_Array);
>     if (!type)
>         ...
>     JSObject *obj = NewInitArray(cx, GET_UINT24(regs.pc), types);
>     if (!obj...
> 
> Similarly, methodjit/Stubcalls.cpp stubs::NewInitArray would be simplified.
> Now, the Ion version can just call into jsinterp -- we can compute the
> TypeObject during MIR building, attaching to the MNewArray. Then this
> pointer can be hardcoded in the code generator:
>    masm.push(lir->mir()->count());
>    masm.push(lir->mir()->typeObject());
> 
> If we land this minor refactoring on mozilla-central, then we may never need
> IonStubCalls.cpp and we can put off parity problems.
>

Comment 1

[Tom S [:evilpie]]

I like the idea of unify all or stubs with the interpreter. So how would the new interpret look like? Would we take them |regs| ?

Comment 2

[Nicolas B. Pierron [:nbp]]

(In reply to Tom Schuster (evilpie) from comment #1)
> I like the idea of unify all or stubs with the interpreter. So how would the
> new interpret look like? Would we take them |regs| ?

The idea would be to have function which are independent of the frame representation.  Thus the interpreter would have to extract information from the interpreter stack and from the bytecode parameters before calling the function.

(In reply to Nicolas B. Pierron [:pierron] from comment #0)
> > JSObject *NewInitArray(JSContext *cx, uint32 count, types::TypeSet *types);
> > 
> > In the interpreter, JSOP_NEWARRAY would become:
> >     TypeObject *type = TypeScript::InitObject(cx, script, regs.pc,
> > JSProto_Array);
> >     if (!type)
> >         ...
> >     JSObject *obj = NewInitArray(cx, GET_UINT24(regs.pc), types);
> >     if (!obj...

Comment 3

[Tom S [:evilpie]]

Attachment: design preview

I already implemented some simple operations. Please tell me what you think. Maybe it would be useful to abstract that even better so every implementation takes a class describing how to get input, set outputs etc.

dvander expressed interested in returning a magic js::Value to indicate errors, I think this could be done rather easy with the current design.

Comment 4

[David Mandelin [:dmandelin]]

Looks like a good start. It might be nice to get rid of the gotos someday, but that can wait.

Comment 5

[Nicolas B. Pierron [:nbp]]

Comment on attachment 569676 [details] [diff] [review]
design preview

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

::: js/src/vm/InterpreterStubs.h
@@ +42,5 @@
> +namespace js {
> +namespace op {
> +
> +bool
> +Add(JSContext *cx, JSScript *script, jsbytecode *pc, Value &lval, Value &rval, Value &result);

I don't think we need such stub calls for IonMonkey.  IonMonkey will produce assembly for most/all? of the operators and fallback to the interpreter if its assumptions are failling.  If you intend to factor stub calls with methodjit you should have a look at methodjit/FastArithmetic.cpp (jsop_binary).

::: js/src/vm/Unicode.h
@@ -36,5 @@
> - *
> - * ***** END LICENSE BLOCK ***** */
> -
> -#ifndef Unicode_h__
> -#define Unicode_h__

Take care to not mess up your patch queue ;)

Comment 6

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

For IonMonkey (for which this bug was originally intended) we'll need stub calls for operations like Add for when we don't know the types, or can't handle a type combination. But it's not yet clear what those signatures should look like, what can be factored, and whether all stub calls will do the same thing as the interpreter. We'll discover this incrementally as we add C++ calls.

It shouldn't hurt to do some of this in JM now, but I'd like to drop the term "stubs" (they're really normal VM functions). Luke suggested keeping everything in jsinterp.cpp.

Comment 7

[Tom S [:evilpie]]

(In reply to David Anderson [:dvander] from comment #6)
> For IonMonkey (for which this bug was originally intended) we'll need stub
> calls for operations like Add for when we don't know the types, or can't
> handle a type combination. But it's not yet clear what those signatures
> should look like, what can be factored, and whether all stub calls will do
> the same thing as the interpreter. We'll discover this incrementally as we
> add C++ calls.
>
> It shouldn't hurt to do some of this in JM now, but I'd like to drop the
> term "stubs" (they're really normal VM functions). Luke suggested keeping
> everything in jsinterp.cpp.
Really? I think this actually looks much clearer already. And it might be easier to do other refactoring, because every VM Function has a kind of standardized signature.

The only downside at the moment is that except some of the really easy ops, we often do funny stuff, like conditionally changing the sp index 

Yes Stub is a horrible name, but it's just the file name, I am open to suggestions.

Comment 8

[Nicolas B. Pierron [:nbp]]

Attachment: Dependency for JSOP_NEWARRAY & JSOP_INITELEM

Sorry if this duplicate work, but I need these functions to for JSOP_NEWARRAY (Bug 685838) and JSOP_INITELEM (Bug 691340).

Comment 9

[Tom S [:evilpie]]

Okay, i will pick this up again :O

Comment 10

[Tom S [:evilpie]]

Without the tracer we could do some simplifications.

Comment 11

[Tom S [:evilpie]]

We are doing this in a lot of ionmonkey patches progressively, so this solution isn't needed anymore.