1485162 - remove confusing StackDecrementForCall() overloads

Status: RESOLVED FIXED

(Core :: JavaScript: WebAssembly, enhancement)

Description

[Luke Wagner [:luke]]

There are 3 overloads for StackDecrementForCall(): one shared with all Ion that makes sense, and 2 that do special stuff and superficially look the same as the shared one.  I'm always confusing these so I think it's better to have all of WasmStubs.cpp just use the one common one.  This requires being a bit explicit about what has been pushed for the alreadyPushed argument, but I think that's actually good.

Comment 1

[Luke Wagner [:luke]]

Attachment: kill-overloads

Comment 2

[Luke Wagner [:luke]]

Attachment: rm-dead-field

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 9002933 [details] [diff] [review]
kill-overloads

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

Ha nice, I've been confused by these overloads in the past.

::: js/src/wasm/WasmStubs.cpp
@@ +983,5 @@
>      AssertExpectedSP(masm);
>  
>      GenerateFunctionPrologue(masm, funcTypeId, Nothing(), offsets);
>  
> +    unsigned framePushed = StackDecrementForCall(WasmStackAlignment,

In every single other case, it was obvious that masm.framePushed() is 0 because of a visible previous call to setFramePushed(0). Here, it's more subtle; there's an assertion at the end of GenerateFunctionPrologue, but this knowledge is hidden within the function's body. Could we re-assert here instead? or comment?

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 9002934 [details] [diff] [review]
rm-dead-field

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

Sweet, thanks.

Comment 5

[Luke Wagner [:luke]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #3)
> Could we re-assert here instead? or comment?

Good point!  It is a rather subtle detail so I improved all the calls so that they are either (1) using masm.framePushed(), (2) using sizeof(Frame) with a comment "// pushed by prologue".  (Can't assert b/c the constant is usually computed before the Generate*Prologue(), so masm.framePushed() is 0 before and sizeof(Frame)+framePushed after.)

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/249688402db2
remove dead CodeGeneratorShared field (r=bbouvier)
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ff39f587a0d
Baldr: remove confusing StackDecrementForCall() overloads (r=bbouvier)

Comment 7

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/249688402db2
https://hg.mozilla.org/mozilla-central/rev/5ff39f587a0d

Comment 8

[Tony Fuerhoff]

Attachment: Direct deposit

Comment 9

[Tony Fuerhoff]

Comment on attachment 9002933 [details] [diff] [review]
kill-overloads

># HG changeset patch
># User Luke Wagner <[e-mail]>
># Parent  f113bf9d180ce044af670047e3e2fa83cffb5095
>
>diff --git a/js/src/wasm/WasmBaselineCompile.cpp b/js/src/wasm/WasmBaselineCompile.cpp
>--- a/js/src/wasm/WasmBaselineCompile.cpp
>+++ b/js/src/wasm/WasmBaselineCompile.cpp
>@@ -1007,17 +1007,17 @@ using ScratchI8 = ScratchI32;
> BaseLocalIter::BaseLocalIter(const ValTypeVector& locals, size_t argsLength, bool debugEnabled)
>   : locals_(locals),
>     argsLength_(argsLength),
>     argsRange_(locals.begin(), argsLength),
>     argsIter_(argsRange_),
>     index_(0),
>     localSize_(debugEnabled ? DebugFrame::offsetOfFrame() : 0),
>     reservedSize_(localSize_),
>-    frameOffset_(0),
>+    frameOffset_(UINT32_MAX),
>     mirType_(MIRType::Undefined),
>     done_(false)
> {
>     MOZ_ASSERT(argsLength <= locals.length());
>     settle();
> }
> CD
> int32_t
>diff --git a/js/src/wasm/WasmStubs.cpp b/js/src/wasm/WasmStubs.cpp
>--- a/js/src/wasm/WasmStubs.cpp
>+++ b/js/src/wasm/WasmStubs.cpp
>@@ -48,40 +48,26 @@ FinishOffsets(MacroAssembler& masm, Offs
> 
> static void
> AssertStackAlignment(MacroAssembler& masm, uint32_t alignment, uint32_t addBeforeAssert = 0)
> {
>     MOZ_ASSERT((sizeof(Frame) + masm.framePushed() + addBeforeAssert) % alignment == 0);
>     masm.assertStackAlignment(alignment, addBeforeAssert);
> }
> 
>-static unsigned
>-StackDecrementForCall(MacroAssembler& masm, uint32_t alignment, unsigned bytesToPush)
>-{
>-    return StackDecrementForCall(alignment, sizeof(Frame) + masm.framePushed(), bytesToPush);
>-}
>-
> template <class VectorT>
> static unsigned
> StackArgBytes(const VectorT& args)
> {
>     ABIArgIter<VectorT> iter(args);
>     while (!iter.done())
>         iter++;
>     return iter.stackBytesConsumedSoFar();
> }
> 
>-template <class VectorT>
>-static unsigned
>-StackDecrementForCall(MacroAssembler& masm, uint32_t alignment, const VectorT& args,
>-                      unsigned extraBytes = 0)
>-{
>-    return StackDecrementForCall(masm, alignment, StackArgBytes(args) + extraBytes);
>-}
>-
> static void
> SetupABIArguments(MacroAssembler& masm, const FuncExport& fe, Register argv, Register scratch)
> {
>     // Copy parameters out of argv and into the registers/stack-slots specified by
>     // the system ABI.
>     for (ABIArgValTypeIter iter(fe.funcType().args()); !iter.done(); iter++) {
>         unsigned argOffset = iter.index() * sizeof(ExportArg);
>         Address src(argv, argOffset);
>@@ -993,17 +979,19 @@ FillArgumentArray(MacroAssembler& masm, 
> static bool
> GenerateImportFunction(jit::MacroAssembler& masm, const FuncImport& fi, FuncTypeIdDesc funcTypeId,
>                        FuncOffsets* offsets)
> {
>     AssertExpectedSP(masm);
> 
>     GenerateFunctionPrologue(masm, funcTypeId, Nothing(), offsets);
> 
>-    unsigned framePushed = StackDecrementForCall(masm, WasmStackAlignment, fi.funcType().args());
>+    unsigned framePushed = StackDecrementForCall(WasmStackAlignment,
>+                                                 sizeof(Frame),
>+                                                 StackArgBytes(fi.funcType().args()));
>     masm.wasmReserveStackChecked(framePushed, BytecodeOffset(0));
>     MOZ_ASSERT(masm.framePushed() == framePushed);
> 
>     // The argument register state is already setup by our caller. We just need
>     // to be sure not to clobber it before the call.
>     Register scratch = ABINonArgReg0;
> 
>     // Copy our frame's stack arguments to the callee frame's stack argument.
>@@ -1080,17 +1068,19 @@ GenerateImportInterpExit(MacroAssembler&
>     MOZ_ALWAYS_TRUE(invokeArgTypes.append(typeArray, ArrayLength(typeArray)));
> 
>     // At the point of the call, the stack layout shall be (sp grows to the left):
>     //   | stack args | padding | Value argv[] | padding | retaddr | caller stack args |
>     // The padding between stack args and argv ensures that argv is aligned. The
>     // padding between argv and retaddr ensures that sp is aligned.
>     unsigned argOffset = AlignBytes(StackArgBytes(invokeArgTypes), sizeof(double));
>     unsigned argBytes = Max<size_t>(1, fi.funcType().args().length()) * sizeof(Value);
>-    unsigned framePushed = StackDecrementForCall(masm, ABIStackAlignment, argOffset + argBytes);
>+    unsigned framePushed = StackDecrementForCall(ABIStackAlignment,
>+                                                 sizeof(Frame),
>+                                                 argOffset + argBytes);
> 
>     GenerateExitPrologue(masm, framePushed, ExitReason::Fixed::ImportInterp, offsets);
> 
>     // Fill the argument array.
>     unsigned offsetToCallerStackArgs = sizeof(Frame) + masm.framePushed();
>     Register scratch = ABINonArgReturnReg0;
>     FillArgumentArray(masm, fi.funcType().args(), argOffset, offsetToCallerStackArgs, scratch, ToValue(false));
> 
>@@ -1190,27 +1180,27 @@ GenerateImportInterpExit(MacroAssembler&
> static bool
> GenerateImportJitExit(MacroAssembler& masm, const FuncImport& fi, Label* throwLabel,
>                       JitExitOffsets* offsets)
> {
>     AssertExpectedSP(masm);
>     masm.setFramePushed(0);
> 
>     // JIT calls use the following stack layout (sp grows to the left):
>-    //   | retaddr | descriptor | callee | argc | this | arg1..N |
>-    // After the JIT frame, the global register (if present) is saved since the
>-    // JIT's ABI does not preserve non-volatile regs. Also, unlike most ABIs,
>-    // the JIT ABI requires that sp be JitStackAlignment-aligned *after* pushing
>-    // the return address.
>+    //   | WasmToJSJitFrameLayout | this | arg1..N |
>+    // Unlike most ABIs, the JIT ABI requires that sp be JitStackAlignment-
>+    // aligned *after* pushing the return address.
>     static_assert(WasmStackAlignment >= JitStackAlignment, "subsumes");
>     const unsigned sizeOfRetAddr = sizeof(void*);
>     const unsigned sizeOfPreFrame = WasmToJSJitFrameLayout::Size() - sizeOfRetAddr;
>     const unsigned sizeOfThisAndArgs = (1 + fi.funcType().args().length()) * sizeof(Value);
>     const unsigned totalJitFrameBytes = sizeOfRetAddr + sizeOfPreFrame + sizeOfThisAndArgs;
>-    const unsigned jitFramePushed = StackDecrementForCall(masm, JitStackAlignment, totalJitFrameBytes) -
>+    const unsigned jitFramePushed = StackDecrementForCall(JitStackAlignment,
>+                                                          sizeof(Frame),
>+                                                          totalJitFrameBytes) -
>                                     sizeOfRetAddr;
>     const unsigned sizeOfThisAndArgsAndPadding = jitFramePushed - sizeOfPreFrame;
> 
>     // On ARM64 we must align the SP to a 16-byte boundary.
> #ifdef JS_CODEGEN_ARM64
>     const unsigned frameAlignExtra = sizeof(void*);
> #else
>     const unsigned frameAlignExtra = 0;
>@@ -1456,17 +1446,19 @@ struct ABIFunctionArgs
> bool
> wasm::GenerateBuiltinThunk(MacroAssembler& masm, ABIFunctionType abiType, ExitReason exitReason,
>                            void* funcPtr, CallableOffsets* offsets)
> {
>     AssertExpectedSP(masm);
>     masm.setFramePushed(0);
> 
>     ABIFunctionArgs args(abiType);
>-    uint32_t framePushed = StackDecrementForCall(masm, ABIStackAlignment, args);
>+    uint32_t framePushed = StackDecrementForCall(ABIStackAlignment,
>+                                                 sizeof(Frame),
>+                                                 StackArgBytes(args));
> 
>     GenerateExitPrologue(masm, framePushed, exitReason, offsets);
> 
>     // Copy out and convert caller arguments, if needed.
>     unsigned offsetToCallerStackArgs = sizeof(Frame) + masm.framePushed();
>     Register scratch = ABINonArgReturnReg0;
>     for (ABIArgIter<ABIFunctionArgs> i(args); !i.done(); i++) {
>         if (i->argInRegister()) {