1179047 - ARM64: Support new.target in trampolines.

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Eric Faust [:efaust]]

There'll also be some cleanups in there....

Comment 1

[Eric Faust [:efaust]]

Attachment: Support new.target in arm trampolines.

Added a few comments. Some may be extraneous. The XXX is removed in the next patch.

Comment 2

[Eric Faust [:efaust]]

Attachment: arm64-Silly.patch

In the original arm trampoline, we couldn't touch r5 because it was implicitly the other half of the value operand with base r4 used in the 64 bit moves. On arm64, a value fits into a single register, so we have a free one to skip this needless extra computation.

Comment 3

[Sean Stangl [:sstangl]]

Comment on attachment 8628346 [details] [diff] [review]
arm64-Silly.patch

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

::: js/src/jit/arm64/Trampoline-arm64.cpp
@@ +331,2 @@
>      masm.And(x6, x1, Operand(CalleeTokenMask));
> +    masm.Mov(x5, x6);

If you change this and the line below it to the following:

masm.And(x5, x1, Operand(CalleeTokenMask);
masm.Ldrh(x6, MemOperand(x5, ...));

Then we can avoid the Mov() also.

Comment 4

[Sean Stangl [:sstangl]]

Comment on attachment 8628345 [details] [diff] [review]
Support new.target in arm trampolines.

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

Thanks for doing this, and for leaving the code better than you found it!

::: js/src/jit/arm64/Trampoline-arm64.cpp
@@ +339,3 @@
>  
>      // Calculate the position that our arguments are at before sp gets modified.
> +    MOZ_ASSERT(ArgumentsRectifierReg == r8, "x8 used for argc in Arguments Rectifier");

Can this be a static_assert?

@@ +342,5 @@
>      masm.Add(x3, masm.GetStackPointer64(), Operand(x8, vixl::LSL, 3));
>      masm.Add(x3, x3, Operand(sizeof(RectifierFrameLayout)));
>  
> +    // Pad to a multiple of 16 bytes. This neglects the |this| value,
> +    // which will aslo be pushed, because the rest of hte frame will

nit: also, the

@@ +357,5 @@
> +
> +        // new.target lives at the end of the pushed args
> +        // NB: The arg vector holder starts at the beginning of the last arg,
> +        //     add a value to get to argv[argc]
> +        masm.Ldr(x4, MemOperand(x3, sizeof(Value), vixl::Offset));

Prefer the normal:

masm.loadPtr(Address(r3, sizeof(Value)), r4).

Honestly most of this code should be written in arch-independent style, but it's not your fault that it isn't.

@@ +388,5 @@
>          masm.Subs(x8, x8, Operand(1));
>          masm.B(&copyLoopTop, Assembler::NotSigned);
>      }
>  
> +    // Fix up the size of the stack frame. +1 accounts for |this|

nit: "." at end

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f5f2f3135bbd
https://hg.mozilla.org/integration/mozilla-inbound/rev/51acad1faab1

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f5f2f3135bbd
https://hg.mozilla.org/mozilla-central/rev/51acad1faab1