Closed Bug 1609057 Opened 4 years ago Closed 4 years ago

Add support for stack results in Ion wasm compiler

Categories

(Core :: JavaScript: WebAssembly, enhancement, P2)

Product:
Core
Component:
JavaScript: WebAssembly
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla75
Tracking Flags:
Tracking Status
firefox75 --- fixed

People

(Reporter: wingo, Assigned: wingo)

References

Details

Attachments

(3 files)

Bug 1609057 - Add support for stack results in Ion wasm compiler
47 bytes, text/x-phabricator-request
Details | Review
Bug 1609057 - Add support for stack results to LIR
47 bytes, text/x-phabricator-request
Details | Review
Bug 1609057 - Wire up stack results in WasmIonCompile
47 bytes, text/x-phabricator-request
Details | Review

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: nobody → wingo

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.

Depends on: 1609889

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.

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.

Keywords: leave-open
Pushed by nbeleuzu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/097aecdda3d0
Add support for stack results in Ion wasm compiler r=lth

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.

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.

That's pretty nice code!

Pushed by csabou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a813a2f0970a
Add support for stack results to LIR r=lth

Backed out changeset a813a2f0970a (Bug 1609057) for build bustages on LIR.h

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&collapsedPushes=656020&resultStatus=busted%2Cexception&revision=a813a2f0970a7fe45d19b3d036c62dc846526767&selectedJob=291817134

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....

Flags: needinfo?(wingo)

Thanks & apologies -- appears to have been a unified build issue. Fixed this and another bug and re-set checkin-needed.

Flags: needinfo?(wingo)
Pushed by csabou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e91c62508fd8
Add support for stack results to LIR r=lth
Keywords: leave-open

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

Status: NEW → RESOLVED
Closed: 4 years ago
status-firefox75: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla75
Pushed by shindli@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/41657d8ce5ac
Wire up stack results in WasmIonCompile r=lth
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: