Closed Bug 1112160 Opened 5 years ago Closed 5 years ago

Align16: Dynamically align Baseline -> Ion ICs.

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox38 --- fixed

People

(Reporter: nbp, Assigned: nbp)

References

Details

Attachments

(2 files, 1 obsolete file)

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.
Assignee: nobody → nicolas.b.pierron
Status: NEW → ASSIGNED
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.
Attachment #8559228 - Flags: review?(jdemooij)
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.
Attachment #8559228 - Flags: review+
Attachment #8559228 - Flags: review+ → review?(jdemooij)
Delta:
 - Fix, IC scripted setter calls, by using the frame register, and optimize
   the code by removing a useless move instruction.
Attachment #8559916 - Attachment is obsolete: true
Attachment #8559916 - Flags: review?(jdemooij)
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.
Attachment #8559228 - Flags: review?(jdemooij) → review+
Attachment #8560542 - Flags: review?(jdemooij)
Attachment #8560542 - Flags: review?(jdemooij) → review+
https://hg.mozilla.org/mozilla-central/rev/2eca2e88f8e3
https://hg.mozilla.org/mozilla-central/rev/25c0f57abbdb
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
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?
(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.
You need to log in before you can comment on or make changes to this bug.