1147629 - Access the StackPointer indirectly in shared code

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Sean Stangl [:sstangl]]

Attachment: patch

This is preparation for landing the ARM64 branch.

On ARM64, there are two problems in dealing with the StackPointer:

1. The StackPointer can only be used as the base register of an address when it is 16-byte aligned. In practice this means that we have two registers representing the StackPointer: one that represents where the JIT thinks the stack ends (unaligned), and one that represents where the hardware thinks the stack ends (16-byte aligned).

To keep the two registers in sync, we have to create a bunch of sync points. This is annoying to do in shared code, because this isn't a problem you have to think of in other architectures.

Instead of inserting sync code, it is easier to make *StackPtr* functions of common MacroAssembler routines. Non-ARM64 implementations can pass-through, while the ARM64 versions sync where necessary.

2. Depending on context, the StackPointer can be read as all zeroes (sp aliases xzr).

The *StackPtr* functions handle this properly.

To prevent further accidental misuse of the StackPointer, ARM64 doesn't define StackPointer. The function masm.GetStackPointer() (non-static on ARM64, inherited from VIXL) returns the active StackPointer for the current context.

Comment 1

[Luke Wagner [:luke]]

FWIW, from irc, I was expressing my hope that, irregxep aside (where we apparently want to do a lot of sub-16-byte push/pop), it seems desirable to instead make Push()/Pop() work on arm64 (by always pushing 16 bytes) and then try to avoid using Push()/Pop() on any hot paths.

Comment 2

[Jan de Mooij [:jandem]]

Sean, any thoughts on Luke's suggestion to keep the stack pointer 16 byte aligned and change push/pop?

After nbp's stack alignment work the stack pointer should be 16-byte aligned in a lot of cases, maybe that'd help here too.

How do other compilers/JITs deal with this problem?

Comment 3

[Nicolas B. Pierron [:nbp]]

(In reply to Jan de Mooij [:jandem] from comment #2)
> After nbp's stack alignment work the stack pointer should be 16-byte aligned
> in a lot of cases, maybe that'd help here too.

On x86 and x64, it is still 8 bytes aligned on ARMv7 and MIPSn32.

Comment 4

[Sean Stangl [:sstangl]]

(In reply to Jan de Mooij [:jandem] from comment #2)
> Sean, any thoughts on Luke's suggestion to keep the stack pointer 16 byte
> aligned and change push/pop?

This works well for code that has a fixed frame size, which should be everything but irregexp. A little extra memory will be used at call sites.

While I like the suggestion, and experimenting with that was in the plan from the beginning, I'm not particularly excited about the idea of making the change at this particular time, when I'm hoping to get the patches in-tree ASAP to prevent constant rebasing. Could we do it later?

> How do other compilers/JITs deal with this problem?

v8's ARM64 port is similar to the implementation we currently have: they have three stack pointers (sp, csp, jssp), the latter two of which both alias to x28 as is suggested by VIXL.

Comment 5

[Jan de Mooij [:jandem]]

Comment on attachment 8583382 [details] [diff] [review]
patch

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

Sorry for the delay. This seems fine for now to get things landed, we can revisit later if it's an issue.

r=me with one request: can we rename masm.GetStackPointer() to masm.getStackPointer()? If vixl uses GetStackPointer we could either rename it or add a getStackPointer alias. I think it's less of an issue in jit/arm64 but it'd be nice if shared code followed the SpiderMonkey style as much as possible. Technically these functions are static on platforms other than ARM64, but they are called as non-static methods so getStackPointer is more natural.

Comment 6

[Sean Stangl [:sstangl]]

Thanks, renamed.

https://hg.mozilla.org/integration/mozilla-inbound/rev/77b3cc5607cd

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/77b3cc5607cd