Closed Bug 1179047 Opened 6 years ago Closed 6 years ago

ARM64: Support new.target in trampolines.

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: efaust, Assigned: efaust)

Details

Attachments

(2 files)

There'll also be some cleanups in there....
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)
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
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.