1425583 - ARM64: getStackPointer() and Register cannot work when using real SP

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, enhancement, P3)

Description

[Lars T Hansen [:lth]]

On ARM64 we have a provision for two stack pointers: the real one, which is sort of weird, and our own pseudostack pointer, which is sort of inefficient.  For the wasm baseline compiler I was hoping to use the real stack pointer (since the compiler doesn't have to push and pop individual words on the stack but allocates the frame in larger chunks), but the current structure of our masm is against that.  In particular, getStackPointer() will return the currently enabled stack pointer register, but if that is the real stack pointer it will force the code of the returned register to be 31.  That is wrong, because that code is used by Vixl for the "zero register" (same encoding as SP in the machine but different meaning in the assembler); the stack pointer has code 63 in vixl.  This matters to vixl in many cases because it generates different code depending on whether that register is 31 or 63.  If it is 31, vixl generally assumes you want a zero.  This breaks down so often that either I should abandon trying to use the real sp with masm, or we need to fix getStackPointer() and anything else that might depend on such a change.

I don't know yet what the consequences would be of just encoding SP as code 63 in a Register; after all, it's not like the SP goes into the register sets.

Comment 1

[Lars T Hansen [:lth]]

One "option" here is to un-translate that translation in the masm when necessary.  I've done a few of them already; there are some pinch points (toMemOperand and doBaseIndex [sic]) that simplifies this but it could be whack-a-mole.  It depends on needs; the baseline compilers have fairly simple needs, in the absence of Ion.

Comment 2

[Lars T Hansen [:lth]]

Discussed this briefly with nbp.  He had a couple of suggestions.  One, we could make the SP register a different type (in C++) so that SP is distinguishable in contexts where it's used.  This might not be too much of a hardship, though it'll create multiple variants of Address and BaseIndex and others, I bet.  Maybe on platforms where we don't care about the distinction we could typedef the SP as Register.  Another option, we could change the meaning of SP and ZERO in Vixl so that SP (which SpiderMonkey uses) has value 31 and getStackPointer() will work fine on it; meanwhile ZERO is not used by SpiderMonkey and SM is not going to be bothered by this change, particularly.

Comment 3

[Lars T Hansen [:lth]]

> ... we could make the SP register a different type (in C++) so that SP is distinguishable in contexts where it's used. ...  Maybe on platforms where we don't care about the distinction we could typedef the SP as Register.

This is a winner.

Comment 4

[Lars T Hansen [:lth]]

Attachment: bug1425583-rollup.patch

This is clean but needs further testing (contains three changesets).

Comment 5

[Lars T Hansen [:lth]]

Attachment: bug1425583-avoid-getStackPointer.patch

Preparatory patch 1: We have nice abstractions that hide the use of the stack pointer, so let's use them.  This reduces the use of getStackPointer() and its return value, which we eventually will change.

Comment 6

[Lars T Hansen [:lth]]

Attachment: bug1425583-avoid-raw-MemOperand.patch

Preparatory patch 2: Translating an Address to a MemOperand will become more complicated shortly, so introduce an abstraction to handle the process.

Comment 7

[Lars T Hansen [:lth]]

Attachment: bug1425583-introduce-RegisterOrSP.patch

Make getStackPointer return a new type RegisterOrSP, and introduce changes here and there to handle SP specially.  By defining the new type as Register on platforms where the SP is simple to handle we avoid many changes.

After this change, there should be no chance that the arm64 back-end can confuse the zero register with the stack pointer; both cases where SP was forced into the same encoding as zero (getStackPointer and asUnsized) are gone, and the SP is strongly typed as something other than js::jit::Register when it appears in SpiderMonkey code (and of course Vixl already handles it properly).

Comment 8

[Lars T Hansen [:lth]]

Attachment: bug1425583-avoid-getStackPointer-v2.patch

This is the preparatory patch from comment 5, cleaned up a little: Removed an unused method and made a couple of methods more sophisticated when the SP is the actual machine SP - more architectural constraints apply then.

Comment 9

[Sean Stangl [:sstangl]]

Comment on attachment 8944231 [details] [diff] [review]
bug1425583-avoid-getStackPointer.patch

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

r+ with nits

::: js/src/jit/arm64/Trampoline-arm64.cpp
@@ +967,4 @@
>  
>          // Store return frame in lastProfilingFrame.
>          // scratch2 := masm.getStackPointer() + Descriptor.size*1 + JitFrameLayout::Size();
> +        masm.Add(ARMRegister(scratch2, 64), masm.GetStackPointer64(), ARMRegister(scratch1, 64));

The changes in this file don't seem to provide any benefit.

::: js/src/jit/x86/MacroAssembler-x86-inl.h
@@ +270,2 @@
>  {
> +    movePtr(getStackPointer(), dest);

moveStackPtrTo()

Comment 10

[Sean Stangl [:sstangl]]

Comment on attachment 8944370 [details] [diff] [review]
bug1425583-avoid-getStackPointer-v2.patch

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

Bah, reviewed an old version. This is the right version.

::: js/src/jit/arm64/MacroAssembler-arm64-inl.h
@@ +1841,5 @@
> +    if (sp.Is(GetStackPointer64())) {
> +        vixl::UseScratchRegisterScope temps(this);
> +        const ARMRegister scratch = temps.AcquireX();
> +        Mov(scratch, sp);
> +        And(sp, scratch, Operand(imm.value));

This needs to syncStackPtr() also, doesn't it?

::: js/src/jit/arm64/Trampoline-arm64.cpp
@@ +967,4 @@
>  
>          // Store return frame in lastProfilingFrame.
>          // scratch2 := masm.getStackPointer() + Descriptor.size*1 + JitFrameLayout::Size();
> +        masm.Add(ARMRegister(scratch2, 64), masm.GetStackPointer64(), ARMRegister(scratch1, 64));

addStackPtrTo()?

@@ +1002,4 @@
>      //
>      masm.bind(&handle_BaselineStub);
>      {
> +        masm.Add(ARMRegister(scratch3, 64), masm.GetStackPointer64(), ARMRegister(scratch1, 64));

addStackPtrTo()

@@ +1060,4 @@
>      masm.bind(&handle_Rectifier);
>      {
>          // scratch2 := StackPointer + Descriptor.size*1 + JitFrameLayout::Size();
> +        masm.Add(ARMRegister(scratch2, 64), masm.GetStackPointer64(), ARMRegister(scratch1, 64));

addStackPtrTo()

@@ +1127,4 @@
>      masm.bind(&handle_IonICCall);
>      {
>          // scratch2 := StackPointer + Descriptor.size + JitFrameLayout::Size()
> +        masm.Add(ARMRegister(scratch2, 64), masm.GetStackPointer64(), ARMRegister(scratch1, 64));

addStackPtrTo()

::: js/src/jit/x64/MacroAssembler-x64-inl.h
@@ +254,2 @@
>  {
> +    movePtr(getStackPointer(), dest);

moveStackPtrTo()

::: js/src/jit/x86/MacroAssembler-x86-inl.h
@@ +270,2 @@
>  {
> +    movePtr(getStackPointer(), dest);

moveStackPtrTo()

Comment 11

[Sean Stangl [:sstangl]]

Comment on attachment 8944232 [details] [diff] [review]
bug1425583-avoid-raw-MemOperand.patch

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

Great!

Comment 12

[Sean Stangl [:sstangl]]

Comment on attachment 8944233 [details] [diff] [review]
bug1425583-introduce-RegisterOrSP.patch

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

It looks like the wrong patch was attached here.

Comment 13

[Lars T Hansen [:lth]]

Attachment: bug1425583-introduce-RegisterOrSP-v2.patch

My bad; let's try again.  See comment 7 for information.

Comment 14

[Lars T Hansen [:lth]]

(In reply to Sean Stangl [:sstangl] from comment #10)
> Comment on attachment 8944370 [details] [diff] [review]
> bug1425583-avoid-getStackPointer-v2.patch
> 
> Review of attachment 8944370 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Bah, reviewed an old version. This is the right version.
> 
> ::: js/src/jit/arm64/MacroAssembler-arm64-inl.h
> @@ +1841,5 @@
> > +    if (sp.Is(GetStackPointer64())) {
> > +        vixl::UseScratchRegisterScope temps(this);
> > +        const ARMRegister scratch = temps.AcquireX();
> > +        Mov(scratch, sp);
> > +        And(sp, scratch, Operand(imm.value));
> 
> This needs to syncStackPtr() also, doesn't it?

No, because if GetStackPointer64() returns sp then syncStackPtr() is a no-op.  That is supposed to be an invariant, so I just assume it here and there.  I can however add a comment, since your point is a good one: it looks like a sync is missing.

> ::: js/src/jit/arm64/Trampoline-arm64.cpp
> @@ +967,4 @@
> >  
> >          // Store return frame in lastProfilingFrame.
> >          // scratch2 := masm.getStackPointer() + Descriptor.size*1 + JitFrameLayout::Size();
> > +        masm.Add(ARMRegister(scratch2, 64), masm.GetStackPointer64(), ARMRegister(scratch1, 64));
> 
> addStackPtrTo()?

There's not a three-operand version of addStackPtrTo(), and since so much other code in this file uses the arm64 opcodes directly it didn't seem worthwhile to introduce a three-operand addStackPtrTo(), no other platform will need it.  (Ditto the subsequent cases in the file.)

Opinions?  

> ::: js/src/jit/x64/MacroAssembler-x64-inl.h
> @@ +254,2 @@
> >  {
> > +    movePtr(getStackPointer(), dest);
> 
> moveStackPtrTo()

Right you are.

Comment 15

[Sean Stangl [:sstangl]]

Comment on attachment 8945781 [details] [diff] [review]
bug1425583-introduce-RegisterOrSP-v2.patch

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

A good change.

::: js/src/jit/Registers.h
@@ +181,5 @@
> +
> +static inline bool
> +IsSP(RegisterOrSP r)
> +{
> +    return false;

I'm worried this could be misleading, since it *could* be rsp, if it had the code for rsp.

Comment 16

[Lars T Hansen [:lth]]

(In reply to Sean Stangl [:sstangl] from comment #15)
> Comment on attachment 8945781 [details] [diff] [review]
> bug1425583-introduce-RegisterOrSP-v2.patch
> 
> Review of attachment 8945781 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> A good change.
> 
> ::: js/src/jit/Registers.h
> @@ +181,5 @@
> > +
> > +static inline bool
> > +IsSP(RegisterOrSP r)
> > +{
> > +    return false;
> 
> I'm worried this could be misleading, since it *could* be rsp, if it had the
> code for rsp.

Agreed, I will change the name of the predicate to "IsHiddenSP", which is truer to its purpose anyway.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b5f9febf1cf0
Rewrite gratuitous uses of getStackPointer(). r=sstangl
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba0b00ac1866
Hide ARM64 address computations behind an abstraction. r=sstangl
https://hg.mozilla.org/integration/mozilla-inbound/rev/0221723d8f6f
RegisterOrSP abstraction. r=sstangl

Comment 18

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/b5f9febf1cf0
https://hg.mozilla.org/mozilla-central/rev/ba0b00ac1866
https://hg.mozilla.org/mozilla-central/rev/0221723d8f6f