Last Comment Bug 735400 - IonMonkey: Optimize JSOP_FUNCALL
: IonMonkey: Optimize JSOP_FUNCALL
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: Sean Stangl [:sstangl]
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on: 755194 755832 776359
Blocks: IonSpeed
  Show dependency treegraph
 
Reported: 2012-03-13 12:43 PDT by David Anderson [:dvander]
Modified: 2012-07-22 08:17 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
JSOP_FUNCALL without inlining. (7.84 KB, patch)
2012-03-26 15:52 PDT, Sean Stangl [:sstangl]
dvander: review+
Details | Diff | Splinter Review

Description David Anderson [:dvander] 2012-03-13 12:43:32 PDT
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.
Comment 1 Sean Stangl [:sstangl] 2012-03-14 15:54:09 PDT
We already support JSOP_FUNCALL as described. The hard case for JSOP_THIS isn't handled, but that is a separate bug.
Comment 2 Sean Stangl [:sstangl] 2012-03-22 14:58:16 PDT
I've been tricked. Dvander pointed out that we don't really handle JSOP_FUNCALL at all -- we go through a native. Reopening.
Comment 3 Sean Stangl [:sstangl] 2012-03-26 15:52:08 PDT
Created attachment 609518 [details] [diff] [review]
JSOP_FUNCALL without inlining.

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 4 Sean Stangl [:sstangl] 2012-03-26 16:32:12 PDT
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.
Comment 5 David Anderson [:dvander] 2012-03-26 18:31:40 PDT
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.
Comment 6 Sean Stangl [:sstangl] 2012-03-27 12:22:02 PDT
https://hg.mozilla.org/projects/ionmonkey/rev/8a2010ae3d08

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