Closed Bug 1620167 Opened 5 years ago Closed 3 years ago

Consider relaxing JIT stack alignment again

Categories

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

task

Tracking

()

RESOLVED WONTFIX

People

(Reporter: jandem, Unassigned)

References

Details

Attachments

(1 file)

For SIMD in JS, the JIT stack layout was changed to always be 16-byte aligned. This complicates and adds overhead to a lot of code, for example here or here.

Without SIMD.js constraints, it'd probably be simpler and more efficient to do this only on the JS -> Wasm boundary.

Depends on: 1416723

I checked, and both the jit->wasm entry and inlined wasm calls align the stack at the boundary, so removing the constraint in the JS JIT should Just Work.

I agree. We'll do SIMD internal to wasm, but at the JS/wasm boundary we currently have no story for transmitting those data. If we ever develop one (questionable IMO even though nobody likes the friction) the data would probably be transmitted as a short TypedArray.

Priority: -- → P3

(In reply to Benjamin Bouvier [:bbouvier] from comment #1)

I checked, and both the jit->wasm entry and inlined wasm calls align the stack at the boundary, so removing the constraint in the JS JIT should Just Work.

As far as I can tell GenerateJitEntry assumes WasmStackAlignment?

Flags: needinfo?(bbouvier)

As far as I can tell GenerateJitEntry assumes WasmStackAlignment?

No, this particular line enforces the stack alignment, so nothing is assumed from the caller. For what it's worth, this stub is a trampoline between JIT code and wasm code, there's nothing to worry about nor to change here. Every other JIT -> wasm entry does something similar, so unless there's something else I'm missing, all the changes in the JS-JIT code should not impact wasm alignment constraints.

Flags: needinfo?(bbouvier)

We talked about this a bit more. JIT -> Wasm calls currently rely on the JIT alignment and we'd have to align the stack dynamically - probably fine, just requires some careful code changes.

Attached patch Hacky first stab (deleted) — Splinter Review

When I filed this bug I prototyped this quickly, posting the patch here for reference.

Closing this. There's the Wasm issue + the alignment lets us speed up ABI calls (bug 1763592) so is probably worth keeping.

Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: