1419034 - Wasm baseline: Refactor stack frame management

Status: RESOLVED FIXED

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

Description

[Lars T Hansen [:lth]]

The stack frame logic - including the indiscrimiate use of StackPointer - is ripe for cleanup, this will reduce the complexity of BaseCompiler for one thing but importantly make it possible to experiment with and use a slightly different stack strategy on ARM64 where the frame has to be aligned on a 16-byte boundary.

There are probably several strategies that would work on ARM64.  The most appealing seems to be a chunked stack, where we have a real stack pointer that advances in chunk increments (say, 32 bytes), but where we also keep track of the stack height that allows there to be some free space at the top of the stack.  At least one sample patch will be provided here to motivate the rewrite.

Comment 1

[Lars T Hansen [:lth]]

Attachment: Bug 1419034 - wasm baseline, factor out the stack frame logic.

This patch does several interconnected things with Rabaldr's stack
frame.

First, it abstracts the stack frame logic from BaseCompiler into
BaseStackFrame; BaseCompiler now only knows very little about the
frame, and what it is allowed to know is documented (mostly that there
is a static part and a dynamic part and that masm.framePushed() is
always correct).  In particular, BaseCompiler must use BaseStackFrame
to push and pop values from its value stack, it can't use masm
directly.  This abstraction is important for ARM64, where we can't
freely push and pop, because SP must be aligned on a 16-byte boundary.
BaseStackFrame is also in charge of allocating locals to frame slots
and to access the locals.  Crucially, these abstractions do not
require us to use a frame pointer.

Second, it removes all uses of the StackPointer variable (not defined
on ARM64) and most uses of masm.getStackPointer() from BaseCompiler;
the few that are left are well-defined cross-platform and are not
sensitive to the layout of the frame, only its size.

Third, it makes BaseLocalIter the property of BaseStackFrame (but
defined outside the latter since the iter class is accessed
externally).

I will also upload a patch that illustrates how this machinery is
adapted for ARM64, but I don't want to land that patch quite yet.

Review commit: https://reviewboard.mozilla.org/r/201980/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/201980/

Comment 2

[Lars T Hansen [:lth]]

Attachment: bug1419034-chunky-stack-wip.patch

This is the proof-of-concept for the ARM64-compatible stack management mentioned in the review comment on the other patch, it is here only to illustrate what we can do with the abstraction.

Comment 3

[Code Review Bot [:reviewbot]]

Comment on attachment 8930874 [details]
Bug 1419034 - wasm baseline, factor out the stack frame logic.

https://reviewboard.mozilla.org/r/201980/#review207372


C/C++ static analysis found 1 defect in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`


::: js/src/wasm/WasmBaselineCompile.cpp:879
(Diff revision 1)
> +    // The stack pointer, cached for brevity.
> +    Register        sp_;
> +
> +  public:
> +
> +    BaseStackFrame(MacroAssembler& masm)

Error: Bad implicit conversion constructor for 'basestackframe' [clang-tidy: mozilla-implicit-constructor]

Comment 4

[Lars T Hansen [:lth]]

Comment on attachment 8930874 [details]
Bug 1419034 - wasm baseline, factor out the stack frame logic.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/201980/diff/1-2/

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8930874 [details]
Bug 1419034 - wasm baseline, factor out the stack frame logic.

https://reviewboard.mozilla.org/r/201980/#review208524

Looks nicer. Mostly nits here and a few suggestions, so r+. Sorry for the slow review time (that being said, this could have been at least 3 patches which would have made it even faster to review: code motion, new structure, random fixups)). Thanks!

::: commit-message-45608:28
(Diff revision 2)
> +Third, it makes BaseLocalIter the property of BaseStackFrame (but
> +defined outside the latter since the iter class is accessed
> +externally).
> +
> +I will also upload a patch that illustrates how this machinery is
> +adapted for ARM64, but I don't want to land that patch quite yet.

Don't forget about updating the changeset message once you're done.

::: js/src/wasm/WasmBaselineCompile.cpp:695
(Diff revision 2)
> +
> +    operator RegI32() const {
> +# ifdef JS_CODEGEN_X86
> +        return RegI32(ScratchRegX86);
> +# else
> +        return RegI32(ScratchRegARM);

Just an idea: instead of `ScratchRegARM` vs `ScratchRegX86`, we could have a single definition of `BaseScratchReg` that's defined per platform. A single section could contain all the per-platform registers, resulting in fewer per-platform ifdefs.

::: js/src/wasm/WasmBaselineCompile.cpp:703
(Diff revision 2)
> +};
> +#else
> +class ScratchI32 : public ScratchRegisterScope
> +{
> +  public:
> +    explicit ScratchI32(MacroAssembler& m) : ScratchRegisterScope(m) {}

Is it expected that under x86 and ARM the ctor takes a BaseRegAlloc& while it takes a MacroAssembler& otherwise?

::: js/src/wasm/WasmBaselineCompile.cpp:710
(Diff revision 2)
> +};
> +#endif
> +
> +#if defined(JS_CODEGEN_X86)
> +// ScratchEBX is a mnemonic device: For some atomic ops we really need EBX,
> +// no other register will do.  And we would normally have to allocate that

pre-existing nit: registers

::: js/src/wasm/WasmBaselineCompile.cpp:715
(Diff revision 2)
> +// no other register will do.  And we would normally have to allocate that
> +// register using ScratchI32 since normally the scratch register is EBX.
> +// But the whole point of ScratchI32 is to hide that relationship.  By using
> +// the ScratchEBX alias, we document that at that point we require the
> +// scratch register to be EBX.
> +typedef ScratchI32 ScratchEBX;

nit: i suggest we start using more recent C++ and write it as `using ScratchEBX = ScratchI32;`

::: js/src/wasm/WasmBaselineCompile.cpp:720
(Diff revision 2)
> +typedef ScratchI32 ScratchEBX;
> +#endif
> +
> +// The stack frame.
> +//
> +// The frame has four parts: the Header, comprising the Frame and DebugFrame

nit: can you make this a list, with one element per line, please?

::: js/src/wasm/WasmBaselineCompile.cpp:721
(Diff revision 2)
> +#endif
> +
> +// The stack frame.
> +//
> +// The frame has four parts: the Header, comprising the Frame and DebugFrame
> +// elements; the Local area, allocated below the header with various forms of

maybe worth to precise what "below" means, i.e. stack growing downwards?

::: js/src/wasm/WasmBaselineCompile.cpp:753
(Diff revision 2)
> +BaseLocalIter::BaseLocalIter(const ValTypeVector& locals,
> +                             size_t argsLength,
> +                             bool debugEnabled)
> +  : locals_(locals),
> +    argsLength_(argsLength),
> +    argsRange_(locals.begin(), argsLength),

considering how it's used, should `argsLength` be named `numArgs` instead?

::: js/src/wasm/WasmBaselineCompile.cpp:772
(Diff revision 2)
> +{
> +    if (nbytes == 8)
> +        localSize_ = AlignBytes(localSize_, 8u);
> +    else if (nbytes == 16)
> +        localSize_ = AlignBytes(localSize_, 16u);
> +    localSize_ += nbytes;

Maybe simply `localSize_ = AlignBytes(localSize_, nbytes) + nbytes;` ? Would also work for less than 8 bytes allocations.

::: js/src/wasm/WasmBaselineCompile.cpp:785
(Diff revision 2)
> +        MOZ_ASSERT(!argsIter_.done());
> +        mirType_ = argsIter_.mirType();
> +        switch (mirType_) {
> +          case MIRType::Int32:
> +            if (argsIter_->argInRegister())
> +                frameOffset_ = pushLocal(4);

if we had a helper like MIRTypeToLength in jit/IonTypes.h, this switch could be much more compact. What do you think of adding it?

::: js/src/wasm/WasmBaselineCompile.cpp:787
(Diff revision 2)
> +        switch (mirType_) {
> +          case MIRType::Int32:
> +            if (argsIter_->argInRegister())
> +                frameOffset_ = pushLocal(4);
> +            else
> +                frameOffset_ = -(argsIter_->offsetFromArgBase() + sizeof(Frame));

Would it work with a debug frame? I assume yes, otherwise tests would have fallen, so: how does it work? (does `sizeof(Frame) == sizeof(DebugFrame)` maybe?) If no, could we make a test for this?

::: js/src/wasm/WasmBaselineCompile.cpp:829
(Diff revision 2)
> +          case ValType::F64:
> +            mirType_ = jit::MIRType::Double;
> +            frameOffset_ = pushLocal(8);
> +            break;
> +          case ValType::I64:
> +            mirType_ = jit::MIRType::Int64;

There's probably a helper to do ValType to MIRType, and another to do ValType to length, so this switch can be much more concise.

::: js/src/wasm/WasmBaselineCompile.cpp:859
(Diff revision 2)
> +class BaseStackFrame
> +{
> +    MacroAssembler& masm;
> +
> +    // Size of local area in bytes (stable after beginFunction).
> +    uint32_t        localSize_;

(fwiw, I'm fine with not having attribute name not all vertically aligned, and in this particular case I'd find it less ugly to have them not aligned)

::: js/src/wasm/WasmBaselineCompile.cpp:868
(Diff revision 2)
> +
> +    // High byte offset + 1 of local area for true locals.
> +    uint32_t        varHigh_;
> +
> +    // The largest stack height, not necessarily zero-based.  Read this for its
> +    // true value only when code generation is finished.

Would it be useful to have a DEBUG only boolean codegenIsFinished, that's asserted when trying to read the stack height?

::: js/src/wasm/WasmBaselineCompile.cpp:915
(Diff revision 2)
> +    typedef Vector<Local, 8, SystemAllocPolicy> LocalVector;
> +
> +  private:
> +
> +    // Offset off of sp_ for `local`.
> +    int32_t localOffset(Local& local) {

nit: `const Local&`

::: js/src/wasm/WasmBaselineCompile.cpp:934
(Diff revision 2)
> +        MOZ_ASSERT(localSize_ % WasmStackAlignment == 0);
> +        maxStackHeight_ = localSize_;
> +    }
> +
> +    // Initialize `localInfo` based on the types of `locals` and `args`.
> +    bool setupLocals(LocalVector& localInfo, const ValTypeVector& locals, const ValTypeVector& args,

nit: as discussed in other bug, LocalVector should be \*, not &

Also since this is an in-out parameter, can it be put at the end of the parameter list?

::: js/src/wasm/WasmBaselineCompile.cpp:949
(Diff revision 2)
> +        varLow_ = i.reservedSize();
> +        for (; !i.done() && i.index() < args.length(); i++) {
> +            MOZ_ASSERT(i.isArg());
> +            MOZ_ASSERT(i.index() == index);
> +            localInfo.infallibleEmplaceBack(i.mirType(), i.frameOffset());
> +            varLow_ = i.currentLocalSize();

Can this be put outside the loop body, just after it?

::: js/src/wasm/WasmBaselineCompile.cpp:958
(Diff revision 2)
> +        varHigh_ = varLow_;
> +        for (; !i.done() ; i++) {
> +            MOZ_ASSERT(!i.isArg());
> +            MOZ_ASSERT(i.index() == index);
> +            localInfo.infallibleEmplaceBack(i.mirType(), i.frameOffset());
> +            varHigh_ = i.currentLocalSize();

ditto

::: js/src/wasm/WasmBaselineCompile.cpp:979
(Diff revision 2)
> +        return localSize_;
> +    }
> +
> +    void zeroLocals(BaseRegAlloc& ra);
> +
> +    void loadLocalI32(RegI32 r, Local& local) {

could it be `const Local& local`? (and ditto for all the other instances below)

also, would it make sense to have the destination be the second paramter? (or is that handled by one of the many other patches)

::: js/src/wasm/WasmBaselineCompile.cpp:1009
(Diff revision 2)
> +
> +    void loadLocalF32(RegF32 r, Local& local) {
> +        masm.loadFloat32(Address(sp_, localOffset(local)), r);
> +    }
> +
> +    void storeLocalI32(RegI32 r, Local& local) {

nit: for these ones the order of arguments looks as expected, but `const Local&` for the second one please

::: js/src/wasm/WasmBaselineCompile.cpp:1036
(Diff revision 2)
> +    // The stack area - the dynamic part of the frame.
> +
> +  private:
> +
> +    // Offset off of sp_ for the slot at stack area location `offset`
> +    int32_t stackOffset(int32_t offset) {

Why do we need two methods for `stackOffset` and `localOffset` that do the same thing? Maybe there could be only one, or `localOffset` would call `stackOffset`?

::: js/src/wasm/WasmBaselineCompile.cpp:1057
(Diff revision 2)
> +#ifdef JS_CODEGEN_ARM
> +    static const size_t StackSizeOfFloat  = sizeof(float);
> +#else
> +    static const size_t StackSizeOfFloat  = sizeof(double);
> +#endif
> +    static const size_t StackSizeOfDouble = sizeof(double);

If you follow the suggestion to have the registers definition grouped outside, to have all platform specific code, maybe these definitions could go in this area too.

::: js/src/wasm/WasmBaselineCompile.cpp:1064
(Diff revision 2)
> +    // We won't know until after we've generated code how big the frame will
> +    // be (we may need arbitrary spill slots and outgoing param slots) so
> +    // emit a patchable add that is patched in endFunction().
> +    //
> +    // ScratchReg may be used by branchPtr(), so use ABINonArgReg0/1 for
> +    // temporaries.

So as a user of this API, I have to make sure that I'm in a place where ABINonArgReg0/1 are not live. Can we instead pass two scratch registers to this function call, to make this responsibility more apparent in the call site? Or can you suffix or prefix this function with "prologue" to make the constraint slightly more visible?

::: js/src/wasm/WasmBaselineCompile.cpp:1067
(Diff revision 2)
> +    //
> +    // ScratchReg may be used by branchPtr(), so use ABINonArgReg0/1 for
> +    // temporaries.
> +
> +    void allocStack(Label* stackOverflowLabel) {
> +        stackAddOffset_ = masm.add32ToPtrWithPatch(masm.getStackPointer(), ABINonArgReg0);

nit: sp_

::: js/src/wasm/WasmBaselineCompile.cpp:1088
(Diff revision 2)
> +    uint32_t stackHeight() const {
> +        return masm.framePushed();
> +    }
> +
> +    // Set the frame height.  This must only be called with a value returned
> +    // from stackHeight().

To statically enforce the constraint (and remove the comment), maybe have a `struct StackHeight` that just wraps an uint32_t, have `stackHeight()` return a struct of this type, and `setStackHeight` take a struct of this type by arg?

::: js/src/wasm/WasmBaselineCompile.cpp:1093
(Diff revision 2)
> +    // from stackHeight().
> +    void setStackHeight(uint32_t amount) {
> +        masm.setFramePushed(amount);
> +    }
> +
> +    uint32_t pushPtr(Register r) {

Why not having pushI32 as an alias to pushPtr? Reading code much below was confusing because e.g. an i64 is by definition two i32, not two ptrs (even if it's coincidentally true on 32 bits platforms).

::: js/src/wasm/WasmBaselineCompile.cpp:1101
(Diff revision 2)
> +        maxStackHeight_ = Max(maxStackHeight_, stackHeight());
> +        MOZ_ASSERT(stackBefore + StackSizeOfPtr == stackHeight());
> +        return stackHeight();
> +    }
> +
> +    uint32_t pushFloat(FloatRegister r) {

nit: I'd suggest `pushFloat32` instead, since float could as well designate a double after all. (I've done this mistake in the past to name `float` things that were related to `float32`, and it was pretty inconvenient, because then at some point we added methods that would work for both float32 and double, and these were called `floatingPoint` or something, making all of this confusing; we have an opportunity to avoid this mistake here)

::: js/src/wasm/WasmBaselineCompile.cpp:1142
(Diff revision 2)
> +            masm.freeStack(bytes);
> +    }
> +
> +    // Before branching to an outer control label, pop the execution
> +    // stack to the level expected by that region, but do not free the
> +    // stack as that will happen as compilation leaves the block.

I guess this comment means to not update masm.framePushed\_, maybe it could be more eloquent in saying it so?

::: js/src/wasm/WasmBaselineCompile.cpp:1152
(Diff revision 2)
> +            masm.addToStackPtr(Imm32(stackHere - destStackHeight));
> +    }
> +
> +    bool willPopStackBeforeBranch(uint32_t destStackHeight) {
> +        uint32_t stackHere = stackHeight();
> +        return stackHere > destStackHeight;

nit: the temporary looks spurious here

::: js/src/wasm/WasmBaselineCompile.cpp:1218
(Diff revision 2)
> +            masm.reserveStack(size);
> +    }
> +
> +    // This is always equivalent to a sequence of masm.freeStack() calls.
> +    void freeArgAreaAndPopBytes(size_t argSize, size_t dropSize) {
> +        if (argSize + dropSize)

no risk it overflows, right?

::: js/src/wasm/WasmBaselineCompile.cpp:1254
(Diff revision 2)
> +
> +    // An unrollLimit of 16 is chosen so that we only need an 8-bit signed
> +    // immediate to represent the offset in the store instructions in the loop
> +    // on x64.
> +
> +    const uint32_t unrollLimit = 16;

pre-existing nit: could this be cased UNROLL_LIMIT and static instead, to make it ultra clear it's a compile-time constant?

::: js/src/wasm/WasmBaselineCompile.cpp:1398
(Diff revision 2)
> -        void setFramePushed(uint32_t framePushed) {
> -            MOZ_ASSERT(framePushed_ == UINT32_MAX);
> -            framePushed_ = framePushed;
> +        void setStackHeight(uint32_t stackHeight) {
> +            MOZ_ASSERT(stackHeight_ == UINT32_MAX);
> +            stackHeight_ = stackHeight;
>          }
>  
> -        void bind(MacroAssembler& masm) {
> +        void bind(BaseStackFrame& fr, MacroAssembler& masm) {

double nit: pointers please!

::: js/src/wasm/WasmBaselineCompile.cpp:1475
(Diff revision 2)
>      Assembler::DoubleCondition  latentDoubleCmp_;// Comparison operator, if latentOp_ == Compare, float types
>  
>      FuncOffsets                 offsets_;
>      MacroAssembler&             masm;            // No '_' suffix - too tedious...
>      BaseRegAlloc                ra;              // Ditto
> +    BaseStackFrame              fr;

(fr means French.)

...

Maybe `frame` isn't too long and explains what it is slightly better?

::: js/src/wasm/WasmBaselineCompile.cpp:1564
(Diff revision 2)
> -
> -    void loadFromFrameF32(FloatRegister r, int32_t offset) {
> -        masm.loadFloat32(Address(StackPointer, localOffsetToSPOffset(offset)), r);
> -    }
>  
> -    int32_t frameOffsetFromSlot(uint32_t slot, MIRType type) {
> +    Local& localFromSlot(uint32_t slot, MIRType type) {

Can it return `const Local&`?

::: js/src/wasm/WasmBaselineCompile.cpp:2077
(Diff revision 2)
> -                masm.Push(scratch);
> +                uint32_t offs = fr.pushPtr(scratch);
>  #else
> -                int32_t offset = frameOffsetFromSlot(v.slot(), MIRType::Int64);
> -                loadFromFrameI32(scratch, offset - INT64HIGH_OFFSET);
> -                masm.Push(scratch);
> -                loadFromFrameI32(scratch, offset - INT64LOW_OFFSET);
> +                fr.loadLocalI64High(scratch, localFromSlot(v.slot(), MIRType::Int64));
> +                fr.pushPtr(scratch);
> +                fr.loadLocalI64Low(scratch, localFromSlot(v.slot(), MIRType::Int64));
> +                uint32_t offs = fr.pushPtr(scratch);

Would have seemed more direct to keep on using pushI32 here, since an i64 is two i32 in the first place (it's also two ptrs because a ptr is the same size as an i32 on 32 bits platforms, but that's less explanatory)

(applies below too)

::: js/src/wasm/WasmBaselineCompile.cpp:2235
(Diff revision 2)
>              break;
>            case Stk::LocalI32:
>              loadLocalI32(r, v);
>              break;
>            case Stk::MemI32:
> -            masm.Pop(r);
> +            fr.popPtr(r);

popI32?

Comment 6

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8930875 [details] [diff] [review]
bug1419034-chunky-stack-wip.patch

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

I think I understand most of it, but without having the previous code before my eyes in a code editor, it's hard to figure out. I know this is only feedback, but it feels it would be super easy to be confused between frame pushed vs stack height at some point. But there's probably a way to prevent this...

::: js/src/wasm/WasmBaselineCompile.cpp
@@ +974,5 @@
>  
>      void endFunctionPrologue() {
> +        MOZ_ASSERT(masm.framePushed() == initialSize());
> +#ifdef RABALDR_CHUNKY_STACK
> +        MOZ_ASSERT(masm.framePushed() == localSize_ + InitialChunk);

(considering the code of initialSize(), these two assertions are equivalent)

@@ +1198,5 @@
>      uint32_t pushPtr(Register r) {
>          DebugOnly<uint32_t> stackBefore = stackHeight();
> +#ifdef RABALDR_CHUNKY_STACK
> +        pushChunkyBytes(StackSizeOfPtr);
> +        masm.storePtr(r, Address(sp_, stackOffset(stackHeight())));

I guess we don't worry about the potential performance impact of doing [addToStackPtr + store] vs just [push], since ARM64 is the only platform that will use this and there it might be the only way to put values onto the stack?

@@ +1367,5 @@
>      void freeArgAreaAndPopBytes(size_t argSize, size_t dropSize) {
> +#ifdef RABALDR_CHUNKY_STACK
> +        if (argSize)
> +            masm.freeStack(argSize);
> +        popChunkyBytes(dropSize);

This code makes me feel that I don't quite understand when to use freeStack vs popChunkyBytes; I guess that it's because dropSize refers to a value that we previously pushed with one of the push() methods, while argSize has been allocated with reserveStack.

It'd be really great if we could use static types (e.g. FramePushed vs StackHeight, both wrappers to uint32) to enforce the use of the right methods:

setStackHeight would now take a StackHeight
framePushedForHeight would take a StackHeight and return a FramePushed
etc

not even sure if this would help for popChunkyBytes, though :/

Comment 7

[Lars T Hansen [:lth]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #5)
> Comment on attachment 8930874 [details]
> Bug 1419034 - wasm baseline, factor out the stack frame logic.
> 
> ::: js/src/wasm/WasmBaselineCompile.cpp:695
> (Diff revision 2)
> > +
> > +    operator RegI32() const {
> > +# ifdef JS_CODEGEN_X86
> > +        return RegI32(ScratchRegX86);
> > +# else
> > +        return RegI32(ScratchRegARM);
> 
> Just an idea: instead of `ScratchRegARM` vs `ScratchRegX86`, we could have a
> single definition of `BaseScratchReg` that's defined per platform. A single
> section could contain all the per-platform registers, resulting in fewer
> per-platform ifdefs.

Separate patch coming (general scratchreg cleanup).

> ::: js/src/wasm/WasmBaselineCompile.cpp:703
> (Diff revision 2)
> > +};
> > +#else
> > +class ScratchI32 : public ScratchRegisterScope
> > +{
> > +  public:
> > +    explicit ScratchI32(MacroAssembler& m) : ScratchRegisterScope(m) {}
> 
> Is it expected that under x86 and ARM the ctor takes a BaseRegAlloc& while
> it takes a MacroAssembler& otherwise?

Ditto.

> ::: js/src/wasm/WasmBaselineCompile.cpp:868
> (Diff revision 2)
> > +
> > +    // High byte offset + 1 of local area for true locals.
> > +    uint32_t        varHigh_;
> > +
> > +    // The largest stack height, not necessarily zero-based.  Read this for its
> > +    // true value only when code generation is finished.
> 
> Would it be useful to have a DEBUG only boolean codegenIsFinished, that's
> asserted when trying to read the stack height?

Didn't do this but am thinking about it something here.

> ::: js/src/wasm/WasmBaselineCompile.cpp:949
> (Diff revision 2)
> > +        varLow_ = i.reservedSize();
> > +        for (; !i.done() && i.index() < args.length(); i++) {
> > +            MOZ_ASSERT(i.isArg());
> > +            MOZ_ASSERT(i.index() == index);
> > +            localInfo.infallibleEmplaceBack(i.mirType(), i.frameOffset());
> > +            varLow_ = i.currentLocalSize();
> 
> Can this be put outside the loop body, just after it?

Not in general, since the loop header updates the iterator and reading the value after the loop would be invalid.  (Yes, this annoys me every time I see it too.)

> ::: js/src/wasm/WasmBaselineCompile.cpp:1036
> (Diff revision 2)
> > +    // The stack area - the dynamic part of the frame.
> > +
> > +  private:
> > +
> > +    // Offset off of sp_ for the slot at stack area location `offset`
> > +    int32_t stackOffset(int32_t offset) {
> 
> Why do we need two methods for `stackOffset` and `localOffset` that do the
> same thing? Maybe there could be only one, or `localOffset` would call
> `stackOffset`?

This is slightly in flux because there's an option here for using a frame pointer, or some other mechanism where these two would have different implementations, so I haven't done anything here yet, but other than that I agree with you.

> ::: js/src/wasm/WasmBaselineCompile.cpp:1088
> (Diff revision 2)
> > +    uint32_t stackHeight() const {
> > +        return masm.framePushed();
> > +    }
> > +
> > +    // Set the frame height.  This must only be called with a value returned
> > +    // from stackHeight().
> 
> To statically enforce the constraint (and remove the comment), maybe have a
> `struct StackHeight` that just wraps an uint32_t, have `stackHeight()`
> return a struct of this type, and `setStackHeight` take a struct of this
> type by arg?

Yes, I want to do this too, but it'll have to be a separate patch / bug.

> ::: js/src/wasm/WasmBaselineCompile.cpp:1093
> (Diff revision 2)
> > +    // from stackHeight().
> > +    void setStackHeight(uint32_t amount) {
> > +        masm.setFramePushed(amount);
> > +    }
> > +
> > +    uint32_t pushPtr(Register r) {
> 
> Why not having pushI32 as an alias to pushPtr? Reading code much below was
> confusing because e.g. an i64 is by definition two i32, not two ptrs (even
> if it's coincidentally true on 32 bits platforms).

So the pattern I use is that pushI32/pushF32/ etc all use the strongly typed register wrappers, but these functions operate on raw registers (and if it weren't for ARM there would be only a pushDouble here); pushPtr does *not* push an I32, it pushes a machine register.

I don't *really* like the "ptr" convention but we use it many places and I've tried to adopt it here in the naming to distinguish machine registers from the register wrapper types.

> ::: js/src/wasm/WasmBaselineCompile.cpp:1475
> (Diff revision 2)
> >      Assembler::DoubleCondition  latentDoubleCmp_;// Comparison operator, if latentOp_ == Compare, float types
> >  
> >      FuncOffsets                 offsets_;
> >      MacroAssembler&             masm;            // No '_' suffix - too tedious...
> >      BaseRegAlloc                ra;              // Ditto
> > +    BaseStackFrame              fr;
> 
> (fr means French.)

Or in this case, "frame" :)

> Maybe `frame` isn't too long and explains what it is slightly better?

Keeping "fr" for now, to partner with "ra" (which could/should be "regalloc" maybe).  Hard to know how much brevity is too much.  Your opposition is noted.

Comment 8

[Lars T Hansen [:lth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f064746526d89dfe92e1c5a607e4cb34f95250b4

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b69380013192
rabaldr, factor out the stack frame logic. r=bbouvier

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/b69380013192