1338920 - Make spreadcall work in Ion

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect, P1)

Description

[Tom S [:evilpie]]

There are various unimplemented spread related opcodes in Ion that we need to implement.

Comment 1

[Tom S [:evilpie]]

A simple spreadcall like f(...a), seems to involve at least these implemented opcodes: spreadcallarray, initelem_inc and spreadcall. We have various more spread variations like spreadnew etc.

Comment 3

[Ted Campbell [:tcampbell]]

Attachment: Bug 1338920 - Support JSOP_SPREADCALLARRAY in Ion

Review commit: https://reviewboard.mozilla.org/r/124934/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/124934/

Comment 4

[Ted Campbell [:tcampbell]]

Attachment: Bug 1338920 - Support JSOP_SPREADCALL in Ion

Review commit: https://reviewboard.mozilla.org/r/124936/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/124936/

Comment 5

[Ted Campbell [:tcampbell]]

This bug depends on 1351951 to land first to avoid rare crashes.

Try run is here:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e430390dc6cd104809431e0f8a529b1fabc62d92

Comment 6

[Hannes Verschore [:h4writer]]

Comment on attachment 8852771 [details]
Bug 1338920 - Support JSOP_SPREADCALLARRAY in Ion

https://reviewboard.mozilla.org/r/124934/#review127572

Comment 7

[Tom S [:evilpie]]

I am trying to understand why you check for arguments, when afterwards we only allow packed arrays anyway? We also need a followup for JSOP_OPTIMIZE_SPREADCALL, maybe we can just remove this optimization now that we ion compile that pattern it's optimizing for.

Comment 8

[Ted Campbell [:tcampbell]]

Oops, you are right. I was using FUNAPPLY as a reference. Looking in to how MagicOptimizedArguments works, you are right that the check is not needed at all.

Also, reviewing the arguments_analysis handling, I also misunderstood that and am removing it.

Taking another look at JSOP_OPTIMIZE_SPREADCALL.

Comment 9

[Ted Campbell [:tcampbell]]

Comment on attachment 8852772 [details]
Bug 1338920 - Support JSOP_SPREADCALL in Ion

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/124936/diff/1-2/

Comment 10

[Ted Campbell [:tcampbell]]

This patch still brings down ARES-6 by 10% (91ms -> 83ms).

Comment 11

[Ted Campbell [:tcampbell]]

Comment on attachment 8852771 [details]
Bug 1338920 - Support JSOP_SPREADCALLARRAY in Ion

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/124934/diff/1-2/

Comment 12

[Ted Campbell [:tcampbell]]

Comment on attachment 8852772 [details]
Bug 1338920 - Support JSOP_SPREADCALL in Ion

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/124936/diff/2-3/

Comment 13

[Ted Campbell [:tcampbell]]

I've updated the comments for SPREADCALL in IonBuilder to hopefully make it clearer.

I'm going to leave on JSOP_OPTIMIZE_SPREADCALL to another bug. Supporting the opcode with a VMCall (same as Baseline) is easy, there are a bunch of control flow annotation issues that need to be handled, so I will open a different bug for that.

Comment 14

[Hannes Verschore [:h4writer]]

Comment on attachment 8852772 [details]
Bug 1338920 - Support JSOP_SPREADCALL in Ion

https://reviewboard.mozilla.org/r/124936/#review128394

Nice improvement :D

::: js/src/jit/IonBuilder.cpp:5067
(Diff revision 3)
> +    // handled when the user objects are expanded and copied into this hidden
> +    // array.
> +    //
> +    // One special case is JSOP_OPTIMIZE_SPREADCALL may be used to bypass this
> +    // temporary array. In this case, the JSOP_OPTIMIZE_SPREADCALL will check
> +    // that the argument is an ArrayObject with unmodified iterator semantics.

I think the explanation of the JSOP_OPTIMIZE_SPREADCALL shouldn't be here but at jsop_optimize_spreadcall. Can you remove it here?

::: js/src/jit/IonBuilder.cpp:5077
(Diff revision 3)
> +    if (TemporaryTypeSet* objTypes = argument->resultTypeSet())
> +        if (const Class* clasp = objTypes->getKnownClass(constraints()))
> +            MOZ_ASSERT(clasp == &ArrayObject::class_);
> +#endif
> +
> +    MDefinition* argObj = current->pop();

Can we call this argArr

Comment 15

[Hannes Verschore [:h4writer]]

(In reply to Tom Schuster [:evilpie] from comment #7)
> ... We also need a followup for
> JSOP_OPTIMIZE_SPREADCALL, maybe we can just remove this optimization now
> that we ion compile that pattern it's optimizing for.

Not sure I totally understand this. The pattern makes sure we don't always have to copy the spreadcalls. Which also works in the interpreter and baseline. As a result removing that optimization might introduce some regression in the interpreter/baseline right?

@Ted: I think a quick fix would be to always "return BooleanValue(false);" for JSOP_OPTIMIZE_SPREADCALL in this bug and create a follow-up bug to properly support it.

Comment 16

[Ted Campbell [:tcampbell]]

You would think that.. Turns out IonControlFlow does not like any part of how we emit the bytecode for JSOP_OPTIMIZE_SPREADCALL. I'm working on a proper fix in the BytecodeEmitter.

Comment 17

[Ted Campbell [:tcampbell]]

Comment on attachment 8852772 [details]
Bug 1338920 - Support JSOP_SPREADCALL in Ion

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/124936/diff/3-4/

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/417a89dc9643
Support JSOP_SPREADCALLARRAY in Ion r=h4writer
https://hg.mozilla.org/integration/autoland/rev/6cecf25056c1
Support JSOP_SPREADCALL in Ion r=h4writer

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/417a89dc9643
https://hg.mozilla.org/mozilla-central/rev/6cecf25056c1