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

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.

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.

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

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%.

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()

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.

I pushed them in parts, to make it easier to blame the right component if fuzzers complain. https://hg.mozilla.org/integration/mozilla-inbound/rev/551b94a23a79 https://hg.mozilla.org/integration/mozilla-inbound/rev/04cea75c849d https://hg.mozilla.org/integration/mozilla-inbound/rev/e8578248b798 Forget the nits in your last comment, sorry https://hg.mozilla.org/integration/mozilla-inbound/rev/b1eaede8e5d7

@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 ;).

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.

https://hg.mozilla.org/mozilla-central/rev/551b94a23a79 https://hg.mozilla.org/mozilla-central/rev/04cea75c849d https://hg.mozilla.org/mozilla-central/rev/e8578248b798 https://hg.mozilla.org/mozilla-central/rev/b1eaede8e5d7 https://hg.mozilla.org/mozilla-central/rev/74d4a1dc470c

Some extra OOM fixes, reviewed by jandem on IRC. https://hg.mozilla.org/integration/mozilla-inbound/rev/bba8e652952f

Nice - this was a 13% win on octane-raytrace

(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