Closed Bug 735400 Opened 12 years ago Closed 12 years ago

IonMonkey: Optimize JSOP_FUNCALL

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: dvander, Assigned: sstangl)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Needed for deltablue and earley-boyer, I think. Ideally we want:

    f.call(x, ...)

To become:

    f(...)   ; where this = x

We should be able to inline through in the best case (I think JM+TI does this), and for other cases fall back to a transformation to MCall.
Assignee: general → sstangl
We already support JSOP_FUNCALL as described. The hard case for JSOP_THIS isn't handled, but that is a separate bug.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → WORKSFORME
I've been tricked. Dvander pointed out that we don't really handle JSOP_FUNCALL at all -- we go through a native. Reopening.
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
This implements JSOP_FUNCALL optimizations without inlining. The normal call mechanism is reused by shifting the slots down by one, overwriting the native, and unwrapping the MPassArg(JSFunction *).

Unfortunately, logic to inline functions queries TI for safety/argument information, and with the transformation of f.call(...) -> f(...), the types returned by TI no longer correspond to the MIR being built.

The obvious suggestion is to collect TI information before shimmying the slots down, but native inlining needs access to arbitrary many TypeSets. Another option is to have poppedTypes() check whether pc == JSOP_FUNCALL in certain situations, and if so, actually check one slot higher. Doesn't sound pleasant.
Comment on attachment 609518 [details] [diff] [review]
JSOP_FUNCALL without inlining.

JM doesn't inline, so it's probably not awful for us to also not inline.

v8-v6 deltablue goes ~500ms -> ~480ms on local x86; all other tests unaffected.
Attachment #609518 - Flags: review?(dvander)
Comment on attachment 609518 [details] [diff] [review]
JSOP_FUNCALL without inlining.

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

::: js/src/ion/IonBuilder.cpp
@@ +125,5 @@
>      return state;
>  }
>  
> +JSObject *
> +IonBuilder::getSingleObject(types::TypeSet *types)

nit: This looks like it could be a static, instead of a member function (maybe not even declared in IonBuilder.h)

@@ +129,5 @@
> +IonBuilder::getSingleObject(types::TypeSet *types)
> +{
> +    if (!types || types->unknownObject() || types->getObjectCount() != 1)
> +        return NULL;
> +    return types->getSingleObject(0);

I think this can just be:
    if (!types)
        return NULL;
    return types->getSingleton();

@@ +2566,5 @@
> +    if (!native || !native->isNative() || native->native() != &js_fun_call)
> +        return makeCall(native, argc, false);
> +
> +    // Extract call target.
> +    JSObject *funobj   = getSingleObject(oracle->getCallArg(script, argc, 0, pc));

Are we guaranteed a non-null funobj here?

::: js/src/ion/MIRGraph.h
@@ +142,5 @@
>      // not be used under most circumstances.
>      void rewriteSlot(uint32 slot, MDefinition *ins);
>  
> +    // Rewrites a slot based on its current depth (same as argument to peek()).
> +    void rewriteDepth(int32 depth, MDefinition *ins);

Maybe "rewriteAtDepth" would be a better name.
Attachment #609518 - Flags: review?(dvander) → review+
https://hg.mozilla.org/projects/ionmonkey/rev/8a2010ae3d08
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
Depends on: 755194
Depends on: 755832
Depends on: 776359
You need to log in before you can comment on or make changes to this bug.