1441142 - Stubs and frames for ARM64

Status: RESOLVED FIXED

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

Description

[Lars T Hansen [:lth]]

Changes to WasmFrameIterator and the Wasm stubs code to accomodate baseline on ARM64.

Comment 1

[Lars T Hansen [:lth]]

Attachment: bug1441142-wasm-frames-arm64.patch

Prologue, Epilogue, and Unwind code for ARM64, and padding for the Frame.  We can't use push and pop so the code offsets don't always make a ton of sense, and the unwind code therefore lumps some cases together.

Comment 2

[Lars T Hansen [:lth]]

Attachment: bug1441142-wasm-stubs-arm64.patch

Adapt the wasm stubs for ARM64.  The commit message has a fair amount of detail.

We might discuss whether there are good abstractions that could be factored further here, I've started with a few just to reduce the amount of ifdeffery but possibly more could be done.

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8954039 [details] [diff] [review]
bug1441142-wasm-frames-arm64.patch

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

Looks good, thanks. I've thought about the callable / jit entry cases and I didn't think there was anything wrong. Is the single-step instruction callback enabled for the ARM64 simulator? (If so, issues in this code would be caught in profiling tests)

::: js/src/wasm/WasmFrameIter.cpp
@@ +389,5 @@
> +
> +        *entry = masm.currentOffset();
> +
> +        masm.Sub(sp, sp, sizeof(Frame));
> +        masm.Str(ARMRegister(lr, 64), MemOperand(sp, offsetof(Frame, returnAddress)));

Can you do the PushedRetAddr assertion?

@@ +390,5 @@
> +        *entry = masm.currentOffset();
> +
> +        masm.Sub(sp, sp, sizeof(Frame));
> +        masm.Str(ARMRegister(lr, 64), MemOperand(sp, offsetof(Frame, returnAddress)));
> +        masm.Str(ARMRegister(WasmTlsReg, 64), MemOperand(sp, offsetof(Frame, tls)));

And the PushedTLS assertion here?

@@ +447,5 @@
>      masm.as_jr(ra);
>      masm.addToStackPtr(Imm32(sizeof(Frame)));
>  
> +#elif defined(JS_CODEGEN_ARM64)
> +

pre-existing nit: no \n before/after #elif

@@ +686,5 @@
> +#elif defined(JS_CODEGEN_ARM64)
> +        offsets->begin = masm.currentOffset();
> +        MOZ_ASSERT(BeforePushRetAddr == 0);
> +        // Subtract from SP first as SP must be aligned before offsetting.
> +        AutoForbidPools afp(&masm, /* number of instructions in scope = */ 3);

nit: for regularity with ARM, can you put it just after the preprocessing elif?

@@ +688,5 @@
> +        MOZ_ASSERT(BeforePushRetAddr == 0);
> +        // Subtract from SP first as SP must be aligned before offsetting.
> +        AutoForbidPools afp(&masm, /* number of instructions in scope = */ 3);
> +        masm.Sub(sp, sp, 8);
> +        masm.adjustFrame(8);

Can this call be put after the storePtr, so the actual instructions are close together, and the side effect isolated?

@@ +907,5 @@
> +        if (offsetFromEntry < PushedFP || codeRange->isThunk()) {
> +            // Constraints above ensure that this covers BeforePushRetAddr,
> +            // PushedRetAddr, and PushedTLS.
> +            //
> +            // On ARM64 we we subtract the size of the Frame from SP and then

nit: we we => "we need to" or just "we"

@@ +961,5 @@
> +                   offsetInCode <= codeRange->ret())
> +        {
> +            fixedPC = reinterpret_cast<Frame*>(sp)->returnAddress;
> +            fixedFP = fp;
> +            AssertMatchesCallSite(fixedPC, fixedFP);

Looks like this code could be shared with MIPS32/64, but since the epilogue generation is different, it's also cool to keep it separated.

::: js/src/wasm/WasmTypes.cpp
@@ +595,5 @@
> +#ifdef JS_CODEGEN_ARM64
> +    // This constraint may or may not be necessary.  If you hit this because
> +    // you've changed the frame size then feel free to remove it, but be extra
> +    // aware of possible problems.
> +    static_assert(sizeof(DebugFrame) % 16 == 0, "ARM64 SP alignment");

Another constraint for sizeof(Frame) is in WasmTypes.h, could we put them together in this file?

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8954040 [details] [diff] [review]
bug1441142-wasm-stubs-arm64.patch

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

Sorry, the number of unexpected whys in my review comments is too high, so I'd like to have another look once I get and understand the answers here.

::: js/src/jit/arm64/MacroAssembler-arm64.cpp
@@ +1394,5 @@
>      wasmStoreImpl(access, AnyRegister(), value, memoryBase, ptr, ptrScratch);
>  }
>  
> +void
> +MacroAssembler::enterFakeExitFrameForWasm(Register cxreg, Register scratch_, ExitFrameType type)

Since this is different only for ARM64, how about making it a static function in WasmStubs instead?

::: js/src/wasm/WasmStubs.cpp
@@ +257,5 @@
>                                                  NonVolatileRegs.fpus().getPushSizeInBytes();
>  #endif
> +
> +#if defined(JS_CODEGEN_ARM64)
> +static const unsigned FramePushedBeforeAlign = NonVolatileRegsPushSize + 2*sizeof(void*);

nit: spaces between *

Is it to align that you need an extra word here? if so, maybe comment?

@@ +263,5 @@
>  static const unsigned FramePushedBeforeAlign = NonVolatileRegsPushSize + sizeof(void*);
> +#endif
> +
> +static void
> +SaveReg(MacroAssembler& masm, Register r)

nit: if they're still needed despite the suggestion related to masm.SetStackPointer64, then I'd call these WasmPush / WasmPop, since they're still doing something equivalent to pushing/popping onto the stack, but using a different stack pointer.

@@ +287,5 @@
> +#endif
> +}
> +
> +static void
> +InitializeRegisterAssignment(MacroAssembler& masm)

The name makes me things that this does much more than what it actually does. Could we rename it `InitializeStackPointer`?

@@ +291,5 @@
> +InitializeRegisterAssignment(MacroAssembler& masm)
> +{
> +#ifdef JS_CODEGEN_ARM64
> +    // Wasm does not use the PseudoStackPointer.
> +    masm.SetStackPointer64(sp);

Couldn't masm.push et al just use the stackPointer which is set by this call? It would avoid a lot of boilerplate here...

@@ +296,5 @@
> +#endif
> +}
> +
> +static void
> +AssertRegisterAssignment(const MacroAssembler& masm)

Maybe just AssertABIStackPointer?

@@ +304,5 @@
> +#endif
> +}
> +
> +static void
> +SetupRegistersForJitABI(MacroAssembler& masm)

Maybe MoveAbiSpToJitSp? or something along these lines?

@@ +308,5 @@
> +SetupRegistersForJitABI(MacroAssembler& masm)
> +{
> +#ifdef JS_CODEGEN_ARM64
> +    // Conform to the Jit ABI by setting up x28 to have the same value as sp.
> +    masm.moveStackPtrTo(r28);

Would it be more explicit to use PseudoStackPointer here? (I think we'd not need the comment with this name)

@@ +326,5 @@
>      offsets->begin = masm.currentOffset();
>  
>      // Save the return address if it wasn't already saved by the call insn.
>  #ifdef JS_USE_LINK_REGISTER
> +# if defined(JS_CODEGEN_ARM)

nit: add MIPS32 and MIPS64 in this list

@@ +337,3 @@
>  #endif
>  
> +    unsigned oldFramePushed = masm.framePushed();

This is needed only because SaveReg will reserve the stack frame, but this is inconsistent with the `pushReturnAddress` call. Can you not set framePushed for this one push, instead, so we don't need oldFramePushed and this hack?

@@ +405,4 @@
>      // Call into the real function. Note that, due to the throw stub, fp, tls
>      // and pinned registers may be clobbered.
>      masm.assertStackAlignment(WasmStackAlignment);
> +    SetupRegistersForJitABI(masm);

Why do we need this here?

@@ +720,4 @@
>      // Call into the real function. Note that, due to the throw stub, fp, tls
>      // and pinned registers may be clobbered.
>      masm.assertStackAlignment(WasmStackAlignment);
> +    SetupRegistersForJitABI(masm);

Could it be done just a few lines above, when setting the other registers?

@@ +765,5 @@
>  
>      MOZ_ASSERT(masm.framePushed() == 0);
> +#ifdef JS_CODEGEN_ARM64
> +    masm.loadPtr(Address(sp, 0), lr);
> +    masm.addToStackPtr(Imm32(8));

Why?

@@ +767,5 @@
> +#ifdef JS_CODEGEN_ARM64
> +    masm.loadPtr(Address(sp, 0), lr);
> +    masm.addToStackPtr(Imm32(8));
> +    // Regenerate the PseudoStackPointer.
> +    masm.moveStackPtrTo(r28);

nit: use PseudoStackPointer here?

@@ +977,4 @@
>  
>      // Call the import exit stub.
>      CallSiteDesc desc(CallSiteDesc::Dynamic);
> +    SetupRegistersForJitABI(masm);

Why do we need this here?

@@ +1227,5 @@
>  
> +    AssertStackAlignment(masm, JitStackAlignment, sizeOfRetAddr + frameAlignExtra);
> +#ifdef JS_CODEGEN_ARM64
> +    // Conform to JIT ABI.
> +    masm.addToStackPtr(Imm32(8));

Why do we need both frameAlignExtra and this add?

@@ +1229,5 @@
> +#ifdef JS_CODEGEN_ARM64
> +    // Conform to JIT ABI.
> +    masm.addToStackPtr(Imm32(8));
> +#endif
> +    SetupRegistersForJitABI(masm);

Why?

@@ +1760,5 @@
>  #elif defined(JS_CODEGEN_ARM64)
> +    // Reserve aligned space to receive the saved LR and the new return address.
> +    const unsigned saveArea = 16;
> +    const unsigned LR_offset = 0;
> +    const unsigned PC_offset = 8;

nit: if these are constants, maybe use static constexpr and UPPERCASE_NAMES?

@@ +1780,5 @@
> +    MOZ_ASSERT(masm.framePushed() % 16 == 0);
> +
> +    // Save SP, APSR and FPSCR in non-volatile registers.
> +    masm.Mrs(x20, vixl::NZCV);
> +    masm.Mrs(x21, vixl::FPCR);

Can you add a comment that the flags should not have been touched by the instructions above, if that's true? (or hoist these two instructions, if that's false)

Comment 5

[Lars T Hansen [:lth]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #3)
> Comment on attachment 8954039 [details] [diff] [review]
> bug1441142-wasm-frames-arm64.patch
> 
> Review of attachment 8954039 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Is the single-step instruction callback enabled for the ARM64 simulator?

Not yet - that's bug 1442536, it requires some work on the simulator (plus enabling in other parts of the code).

> ::: js/src/wasm/WasmTypes.cpp
> @@ +595,5 @@
> > +#ifdef JS_CODEGEN_ARM64
> > +    // This constraint may or may not be necessary.  If you hit this because
> > +    // you've changed the frame size then feel free to remove it, but be extra
> > +    // aware of possible problems.
> > +    static_assert(sizeof(DebugFrame) % 16 == 0, "ARM64 SP alignment");
> 
> Another constraint for sizeof(Frame) is in WasmTypes.h, could we put them
> together in this file?

That does not seem natural, since Frame is defined in WasmTypes.h and DebugFrame is defined here (and the assert here is even in a DebugFrame method, not that that should matter).  IMO assertions about the types should live with their definitions when possible.

Comment 6

[Lars T Hansen [:lth]]

Comment on attachment 8954039 [details] [diff] [review]
bug1441142-wasm-frames-arm64.patch

I will assume Luke is OK to defer review to Benjamin.

Comment 7

[Lars T Hansen [:lth]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #4)
> Comment on attachment 8954040 [details] [diff] [review]
> bug1441142-wasm-stubs-arm64.patch
> 
> Review of attachment 8954040 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jit/arm64/MacroAssembler-arm64.cpp
> @@ +1394,5 @@
> >      wasmStoreImpl(access, AnyRegister(), value, memoryBase, ptr, ptrScratch);
> >  }
> >  
> > +void
> > +MacroAssembler::enterFakeExitFrameForWasm(Register cxreg, Register scratch_, ExitFrameType type)
> 
> Since this is different only for ARM64, how about making it a static
> function in WasmStubs instead?

I did not do that because masm.linkExitFrame(), which is needed from the ARM64 implementation, is private in the MacroAssembler and apparently not by accident.  Also there is already a function masm.enterFakeExitFrameForNative(), so there's an existing convention that I can follow.  On balance it seemed better to implement a new masm API.

Let me know what you prefer once you've considered the above; in the mean time I'll leave this unchanged.

(Rearranging some comments here for clarity)

> @@ +291,5 @@
> > +InitializeRegisterAssignment(MacroAssembler& masm)
> > +{
> > +#ifdef JS_CODEGEN_ARM64
> > +    // Wasm does not use the PseudoStackPointer.
> > +    masm.SetStackPointer64(sp);
> 
> Couldn't masm.push et al just use the stackPointer which is set by this call?

No, that's the whole point.  The SP register cannot be used for address generation unless it is 16-byte aligned when it is read, and as a consequence it hardly ever makes sense on ARM64 to push a sequence of single words as the existing prologue code does.  (And much of the Ion code, I might add.)  This is why the baseline jit has a double stack pointer, to avoid using the SP for memory addressing at all, so that code that assumes one can push and pop single words will continue to work.

We could change push() to, say, push 16 bytes and store 8 if the SP is the real SP, as WasmPush does here, but that seems like a very non-local change when what we need is a local adjustment to stubs code, for the time being.

> @@ +296,5 @@
> > +#endif
> > +}
> > +
> > +static void
> > +AssertRegisterAssignment(const MacroAssembler& masm)
> 
> Maybe just AssertABIStackPointer?

How about AssertExpectedSP (I changed InitializeRegisterAssignment to InitializeSP).

> @@ +304,5 @@
> > +#endif
> > +}
> > +
> > +static void
> > +SetupRegistersForJitABI(MacroAssembler& masm)
> 
> Maybe MoveAbiSpToJitSp? or something along these lines?

How about MoveSPForJitABI.

> @@ +308,5 @@
> > +SetupRegistersForJitABI(MacroAssembler& masm)
> > +{
> > +#ifdef JS_CODEGEN_ARM64
> > +    // Conform to the Jit ABI by setting up x28 to have the same value as sp.
> > +    masm.moveStackPtrTo(r28);
> 
> Would it be more explicit to use PseudoStackPointer here?

Yeah, good point, I caught this a couple places but it's good to get them all.

> @@ +405,4 @@
> >      // Call into the real function. Note that, due to the throw stub, fp, tls
> >      // and pinned registers may be clobbered.
> >      masm.assertStackAlignment(WasmStackAlignment);
> > +    SetupRegistersForJitABI(masm);
> 
> Why do we need this here?

I think it is not needed (ditto at the next item), I was just too eager when I was hunting down a bug.

> @@ +720,4 @@
> >      // Call into the real function. Note that, due to the throw stub, fp, tls
> >      // and pinned registers may be clobbered.
> >      masm.assertStackAlignment(WasmStackAlignment);
> > +    SetupRegistersForJitABI(masm);
> 
> Could it be done just a few lines above, when setting the other registers?

I suspect it does not need to be done at all, as above.  Investigating.

> @@ +765,5 @@
> >  
> >      MOZ_ASSERT(masm.framePushed() == 0);
> > +#ifdef JS_CODEGEN_ARM64
> > +    masm.loadPtr(Address(sp, 0), lr);
> > +    masm.addToStackPtr(Imm32(8));
> 
> Why?

Because the JIT ABI is to call with a mis-aligned stack.  Earlier, GenerateJitEntryPrologue has subtracted 8 from the SP and then stored LR into that slot (and the SP is aligned at that point).  Here we do the opposite.

> @@ +977,4 @@
> >  
> >      // Call the import exit stub.
> >      CallSiteDesc desc(CallSiteDesc::Dynamic);
> > +    SetupRegistersForJitABI(masm);
> 
> Why do we need this here?

Because the import function is generated by the baseline jit and it expects the PSP to have the correct value on entry (and also when one returns to it from a call).
  
> @@ +1227,5 @@
> >  
> > +    AssertStackAlignment(masm, JitStackAlignment, sizeOfRetAddr + frameAlignExtra);
> > +#ifdef JS_CODEGEN_ARM64
> > +    // Conform to JIT ABI.
> > +    masm.addToStackPtr(Imm32(8));
> 
> Why do we need both frameAlignExtra and this add?

Those have different, if related, purposes.

Throughout this function the SP is aligned, as a result of allocating the extra word (frameAlignExtra) in the prologue.  We then account for this extra word in assertions and so on.  But before we make the call we must mis-align the SP for the JIT ABI and then set up the PSP.

> @@ +1229,5 @@
> > +#ifdef JS_CODEGEN_ARM64
> > +    // Conform to JIT ABI.
> > +    masm.addToStackPtr(Imm32(8));
> > +#endif
> > +    SetupRegistersForJitABI(masm);
> 
> Why?

Because the BaselineJIT-compiled code requires the PSP to have the correct value.

Comment 8

[Lars T Hansen [:lth]]

Comment on attachment 8954040 [details] [diff] [review]
bug1441142-wasm-stubs-arm64.patch

Cancelling review, new patch coming.

Comment 9

[Lars T Hansen [:lth]]

bbouvier, ni? re question about MacroAssembler, see top of comment 7.

Comment 10

[Benjamin Bouvier [:bbouvier] (inactive)]

Sounds fine as is (leaving unchanged).

Comment 11

[Lars T Hansen [:lth]]

Attachment: bug1441142-wasm-stubs-arm64-v2.patch

I think this addresses all concerns in the previous review (except one thing about MIPS that I prefer them to fix themselves).  The important thing to remember is that the ARM64 SP (doubleword aligned) is in a sense fundamentally incompatible with the JIT calling conventions (intentionally not doubleword aligned), and so there are some gyrations to make that fit at the interface with JIT code; obviously I'm happy to add more comments if that's what it takes to make this more transparent.

Comment 12

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8957492 [details] [diff] [review]
bug1441142-wasm-stubs-arm64-v2.patch

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

Great work, thanks. Excited to have ARM64 working on the baseline compiler.

::: js/src/jit/arm64/MacroAssembler-arm64.cpp
@@ +1382,5 @@
>      wasmStoreImpl(access, AnyRegister(), value, memoryBase, ptr, ptrScratch);
>  }
>  
> +void
> +MacroAssembler::enterFakeExitFrameForWasm(Register cxreg, Register scratch_, ExitFrameType type)

nit: can you avoid the trailing _ in parameter name, please? It's confusing because we use _ as a hint for "member" in so many places in Spidermonkey. (Maybe the `scratch` names before could be renamed `armScratch`)

::: js/src/wasm/WasmStubs.cpp
@@ +258,5 @@
>  #endif
> +
> +#if defined(JS_CODEGEN_ARM64)
> +// Stacks are 16-byte aligned, hence the extra word.
> +static const unsigned FramePushedBeforeAlign = NonVolatileRegsPushSize + 2 * sizeof(void*);

Can the comment above be static_assert'd?

@@ +264,5 @@
>  static const unsigned FramePushedBeforeAlign = NonVolatileRegsPushSize + sizeof(void*);
> +#endif
> +
> +void
> +js::wasm::InitMacroAssemblerForWasmStubsAndBuiltins(MacroAssembler& masm)

Since this always happens after the creation of the MacroAssembler, and the MacroAssembler already uses a token to indicate it's a wasm masm, could we just put this code under the MacroAssembler ctor itself?

@@ +423,5 @@
>      masm.freeStack(argDecrement);
>  
>      // Pop the stack pointer to its value right before dynamic alignment.
> +#ifdef JS_CODEGEN_ARM64
> +    static_assert(WasmStackAlignment == 16, "ARM64 SP alignment");

Since this static assert is redundant, maybe we could just put it as a comment here?

@@ +1184,5 @@
> +    const unsigned sizeOfThisAndArgs = (1 + fi.sig().args().length()) * sizeof(Value);
> +    const unsigned totalJitFrameBytes = sizeOfRetAddr + sizeOfPreFrame + sizeOfThisAndArgs;
> +    const unsigned jitFramePushed = StackDecrementForCall(masm, JitStackAlignment, totalJitFrameBytes) -
> +                                    sizeOfRetAddr;
> +    const unsigned sizeOfThisAndArgsAndPadding = jitFramePushed - sizeOfPreFrame;

Did adding all the consts over here made it clearer for you that these values never change within this function? Asking because we usually don't do this (and it increases line size :p), but it'd be fine to keep.

@@ +1801,5 @@
> +    masm.Mrs(x20, vixl::NZCV);
> +    masm.Mrs(x21, vixl::FPCR);
> +    masm.Mov(x22, sp);
> +
> +    // The stack is already aligned

nit: end with .

Comment 13

[Lars T Hansen [:lth]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #12)
> Comment on attachment 8957492 [details] [diff] [review]
> bug1441142-wasm-stubs-arm64-v2.patch
> 
> Review of attachment 8957492 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/wasm/WasmStubs.cpp
> @@ +258,5 @@
> >  #endif
> > +
> > +#if defined(JS_CODEGEN_ARM64)
> > +// Stacks are 16-byte aligned, hence the extra word.
> > +static const unsigned FramePushedBeforeAlign = NonVolatileRegsPushSize + 2 * sizeof(void*);
> 
> Can the comment above be static_assert'd?

It's *possible* that it can be done if I sprinke constexpr deeply enough throughout the JIT but I don't think I'm up for this today.  (Really we should probably be having a longer discussion about constexprs and constexpr methods; they're great when they work and are excellent to safeguard performance but are sometimes at odds with making code work...)  There are assertions in the ARM64 back-end that will catch the problem if it turns out not to be the case (because then we cannot push the nonvolatiles properly because we end up with an unaligned SP) so we're not flying blind.  I'll probably just let this be.

> @@ +264,5 @@
> >  static const unsigned FramePushedBeforeAlign = NonVolatileRegsPushSize + sizeof(void*);
> > +#endif
> > +
> > +void
> > +js::wasm::InitMacroAssemblerForWasmStubsAndBuiltins(MacroAssembler& masm)
> 
> Since this always happens after the creation of the MacroAssembler, and the
> MacroAssembler already uses a token to indicate it's a wasm masm, could we
> just put this code under the MacroAssembler ctor itself?

Sweet, seems to work well, and reduces complexity everywhere.

> @@ +1184,5 @@
> > +    const unsigned sizeOfThisAndArgs = (1 + fi.sig().args().length()) * sizeof(Value);
> > +    const unsigned totalJitFrameBytes = sizeOfRetAddr + sizeOfPreFrame + sizeOfThisAndArgs;
> > +    const unsigned jitFramePushed = StackDecrementForCall(masm, JitStackAlignment, totalJitFrameBytes) -
> > +                                    sizeOfRetAddr;
> > +    const unsigned sizeOfThisAndArgsAndPadding = jitFramePushed - sizeOfPreFrame;
> 
> Did adding all the consts over here made it clearer for you that these
> values never change within this function? Asking because we usually don't do
> this (and it increases line size :p), but it'd be fine to keep.

I think so - I was being paranoid at one point and was trying to ensure that nothing would change.  I can revert.  Sadly c++ is broken in this regard, obviously mutability of locals should be opt-in.

Comment 14

[Lars T Hansen [:lth]]

(In reply to Lars T Hansen [:lth] from comment #13)
> (In reply to Benjamin Bouvier [:bbouvier] from comment #12)
> > Comment on attachment 8957492 [details] [diff] [review]
> > bug1441142-wasm-stubs-arm64-v2.patch
> > 
> > Review of attachment 8957492 [details] [diff] [review]:
> > -----------------------------------------------------------------

> > @@ +1184,5 @@
> > > +    const unsigned sizeOfThisAndArgs = (1 + fi.sig().args().length()) * sizeof(Value);
> > > +    const unsigned totalJitFrameBytes = sizeOfRetAddr + sizeOfPreFrame + sizeOfThisAndArgs;
> > > +    const unsigned jitFramePushed = StackDecrementForCall(masm, JitStackAlignment, totalJitFrameBytes) -
> > > +                                    sizeOfRetAddr;
> > > +    const unsigned sizeOfThisAndArgsAndPadding = jitFramePushed - sizeOfPreFrame;
> > 
> > Did adding all the consts over here made it clearer for you that these
> > values never change within this function? Asking because we usually don't do
> > this (and it increases line size :p), but it'd be fine to keep.
> 
> I think so - I was being paranoid at one point and was trying to ensure that
> nothing would change.  I can revert.  Sadly c++ is broken in this regard,
> obviously mutability of locals should be opt-in.

Actually some of them are required to make a static_assert work, so I'll just leave them all in.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/62541e869aa2
Adapt wasm frame code for ARM64.  r=bbouvier
https://hg.mozilla.org/integration/mozilla-inbound/rev/40d0f78013ad
Adapt wasm stubs for ARM64.  r=bbouvier

Comment 16

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/62541e869aa2
https://hg.mozilla.org/mozilla-central/rev/40d0f78013ad