1112160 - Align16: Dynamically align Baseline -> Ion ICs.

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Nicolas B. Pierron [:nbp]]

Baseline has no garantee about the frame size, so even if the trampoline was
aligned, we might have lost this property.  We should ensure that Baseline ICs
for calling into Ion are aligned such that the next Ion frame is aligned on 16
bytes.

Comment 1

[Nicolas B. Pierron [:nbp]]

Attachment: Baseline Stubs: Align Jit frames before calling any jitted code.

Comment 2

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8559228 [details] [diff] [review]
Baseline Stubs: Align Jit frames before calling any jitted code.

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

::: js/src/jit/BaselineIC.cpp
@@ +10817,5 @@
>          regs.add(BaselineTailCallReg);
>  
>      // Values are on the stack left-to-right. Calling convention wants them
>      // right-to-left so duplicate them on the stack in reverse order.
> +    pushCallArguments(masm, regs, argcReg, /* isJitCall = */ true);

This line is bogus, because fun.call is popping the |this| argument from the stack, which shift everything by one Value, and thus make the stack always be miss-aligned.

Comment 3

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8559228 [details] [diff] [review]
Baseline Stubs: Align Jit frames before calling any jitted code.

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

Ok, I am asking again for review, knowing that this is line is currently bogus, but the next upcoming patch add a new function that I used to fix this code path.

Comment 4

[Nicolas B. Pierron [:nbp]]

Attachment: BaselineIC, align the stack when the number of argument is statically known.

Comment 5

[Nicolas B. Pierron [:nbp]]

Attachment: BaselineIC, align the stack when the number of argument is statically known.

Delta:
 - Fix, IC scripted setter calls, by using the frame register, and optimize
   the code by removing a useless move instruction.

Comment 6

[Jan de Mooij [:jandem]]

Comment on attachment 8559228 [details] [diff] [review]
Baseline Stubs: Align Jit frames before calling any jitted code.

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

Nice!

::: js/src/jit/BaselineIC.cpp
@@ +10183,5 @@
> +
> +        // Load the number of arguments present before the stub frame.
> +        Register argcReg = JSReturnOperand.scratchReg();
> +        if (isSpread_) {
> +            // account for the Array object.

Nit: s/account/Account/

::: js/src/jit/MacroAssembler.cpp
@@ +2051,5 @@
>      bind(&spsNotEnabled);
>  }
> +
> +void
> +MacroAssembler::alignJitStackBasedOnNArgs(Register nargs)

Nit: maybe s/JitStack/Stack/, because there's one stack and we're in js/src/jit/ so it should be clear.

Comment 7

[Nicolas B. Pierron [:nbp]]

(Try) https://treeherder.mozilla.org/#/jobs?repo=try&revision=220ed7bf0305
https://hg.mozilla.org/integration/mozilla-inbound/rev/2eca2e88f8e3
https://hg.mozilla.org/integration/mozilla-inbound/rev/25c0f57abbdb

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2eca2e88f8e3
https://hg.mozilla.org/mozilla-central/rev/25c0f57abbdb

Comment 9

[Cameron Kaiser [:spectre]]

This makes the PowerPC JIT (which worked in Fx36) assert, since it was written to assume no particular alignment other than word size. What was the rationale for this?

Comment 10

[Nicolas B. Pierron [:nbp]]

(In reply to Cameron Kaiser [:spectre] from comment #9)
> This makes the PowerPC JIT (which worked in Fx36) assert, since it was
> written to assume no particular alignment other than word size. What was the
> rationale for this?

This is needed on some platform, as SIMD registers cannot be spilled if the pointers are not aligned.  As IonMonkey does not dynamically aligned its frames, we ensure the alignment is correct at calls into IonMonkey code.

All these assertions should be based on the constants defined for the Architecture.