1747787 - LStackArea alignment is always 8, even if SIMD might require 16

Status: NEW

(Core :: JavaScript: WebAssembly, defect, P3)

Description

[Lars T Hansen [:lth]]

The stack-allocated multi-result area in Ion is managed in concert by WasmIonCompile, lowering, and the register allocator. Eventually, in BacktrackingAllocator::allocateStackDefinition, an LStackArea object is created and stackSlotAllocator.allocateStackArea is called on this value. The latter requires the alignment of the LStackArea to be 8 exactly. It would be more natural for there to be a case here for 16 and for the LStackArea to pass in the alignment required by the result area, which, if it contains SIMD values, would be 16. Then the result values would themselves be naturally aligned within the area.

I'm uncertain about what exactly goes on with the baseline compiler but it does not look like much effort is made to align anything naturally here either. Ion and Baseline multi-return ABIs must be compatible, so if one does not try to align results, the other can't rely on them being aligned.

(In principle this is also an issue for Cranelift. Cranelift does not have SIMD, but if baseline assumes the frame will be 16-aligned, say, then there may be trouble. So the right answer is probably to choose an alignment based on the return types, or on whether SIMD is available in the build, to avoid problems. On the other hand, Cranelift has no tail calls either, and enabling Cranelift should reasonably shut off both SIMD and tail calls.)

On every platform we currently care about, we assume unaligned access is OK, either because the architecture says it is or we generate instructions that specifically allow unaligned accesses. But there's the general presumption that aligning is better and will lead to marginally faster code.

In the current case I think it's also a matter of exposing the alignment decision better.

Comment 1

[Lars T Hansen [:lth]]

When fixing this, be sure to update the ABI docs in WasmFrame.h, because they reference this bug. See also bug 1744513.

Comment 2

[Lars T Hansen [:lth]]

BTW, I think this blocks tail calls because tail calls will need to know exactly how much space is occupied by the argument area, since the caller and callee will collaborate on managing this space. And that includes any padding above the argument area, and that padding depends on the alignment of the multi-return area.

Comment 3

[Lars T Hansen [:lth]]

Two different things here I think and let's not mix them up if we can avoid it:

• the LStackArea should be aligned at a known boundary so that tail calls can know exactly how much space is occupied by the stack args below the LStackArea, including padding
• the values within the LStackArea should be naturally aligned

The second part is going to be hard to make work because the ABIResultIter lays out values within the LStackArea according to the layout conventions of the baseline compiler's internal stack. In turn this is because the ABIResultIter is used for block results within the baseline compiler and so that compatibility is required. This entanglement means that it is easy for the baseline compiler to receive stack results from calls, but it makes everything else coupled. But since the baseline compiler does not align elements on its internal stack then the ABIResultIter can't either.

Fortunately the second part is just a nice-to-have.

Comment 4

[Lars T Hansen [:lth]]

Tail calls are not actually blocked by this; tail calls only need to know the alignment of the stack argument area, and that is independent of the LStackArea.