1619343 - Support SpreadNew and SpreadSuperCall in Ion

Status: RESOLVED FIXED

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

Description

[André Bargull [:anba]]

It isn't too hard to support JSOp::SpreadNew and JSOp::SpreadSuperCall in Ion when we reuse the existing code for JSOp::SpreadCall. That code does have some issues, but it's all we have at the moment and we probably want to wait for WarpBuilder to write a cleaner implementation for spread calls. Also supporting JSOp::SpreadSuperCall means we're no longer forced to stay in baseline when running derived class constructors!

Comment 1

[André Bargull [:anba]]

Attachment: Bug 1619343 - Part 1: Move zero arguments jump out of emitAllocateSpaceForApply. r=jandem!

Move the branch instruction out of |emitAllocateSpaceForApply()| to make it
easier to follow the control flow.

Drive-by fix:

• Reformat a comment which was broken into two lines by clang-format.
• Use the correct lir-class when accessing |ThisIndex| for |LApplyArrayGeneric|.

Comment 2

[André Bargull [:anba]]

Attachment: Bug 1619343 - Part 2: Split emitPushArguments to make it reusable. r=jandem!

Split |emitPushArguments()| so we can reuse the elements copying code for
part 4.

Depends on D64978

Comment 3

[André Bargull [:anba]]

Attachment: Bug 1619343 - Part 3: Prepare emitApplyGeneric to support constructor calls. r=jandem!

SpreadNew and SpreadSuperCall will reuse this function, so prepare it to
support constructor calls by adding the missing pieces based on what we perform
in |visitCallGeneric()|.

Depends on D64979

Comment 4

[André Bargull [:anba]]

Attachment: Bug 1619343 - Part 4: Support SpreadNew and SpreadSuperCall in Ion. r=jandem!

Support |JSOp::SpreadNew| and |JSOp::SpreadSuperCall| by reusing the already
present infrastructure for |JSOp::SpreadCall|.

|LConstructArrayGeneric| needs an additional register for the |new.target|
value, so we have to share |tempStackCounter| with the |new.target| register,
similar to how we already share the |elements| register with the |argc|
register.

This register sharing leads to a slightly different code in
|emitAllocateSpaceForConstructAndPushNewTarget()| compared its counterpart in
|emitAllocateSpaceForApply()|, because we have to push |new.target| onto the
stack before we have a free register available to store |extraStackSpace|. And
contrary to |emitAllocateSpaceForApply()|, we're also always pushing
|JS_ARG_POISON| when we need padding instead of leaving the stack contents
undefined. If want to squeeze out a bit more performance, we could use
masm.subFromStackPtr(Imm32(sizeof(Value))), but it probably won't make much
of a difference in practice, so it seems nicer to use the "safer" route and
always push |JS_ARG_POISON|.

Depends on D64980

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3e88ffbc2168
Part 1: Move zero arguments jump out of emitAllocateSpaceForApply. r=jandem
https://hg.mozilla.org/integration/autoland/rev/74c24cb3ac9a
Part 2: Split emitPushArguments to make it reusable. r=jandem
https://hg.mozilla.org/integration/autoland/rev/2f8f4267d3e7
Part 3: Prepare emitApplyGeneric to support constructor calls. r=jandem
https://hg.mozilla.org/integration/autoland/rev/2872f192769e
Part 4: Support SpreadNew and SpreadSuperCall in Ion. r=jandem

Comment 6

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/3e88ffbc2168
https://hg.mozilla.org/mozilla-central/rev/74c24cb3ac9a
https://hg.mozilla.org/mozilla-central/rev/2f8f4267d3e7
https://hg.mozilla.org/mozilla-central/rev/2872f192769e