835178 - IonMonkey: inline all funapply functions in raytrace

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Hannes Verschore [:h4writer]]

This is to inline the rest of the funapply funcctions in raytrace. Bug 824473 does already most of them, but Color doesn't get inlined yet...

Comment 1

[Hannes Verschore [:h4writer]]

Attachment: Inline JSOP_SETARG

If I'm not mistaken, that is the only extra code needed to inline JSOP_SETARG (when there is no arguments object).

Boosts v8-raytrace with 7%.

Comment 2

[Hannes Verschore [:h4writer]]

Attachment: Inline JSOP_SETARG

Comment 3

[Nicolas B. Pierron [:nbp]]

Comment on attachment 707520 [details] [diff] [review]
Inline JSOP_SETARG

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

I think there might be an error as the non-formal actual arguments might be copied from the inliner frame instead of the inlinee frame.  Can you ensure that this is working and re-ask for a review on this patch if I am wrong.

::: js/src/ion/IonFrameIterator-inl.h
@@ +61,5 @@
>      JS_ASSERT(start <= end && end <= nactual);
>  
> +    SnapshotIterator s(si_);
> +    Value *argv = frame_->actualArgs();
> +    s.readFrameArgs(op, argv, NULL, NULL, start, nformal, end);

Ok, this is a bit confusing to use frame->actualArgs() for inlined frames.
Can you initialize it that way:

  Value *argv = more() ? NULL : frame_->actualArgs();

You will probably have to set the end to be nformal instead of end, such as this would not copy anything from the actual arguments.  And you probably want to add a test case to ensure that we don't get actual arguments of the inliner instead of the inlinee.

At the end, I think keeping the original split might still be advantageous.

::: js/src/jit-test/tests/ion/bug835178.js
@@ +4,5 @@
> +function inlined() { return foo.apply({}, arguments); }
> +function test(a,b,c) { return inlined(a,b,c) }
> +
> +print(test(1,2,3));
> +print(test(0,2,3));

use assertEq instead of print in a test case.

::: js/src/jsanalyze.cpp
@@ +446,5 @@
>            }
>  
>            case JSOP_SETARG:
>              modifiesArguments_ = true;
> +            isJaegerInlineable = false;

Can you also add a test case to make sure that arguments given to fun.apply are correct even if we have a SETARG in the inliner function?

Comment 4

[Hannes Verschore [:h4writer]]

Attachment: Inline JSOP_SETARG

Good catch! This should be better

Comment 6

[Nicolas B. Pierron [:nbp]]

Comment on attachment 707599 [details] [diff] [review]
Inline JSOP_SETARG

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

::: js/src/ion/IonBuilder.cpp
@@ +881,5 @@
> +
> +        // Currently arguments and setarg are not used together, see abort above.
> +        // But for good bookkeeping we already update the inlinedArguments_ array.
> +        if (inliningDepth > 0)
> +            inlinedArguments_[GET_SLOTNO(pc)] = current->peek(-1);

This is not needed for this patch, and tihs is wrong as this does not account of the Phi produced for formal argument in cases where 2 JSOP_SETARG are done in 2 different branches.

If you want to do so you should consider extracting the formal args from the basic block, and prevent arguments to be used out-side fun.apply, filtering out arguments[i] as i might iterate on formal arguments (unless the function has no formal args).

::: js/src/jit-test/tests/ion/bug835178.js
@@ +13,5 @@
> +assertEq(h(false, false), true);
> +assertEq(h(false, false), true);
> +
> +function g2(a) { if (a) { if (g2.arguments[1]) return true; return false; } return true; };
> +function f2() { return g2(false, true); };

I guess you want to call g2 with the first argument of h2?

Comment 7

[Hannes Verschore [:h4writer]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e9a03227ae72

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e9a03227ae72