Add support for stack results in Ion wasm compiler
Categories
(Core :: JavaScript: WebAssembly, enhancement, P2)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox75 | --- | fixed |
People
(Reporter: wingo, Assigned: wingo)
References
Details
Attachments
(3 files)
With multi-value, WebAssembly functions can return multiple results. Some of these results may be in registers; see bug 1608121 for how that's handled. Any result not allocated to a register will be returned on the stack. Functions that have stack results take an additional synthetic argument pointing to a stack results area. That single area will store all stack results, packed contiguously according to the usual alignment rules, with the last result having the lowest address.
To support calls to functions returning stack results in Ion, we will need to make a MWasmStackResultsArea. This value will be defined before a call and will be passed as a pointer to the call, as the synthetic stack results argument.
To extract values from the stack area, we will define MWasmStackResult, which uses the stack results area and includes an offset into that area.
I am not quite sure about two things: (1) how to make it so that we allocate the stack space; does that need a tie-in with the register allocator? (2) how to make sure that the stack results aren't stompled before the values are used or otherwise loaded from the stack?
|
Assignee | |
Updated•5 years ago
|
|
|
Assignee | |
Comment 1•5 years ago
|
||
Current draft strategy is to add MWasmStackResult definitions before the MWasmCall, lowering to fixed LStackSlot allocations. This ensures they are in the stack map during the call, and that any write to a GC-managed result is sure to be marked by any stack walk. However I am still trying to understand how to allocate the slots contiguously. I suppose I can keep calling StackSlotAllocator::addAvailableSlot until I have the right stack size and alignment, if no suitably-large range of slots is found. I am also not sure if I can make a MWasmStackResults area allocated to those slots, but whose stack slots can be aliased by MWasmStackResult instances.
I will probably also need a new STACK_RESULTS_AREA LAllocation::Kind to use as the pointer to the stack to pass to Wasm calls with stack results.
|
|
Assignee | |
Comment 2•5 years ago
|
||
This patch extends MIR with a MWasmStackResultArea which contains the
locations to which stack result values will be written. The intention
is that the MWasmStackResultArea will get passed as an additional
argument to calls with stack results, and the results will get extracted
from the area via instances of MWasmStackResult.
Next steps will be to wire up WasmIonCompile and implement the back-end,
extending the register allocator to handle stack result areas specially.
|
|
Assignee | |
Comment 3•5 years ago
|
||
This patch extends LIR with the ability to allocate stack result areas
and stack results.
There are a few interesting challenges here. The first is that stack
result areas are of variable size, so they require an extension to
StackSlotAllocator. Additionally, to store both stack frame offset and
information about the values that stack result areas contain while not
expanding the size of LAllocation, the new LAllocation subtype of
LStackArea encodes its data via a pointer to LIR instead of inline.
The second is that stack results alias the area in which they are
defined. To allow for this, this patch extends the register allocator
to eagerly allocate stack result areas and then give fixed allocations
to the individual stack results.
The next patch will add support for loading a stack result area address
into a register, via extending MoveOp. Then we will be able to finally
wire up multi-value calls in Ion.
|
|
Assignee | |
Updated•5 years ago
|
|
|
Assignee | |
Comment 5•5 years ago
|
||
This patch enables the two sides of multiple-value calls in Ion:
-
Calls from Ion to functions that return multiple values will pass a
pointer to a stack result area -
Compiling a function that returns multiple values with Ion will write
the values appropriately. There's a new MWasmStoreStackResult for
non-ref results; ref results use an MWasmDerivedPointer and
MWasmStoreRef.
The patch is otherwise untested but as multiple-value calls are still
gated by wasm::MaxFuncResults which is still 1, there's no risk to
browser users. Tests to follow.
|
||
Comment 6•5 years ago
|
||
| bugherder | ||
|
|
Assignee | |
Comment 7•5 years ago
|
||
FYI, for this module:
(module
(func (param i32 i64 i32 i64 i32
i64 i32 i64 i32 i64)
(result i64 i32 i64)
(local i32 i64 i32 i64)
(local.get 7)
(local.get 8)
(local.get 9))
(func (export "run") (result i32)
(i32.const 0)
(i64.const 1)
(i32.const 2)
(i64.const 3)
(i32.const 4)
(i64.const 5)
(i32.const 6)
(i64.const 7)
(i32.const 52)
(i64.const 10)
(call 0)
i32.wrap_i64
i32.sub
i64.extend_i32_s
i64.sub
i32.wrap_i64))
Ion does this for the first function:
[Codegen] # Emitting wasm code
[Codegen] .balign 16, 0xf4 # hlt
[Codegen] .set .Llabel0, .
[Codegen] movq 0x60(%r14), %rax
[Codegen] cmpq %rax, %r10
[Codegen] je .Lfrom13
[Codegen] ud2
[Codegen] .set .Llabel15, .
[Codegen] .balign 16
[Codegen] .set .Llabel16, .
[Codegen] .set .Lfrom13, .Llabel16
[Codegen] .set .Llabel16, .
[Codegen] .set .Llabel16, .
[Codegen] push %r14
[Codegen] .set .Llabel18, .
[Codegen] push %rbp
[Codegen] .set .Llabel19, .
[Codegen] movq %rsp, %rbp
[Codegen] .set .Llabel22, .
[Codegen] .set .Llabel22, .
[Codegen] ==== BEGIN CodeGenerator::generateBody ====
[Codegen] # block0 ?:0:0:
[Codegen] .set .Llabel22, .
[Codegen] # instruction WasmParameter
[Codegen] # instruction WasmParameter
[Codegen] # instruction WasmParameter
[Codegen] # instruction WasmParameter
[Codegen] # instruction WasmParameter
[Codegen] # instruction WasmParameter
[Codegen] # instruction WasmParameter
[Codegen] # instruction WasmParameterI64
[Codegen] # instruction WasmParameter
[Codegen] # instruction WasmParameterI64
[Codegen] # instruction WasmParameter
[Codegen] # instruction WasmParameter
[Codegen] # instruction MoveGroup
[Codegen] movq 0x38(%rsp), %rax
[Codegen] movq 0x20(%rsp), %rcx
[Codegen] # instruction WasmStoreSlotI64
[Codegen] movq %rcx, 0x8(%rax)
[Codegen] # instruction MoveGroup
[Codegen] movl 0x28(%rsp), %ecx
[Codegen] # instruction WasmStoreSlot
[Codegen] movl %ecx, 0x0(%rax)
[Codegen] # instruction MoveGroup
[Codegen] movq 0x30(%rsp), %rax
[Codegen] # instruction WasmReturnI64
[Codegen] ==== END CodeGenerator::generateBody ====
[Codegen] .set .Llabel47, .
[Codegen] pop %rbp
[Codegen] .set .Llabel48, .
[Codegen] pop %r14
[Codegen] .set .Llabel50, .
[Codegen] .set .Llabel50, .
[Codegen] ret
[Codegen] .set .Llabel51, .
[Codegen] # wasm::IonCompileFunctions: completed function index 0
And for the second (exported) function:
[Codegen] # Emitting wasm code
[Codegen] .balign 16, 0xf4 # hlt
[Codegen] .set .Llabel64, .
[Codegen] cmpl $0x3, %r10d
[Codegen] je .Lfrom74
[Codegen] ud2
[Codegen] .set .Llabel76, .
[Codegen] .balign 16
[Codegen] .set .Llabel80, .
[Codegen] .set .Lfrom74, .Llabel80
[Codegen] .set .Llabel80, .
[Codegen] .set .Llabel80, .
[Codegen] push %r14
[Codegen] .set .Llabel82, .
[Codegen] push %rbp
[Codegen] .set .Llabel83, .
[Codegen] movq %rsp, %rbp
[Codegen] .set .Llabel86, .
[Codegen] .set .Llabel86, .
[Codegen] subq $56, %rsp
[Codegen] cmpq %rsp, 0x30(%r14)
[Codegen] jb .Lfrom100
[Codegen] ud2
[Codegen] .set .Llabel102, .
[Codegen] .set .Llabel102, .
[Codegen] .set .Llabel102, .
[Codegen] .set .Lfrom100, .Llabel102
[Codegen] ==== BEGIN CodeGenerator::generateBody ====
[Codegen] # block0 ?:0:0:
[Codegen] .set .Llabel102, .
[Codegen] # instruction WasmParameter
[Codegen] # instruction WasmStackArg
[Codegen] movq $6, 0x0(%rsp)
[Codegen] # instruction WasmStackArgI64
[Codegen] movq $7, 0x8(%rsp)
[Codegen] # instruction WasmStackArg
[Codegen] movq $52, 0x10(%rsp)
[Codegen] # instruction WasmStackArgI64
[Codegen] movq $10, 0x18(%rsp)
[Codegen] # instruction WasmStackResultArea
[Codegen] # instruction MoveGroup
[Codegen] leaq 0x28(%rsp), %rax
[Codegen] # instruction WasmStackArg
[Codegen] movq %rax, 0x20(%rsp)
[Codegen] # instruction Integer
[Codegen] xorl %edi, %edi
[Codegen] # instruction Integer64
[Codegen] movl $0x1, %esi
[Codegen] # instruction Integer
[Codegen] movl $0x2, %edx
[Codegen] # instruction Integer64
[Codegen] movl $0x3, %ecx
[Codegen] # instruction Integer
[Codegen] movl $0x4, %r8d
[Codegen] # instruction Integer64
[Codegen] movl $0x5, %r9d
[Codegen] # instruction WasmCall
[Codegen] testb $0xf, %spl
[Codegen] je .Lfrom186
[Codegen] int3
[Codegen] .set .Llabel187, .
[Codegen] .set .Lfrom186, .Llabel187
[Codegen] call .Lfrom192
[Codegen] .set .Llabel192, .
[Codegen] # instruction WasmStackResult64
[Codegen] # instruction WasmStackResult
[Codegen] # instruction WasmRegisterResult
[Codegen] # instruction WrapInt64ToInt32
[Codegen] movl %eax, %eax
[Codegen] # instruction MoveGroup
[Codegen] movl 0x28(%rsp), %ecx
[Codegen] # instruction SubI
[Codegen] subl %eax, %ecx
[Codegen] # instruction ExtendInt32ToInt64
[Codegen] movslq %ecx, %rax
[Codegen] # instruction MoveGroup
[Codegen] movq 0x30(%rsp), %rcx
[Codegen] # instruction SubI64
[Codegen] subq %rax, %rcx
[Codegen] # instruction WrapInt64ToInt32
[Codegen] movl %ecx, %eax
[Codegen] # instruction WasmReturn
[Codegen] ==== END CodeGenerator::generateBody ====
[Codegen] .set .Llabel213, .
[Codegen] addq $56, %rsp
[Codegen] pop %rbp
[Codegen] .set .Llabel218, .
[Codegen] pop %r14
[Codegen] .set .Llabel220, .
[Codegen] .set .Llabel220, .
[Codegen] ret
[Codegen] .set .Llabel221, .
[Codegen] # wasm::IonCompileFunctions: completed function index 1
As you can see, the stack results are left in place until they are used, at which point a MoveGroup loads them up into a register for LSubI. Looking reasonable to me for now.
|
||
Comment 8•5 years ago
|
||
That's pretty nice code!
|
||
Comment 10•5 years ago
|
||
Backed out changeset a813a2f0970a (Bug 1609057) for build bustages on LIR.h
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=291817134&repo=autoland
Backout link: https://hg.mozilla.org/integration/autoland/rev/f66db742b2e70cc8dbead25b6fc2585f2a1a6df2
[task 2020-03-05T13:37:53.269Z] 13:37:53 INFO - In file included from /builds/worker/workspace/build/src/js/src/jit/BacktrackingAllocator.h:15:
[task 2020-03-05T13:37:53.269Z] 13:37:53 INFO - In file included from /builds/worker/workspace/build/src/js/src/jit/RegisterAllocator.h:13:
[task 2020-03-05T13:37:53.269Z] 13:37:53 ERROR - /builds/worker/workspace/build/src/js/src/jit/LIR.h:411:33: error: unknown type name 'LNode'; did you mean 'MNode'?
[task 2020-03-05T13:37:53.269Z] 13:37:53 INFO - inline LStackSlot resultAlloc(LNode* lir) const;
[task 2020-03-05T13:37:53.269Z] 13:37:53 INFO - ^~~~~
[task 2020-03-05T13:37:53.269Z] 13:37:53 INFO - MNode
[task 2020-03-05T13:37:53.269Z] 13:37:53 INFO - /builds/worker/workspace/build/src/js/src/jit/MIR.h:263:7: note: 'MNode' declared here
[task 2020-03-05T13:37:53.269Z] 13:37:53 INFO - class MNode : public TempObject {
[task 2020-03-05T13:37:53.269Z] 13:37:53 INFO - ^
[task 2020-03-05T13:37:53.269Z] 13:37:53 INFO - In file included from Unified_cpp_js_src_jit0.cpp:20:
[task 2020-03-05T13:37:53.269Z] 13:37:53 INFO - In file included from /builds/worker/workspace/build/src/js/src/jit/BacktrackingAllocator.cpp:7:
[task 2020-03-05T13:37:53.269Z] 13:37:53 INFO - In file included from /builds/worker/workspace/build/src/js/src/jit/BacktrackingAllocator.h:15:
[task 2020-03-05T13:37:53.269Z] 13:37:53 INFO - In file included from /builds/worker/workspace/build/src/js/src/jit/RegisterAllocator.h:13:
[task 2020-03-05T13:37:53.269Z] 13:37:53 INFO - In file included from /builds/worker/workspace/build/src/js/src/jit/LIR.h:1920:
[task 2020-03-05T13:37:53.269Z] 13:37:53 ERROR - /builds/worker/workspace/build/src/js/src/jit/shared/LIR-shared.h:6806:31: error: out-of-line definition of 'resultAlloc' does not match any declaration in 'js::jit::LStackArea'
[task 2020-03-05T13:37:53.269Z] 13:37:53 INFO - inline LStackSlot LStackArea::resultAlloc(LNode* lir) const {
[task 2020-03-05T13:37:53.269Z] 13:37:53 INFO - ^~~~~~~~~~~
[task 2020-03-05T13:37:53.269Z] 13:37:53 INFO - In file included from Unified_cpp_js_src_jit0.cpp:20:
[task 2020-03-05T13:37:53.269Z] 13:37:53 ERROR - /builds/worker/workspace/build/src/js/src/jit/BacktrackingAllocator.cpp:1189:49: error: cannot initialize a parameter of type 'js::jit::MNode *' with an rvalue of type 'js::jit::LNode '
[task 2020-03-05T13:37:53.269Z] 13:37:53 INFO - reg.def()->setOutput(areaAlloc->resultAlloc(reg.ins()));
[task 2020-03-05T13:37:53.269Z] 13:37:53 INFO - ^~~~~~~~~
[task 2020-03-05T13:37:53.270Z] 13:37:53 INFO - /builds/worker/workspace/build/src/js/src/jit/LIR.h:411:40: note: passing argument to parameter 'lir' here
[task 2020-03-05T13:37:53.272Z] 13:37:53 INFO - inline LStackSlot resultAlloc(LNode lir) const;
[task 2020-03-05T13:37:53.272Z] 13:37:53 INFO - ^
[task 2020-03-05T13:37:53.272Z] 13:37:53 INFO - 3 errors generated.
[task 2020-03-05T13:37:53.272Z] 13:37:53 INFO - /builds/worker/workspace/build/src/config/rules.mk:745: recipe for target 'Unified_cpp_js_src_jit0.o' failed
[task 2020-03-05T13:37:53.272Z] 13:37:53 ERROR - make[4]: *** [Unified_cpp_js_src_jit0.o] Error 1
[task 2020-03-05T13:37:53.272Z] 13:37:53 INFO - make[4]: Leaving directory '/builds/worker/workspace/build/src/obj-firefox/js/src/jit'
[task 2020-03-05T13:37:53.273Z] 13:37:53 INFO - make[4]: *** Waiting for unfinished jobs....
|
|
Assignee | |
Comment 11•5 years ago
|
||
Thanks & apologies -- appears to have been a unified build issue. Fixed this and another bug and re-set checkin-needed.
|
||
Comment 12•5 years ago
|
||
|
|
Assignee | |
Updated•5 years ago
|
|
||
Comment 13•5 years ago
|
||
| bugherder | ||
|
|
||
Comment 14•5 years ago
|
||
|
||
Comment 15•5 years ago
|
||
| bugherder | ||
Description
•