1250370 - IonMonkey: MIPS: Fix stack alignment checking in EmitBaselineEnterStubFrame

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[hev [:hev]]

We need to remove the stack alignemnt checking in EmitBaselineEnterStubFrame for MIPS64.

On MIPS32, The stack alignment is 8-byte. If do a JSOP_CALL with one value, two 4-byte will be pushed to stack, that's ok.

But on MIPS64, The stack alignment is 16-byte. If do a JSOP_CALL with one value, one 8-byte pushed, the stack doesn't aligned by 16-byte at enter stub frame.

Comment 1

[hev [:hev]]

Attachment: 0001-Bug-1250370-IonMonkey-MIPS-Don-t-check-stack-alignme.patch

Comment 2

[Tooru Fujisawa [:arai]]

(In reply to Heiher [:hev] from comment #0)
> On MIPS32, The stack alignment is 8-byte. If do a JSOP_CALL with one value,
> two 4-byte will be pushed to stack, that's ok.
> 
> But on MIPS64, The stack alignment is 16-byte. If do a JSOP_CALL with one
> value, one 8-byte pushed

Can you point me to the code where the value is pushed?

Comment 3

[hev [:hev]]

(In reply to Tooru Fujisawa [:arai] from comment #2)
> (In reply to Heiher [:hev] from comment #0)
> > On MIPS32, The stack alignment is 8-byte. If do a JSOP_CALL with one value,
> > two 4-byte will be pushed to stack, that's ok.
> > 
> > But on MIPS64, The stack alignment is 16-byte. If do a JSOP_CALL with one
> > value, one 8-byte pushed
> 
> Can you point me to the code where the value is pushed?

Yes. In jit-test/tests/ion/bug1233343.js:23 and js/src/jit/BaselineCompiler.cpp:emit_JSOP_INT8

Comment 4

[Tooru Fujisawa [:arai]]

thanks, now I think I understand the situation.

I was searching the difference between arm64 and mips64, as arm64 code says it's aligned to 16-bytes.

https://dxr.mozilla.org/mozilla-central/rev/789a12291942763bc1e3a89f97e0b82dc1c9d00b/js/src/jit/arm64/SharedICHelpers-arm64.h#171
> inline void
> EmitBaselineEnterStubFrame(MacroAssembler& masm, Register scratch)
> {
> ...
>     // Stack should remain 16-byte aligned.
>     masm.checkStackAlignment();
> }

but the commnet seems to be wrong (or at least inappropriate), as checkStackAlignment checks 8-byte alignment.

https://dxr.mozilla.org/mozilla-central/rev/789a12291942763bc1e3a89f97e0b82dc1c9d00b/js/src/jit/arm64/MacroAssembler-arm64.h#2542
>     void checkStackAlignment() {
> #ifdef DEBUG
>         checkARMRegAlignment(GetStackPointer64());
> 
>         // If another register is being used to track pushes, check sp explicitly.
>         if (!GetStackPointer64().Is(vixl::sp))
>             checkARMRegAlignment(vixl::sp);
> #endif
>     }

https://dxr.mozilla.org/mozilla-central/rev/789a12291942763bc1e3a89f97e0b82dc1c9d00b/js/src/jit/arm64/MacroAssembler-arm64.h#2527
>    void checkARMRegAlignment(const ARMRegister& reg) {
> ...
>        Label aligned;
>        Mov(scratch64, reg);
>        Tst(scratch64, Operand(StackAlignment - 1));
>        B(Zero, &aligned);
> ...
>    }

https://dxr.mozilla.org/mozilla-central/rev/789a12291942763bc1e3a89f97e0b82dc1c9d00b/js/src/jit/arm64/Architecture-arm64.h#312
> // Although sp is only usable if 16-byte alignment is kept,
> // the Pseudo-StackPointer enables use of 8-byte alignment.
> static const uint32_t StackAlignment = 8;

(the comment in arm64 code should be fixed in another bug)

anyway, we might need something similar to arm64-equivelant masm.checkStackAlignment for mips64?
As what we're checking in mips32 is also Value-size alignment, that is not necessarily native stack alignment.

sstangl, can I have your opinion about stack alignment check after EmitBaselineEnterStubFrame and the expected alignment size?

Comment 5

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8722306 [details] [diff] [review]
0001-Bug-1250370-IonMonkey-MIPS-Don-t-check-stack-alignme.patch

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

::: js/src/jit/mips-shared/SharedICHelpers-mips-shared.h
@@ +208,2 @@
>      // Stack should remain aligned.
>      masm.checkStackAlignment();

Instead of disabling this code, would it make more sense to replace it by a call to

    masm.assertStackAlignment(JitStackAlignment, 2 * sizeof(Value));

?

Comment 6

[Nicolas B. Pierron [:nbp]]

FYI, here is the code [1] which verifies that Jit frames are aligned correctly, and this does not involve checking baseline Stub frames.  If I do recall correctly the baseline stubs are padding the frame to align correctly for Ion / Baseline calls, but not for baseline stubs.

So, the only restriction would be to make sure that we can push double/Value aligned on the stack, with no offset.  So this would be:

    masm.assertStackAlignment(sizeof(Value), 0);

[1] https://dxr.mozilla.org/mozilla-central/source/js/src/jit/JitFrames.cpp#3069

Comment 7

[Tooru Fujisawa [:arai]]

Comment on attachment 8722306 [details] [diff] [review]
0001-Bug-1250370-IonMonkey-MIPS-Don-t-check-stack-alignme.patch

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

Thank you nbp for the explanation :)

hev, can you check if |masm.assertStackAlignment(sizeof(Value), 0);| works on mips64?

Comment 8

[hev [:hev]]

(In reply to Tooru Fujisawa [:arai] from comment #7)
> Comment on attachment 8722306 [details] [diff] [review]
> 0001-Bug-1250370-IonMonkey-MIPS-Don-t-check-stack-alignme.patch
> 
> Review of attachment 8722306 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thank you nbp for the explanation :)
> 
> hev, can you check if |masm.assertStackAlignment(sizeof(Value), 0);| works
> on mips64?

It works fine for mips32 and mips64. Thank you arai, nbp. :D

Comment 9

[hev [:hev]]

Attachment: 0001-Bug-1250370-IonMonkey-MIPS-Fix-stack-alignment-check.patch

Comment 10

[Tooru Fujisawa [:arai]]

Comment on attachment 8724416 [details] [diff] [review]
0001-Bug-1250370-IonMonkey-MIPS-Fix-stack-alignment-check.patch

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

looks good to me, but I think I'm not an appropriate reviewer for this case.
forwarding to sstangl.

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1d161d4e6cc5

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1d161d4e6cc5