Closed Bug 824473 Opened 9 years ago Closed 9 years ago

IonMonkey: possible inline callee function of inlined funapplyarguments

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla21

People

(Reporter: h4writer, Assigned: h4writer)

References

Details

Attachments

(3 files, 1 obsolete file)

Currently when a funapply function gets inlined we only emit a call to the actual function, instead of calling the funapply function. Now we should look if it is possible to inline that call. Should give another 8% on raytrace
Blocks: 768745
Attached patch Refactor calls + inline WIP (obsolete) — Splinter Review
Quite a lot of refactoring, but afterwards inlining this call is only 4 extra lines.

Currently still fails on:
    --ion-eager jit-test/tests/basic/testRebranding.js
    --ion-eager jit-test/tests/ion/bug-770309-mcall-bailout.js
    --ion-eager jit-test/tests/ion/new-9.js
    --ion-eager jit-test/tests/jaeger/recompile/inlinestubs.js
    --ion-eager jit-test/tests/v8-v5/check-deltablue.js

Also raytrace doesn't get inlined yet because "code->monitoredTypes || code->monitoredTypesReturn" is true.
Assignee: general → hv1989
Depends on: 796114
Create a CallInfo class that contains the fun/this/args. This class can be used to provide information to functions, instead of having a lot of function arguments. All Call operations are also adjusted to fit in this system.

It also add inlineScripedCall(target). This is almost the same as jsop_call_inline. It does a few things more, to allow it to be called out of inlineScriptedCalls(targets ...). Eventually jsop_call_inline should die and also when multiple calls are inlined we should use inlineScripedCall(target) to inline a call at a time. This isn't done yet and could introduce new bugs etc as this code is hacky. Therefore I will open a new bug for this.

Functionality wise this bug shouldn't make any changes. It's all preparations.
Attachment #698698 - Attachment is obsolete: true
Attachment #706473 - Flags: review?(dvander)
To get decent information about inlining a funapply call, we need to remove looking at the callsite. monitoredTypesReturn will always be true, because in the bytecode there is a native call over there. This patch adjusts this checks and compares the TypeSet of the returned value to the TypeSet of the different targets. Should give the same information as just looking to monitoredTypesReturn, but now we can use the same mechanism to decide when to inline funapply
Attachment #706479 - Flags: review?(dvander)
Attached patch Inline funapplySplinter Review
Now inlining funapply calls isn't that hard anymore. This is the logic.

I'll open a follow-up bug on this, because now we don't inline all funapply functions of raytrace, but it should already be a nice speedup of 8%.
Attachment #706481 - Flags: review?(dvander)
Attachment #706473 - Attachment is patch: true
Comment on attachment 706473 [details] [diff] [review]
Structural changes to IonBuilder

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

::: js/src/ion/IonBuilder.cpp
@@ +3434,5 @@
> +        return false;
> +
> +    // The function will get replaced with constant
> +    //callInfo.fun()->setFoldedUnchecked();
> +    //callInfo.setFun(NULL);

Was this intended to be commented?

@@ +6332,5 @@
>      current->push(wrapper);
>  
> +    CallInfo callInfo(cx, false);
> +    current->popFormals(callInfo, 0);
> +    callInfo.setTypeInfo(types, barrier);

No CallInfo(cx, types, barrier, false)?

@@ +6493,5 @@
>  
>          // Call the setter. Note that we have to push the original value, not
>          // the setter's return value.
> +        CallInfo callInfo(cx, false);
> +        current->popFormals(callInfo, 1);

Maybe a better pattern for these would be something like:

    CallInfo info(cx, current, false);

Or if it has to be fallible,

    CallInfo info(cx, false);
    if (!info.init(current))
        ...

::: js/src/ion/MIRGraph.cpp
@@ +383,5 @@
> +    stackPosition_ -= n;
> +}
> +
> +void
> +MBasicBlock::popFormals(CallInfo &info, uint32_t argc)

I think popFormals/pushFormals should be part of IonBuilder instead of MBasicBlock. CallInfo is already mostly localized there.

@@ +390,5 @@
> +    JS_ASSERT(info.argc() == 0);
> +
> +    // Get the arguments in the right order
> +    Vector<MDefinition *> *args = info.argv();
> +    args->reserve(argc);

reserve is fallible, so you might as well just check append()
Attachment #706473 - Flags: review?(dvander) → review+
Attachment #706479 - Flags: review?(dvander) → review+
Comment on attachment 706481 [details] [diff] [review]
Inline funapply

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

::: js/src/ion/Bailouts.cpp
@@ +179,5 @@
>  
>      IonSpew(IonSpew_Bailouts, " new PC is offset %u within script %p (line %d)",
>              pcOff, (void *)script(), PCToLineNumber(script(), regs.pc));
> +
> +    // For fun.apply({}, arguments) the reconstructStackDepth will have stackdepth 4,

"at least 4" (since the expression could be nested)

::: js/src/ion/FixedList.h
@@ +45,5 @@
>          JS_ASSERT(num < length_);
>          length_ -= num;
>      }
>  
> +    bool increase(size_t num) {

s/increase/growBy/

@@ +47,5 @@
>      }
>  
> +    bool increase(size_t num) {
> +        T *list = (T *)GetIonContext()->temp->allocate((length_ + num) * sizeof(T));
> +        if (list != NULL) {

nit:

if (!list)
    return false;

...

::: js/src/ion/IonBuilder.cpp
@@ +3961,5 @@
>  
> +    // Set type information
> +    types::StackTypeSet *barrier;
> +    types::StackTypeSet *types = oracle->returnTypeSet(script(), pc, &barrier);
> +    callInfo.setTypeInfo(types, barrier);

If this pattern is common in the earlier patches, it might look nicer to have oracle->getReturnTypes(&callInfo);

::: js/src/ion/IonFrames.cpp
@@ +927,5 @@
>          // Recover the number of actual arguments from the script.
> +        if (JSOp(*pc_) != JSOP_FUNAPPLY)
> +            numActualArgs_ = GET_ARGC(pc_);
> +
> +        JS_ASSERT(numActualArgs_ != 0xbad);

I think this assert is bogus, numActualArgs can be up to 0xFFFF (in theory). If something is debug-only initializing it, maybe change it to 0xbadbad.
Attachment #706481 - Flags: review?(dvander) → review+
@Dave:

I went for:
CallInfo callInfo(cx, constructing);
if (!callInfo.init(current, argc))

Because you didn't want pop/pushformals in MBasicGraph, I moved them to CallInfo instead of IonBuilder. Else it wasn't possible to add the previous "init" function. Looks much better ;).
Depends on: 835178, 835180
Hannes, this was causing build errors. I pushed a fix here:

https://hg.mozilla.org/integration/mozilla-inbound/rev/74d4a1dc470c

Crucially, I reversed a test that looked wrong to me. Please look this over and back out if you disagree.
Some extra OOM fixes, reviewed by jandem on IRC.

https://hg.mozilla.org/integration/mozilla-inbound/rev/bba8e652952f
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Nice - this was a 13% win on octane-raytrace
https://hg.mozilla.org/mozilla-central/rev/bba8e652952f
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
(In reply to David Anderson [:dvander] from comment #12)
> Nice - this was a 13% win on octane-raytrace
Cool, more than estimated :D

I also had some testcases for this. Seems like they disappeared somewhere. As far as I know there is no need for review to add testcases. So I just pushed them.
https://hg.mozilla.org/integration/mozilla-inbound/rev/7d91cd6f1124
No longer depends on: 796114
Depends on: 836274
You need to log in before you can comment on or make changes to this bug.