Closed Bug 835178 Opened 9 years ago Closed 9 years ago

IonMonkey: inline all funapply functions in raytrace


(Core :: JavaScript Engine, defect)

Not set





(Reporter: h4writer, Assigned: h4writer)




(1 file, 2 obsolete files)

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...
Blocks: 824473
Attached patch Inline JSOP_SETARG (obsolete) — Splinter Review
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%.
Assignee: general → hv1989
Attached patch Inline JSOP_SETARG (obsolete) — Splinter Review
Attachment #707372 - Attachment is obsolete: true
Attachment #707520 - Flags: review?(nicolas.b.pierron)
Attachment #707520 - Attachment is patch: true
Comment on attachment 707520 [details] [diff] [review]

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?
Attachment #707520 - Flags: review?(nicolas.b.pierron)
Good catch! This should be better
Attachment #707520 - Attachment is obsolete: true
Attachment #707599 - Flags: review?(nicolas.b.pierron)
Duplicate of this bug: 827153
Comment on attachment 707599 [details] [diff] [review]

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?
Attachment #707599 - Flags: review?(nicolas.b.pierron) → review+
Depends on: 835877
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in before you can comment on or make changes to this bug.