ARM64: Support new.target in trampolines.

RESOLVED FIXED in Firefox 42

Status

()

Core
JavaScript Engine: JIT
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: efaust, Assigned: efaust)

Tracking

unspecified
mozilla42
Points:
---

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

3 years ago
There'll also be some cleanups in there....
(Assignee)

Comment 1

3 years ago
Created attachment 8628345 [details] [diff] [review]
Support new.target in arm trampolines.

Added a few comments. Some may be extraneous. The XXX is removed in the next patch.
Assignee: nobody → efaustbmo
Status: NEW → ASSIGNED
Attachment #8628345 - Flags: review?(sstangl)
(Assignee)

Comment 2

3 years ago
Created attachment 8628346 [details] [diff] [review]
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.
Attachment #8628346 - Flags: review?(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.
Attachment #8628346 - Flags: review?(sstangl) → review+
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
Attachment #8628345 - Flags: review?(sstangl) → review+
https://hg.mozilla.org/mozilla-central/rev/f5f2f3135bbd
https://hg.mozilla.org/mozilla-central/rev/51acad1faab1
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox42: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.