Stream: git-wasmtime

Topic: wasmtime / PR #4907 Initial skeleton for Winch

Wasmtime GitHub notifications bot (Oct 18 2022 at 02:04):

saulecabrera edited PR #4907 from winch-skeleton to main:

This change introduces the initial skeleton for Winch, the "baseline" compiler, discussed https://github.com/bytecodealliance/rfcs/pull/28

The skeleton contains the just the necessary code for the main abstractions to support the following:

• Function prologue and epilogue (excluding stack checks)
• Calculation of local slots
• An initial pass on a generic MacroAssembler interface
• An initial pass on a x64 Assembler
• A simple CLI tools to compile WebAssembly programs (crates/winch/src/main.rs)
• String based emission for programs that use the following WebAssembly instructions: i32.add, local.get, local.set

As of this change, the high-level structure of the compiler is as follows:

      +-----------------------+
       |                       |
       |  +-----------+      +---------------------+
       |  | Compiler  | ---> |         ISA         | ---------------------------------------------------------------+
       |  +-----------+      +---------------------+                                                                |
       |                       |                                                                                    |
       |                       |                                                                                    |
       |                       v                                                                                    |
       |  +-----------+-----+----------------------+     +----------------+     +-------+----------------+-------+  |
       +> | Registers | ABI |        CodeGen       | --> | CodeGenContext | --> | Frame | MacroAssembler | Stack |  |
          +-----------+-----+----------------------+     +----------------+     +-------+----------------+-------+  |
                         ^     |                                                                                    |
                         +-----+------------------------------------------------------------------------------------+
                               |
                               |
                               |
                               v
                             +---------------------+
                             | Register Allocation |
                             +---------------------+

---

Next steps

As mentioned above, this change doesn't use any of Cranelift's backends for code emission, yet. These are the immediate next steps after landing this change:

• Extract the relevant Cranelift pieces into the cranelift_asm crate, which will be used by Winch
• Swap the string based implementation of the x64 assembler and use the assembler provided by cranelift_asm
• Add initial support for arm64 through cranelift_asm
• Increase test coverage

---

Misc

Winch's CLI can be used by running `cargo run -- path/to/file.wat --target=<target>

Here's a set of WebAssembly programs and their corresponding x64 output:
<details>
<summary>Basic i32 add</summary>

(module
  (export "main" (func $main))

  (func $main (result i32)
    (i32.const 10)
    (i32.const 20)
    i32.add)
)

push rbp
mov rbp, rsp
mov eax, 10
add eax, 20
pop rbp
ret

</details>

<details>
<summary> i32 add with local operators </summary>

(module
  (export "main" (func $main))

  (func $main (result i32)
    (local $foo i32)
    (local $bar i32)
    (i32.const 10)
    (local.set $foo)
    (i32.const 20)
    (local.set $bar)
    (local.get $foo)
    (local.get $bar)
    i32.add
  )
)

push rbp
mov rbp, rsp
sub rsp, 8
mov qword [rsp], 0
mov eax, 10
mov [rsp + 4], eax
mov ecx, 20
mov [rsp], ecx
mov eax, [rsp]
mov edx, [rsp + 4]
add edx, eax
mov rax, rdx
add rsp, 8
pop rbp
ret

</details>

<details>
<summary>Basic i32 with function arguments </summary>

(module
  (export "main" (func $main))

  (func $main (param i32) (param i32) (result i32)
      (local.get 0)
      (local.get 1)
      i32.add
    )
)

push rbp
mov rbp, rsp
sub rsp, 8
mov [rsp + 4], edi
mov [rsp], esi
mov eax, [rsp]
mov ecx, [rsp + 4]
add ecx, eax
mov rax, rcx
add rsp, 8
pop rbp
ret

</details>

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.
• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.
• [ ] This PR contains test cases, if meaningful.
• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the
  [message truncated]

Wasmtime GitHub notifications bot (Oct 18 2022 at 11:25):

saulecabrera updated PR #4907 from winch-skeleton to main.

Wasmtime GitHub notifications bot (Oct 18 2022 at 11:31):

saulecabrera updated PR #4907 from winch-skeleton to main.

Wasmtime GitHub notifications bot (Oct 18 2022 at 11:33):

saulecabrera updated PR #4907 from winch-skeleton to main.

Wasmtime GitHub notifications bot (Oct 18 2022 at 11:42):

saulecabrera edited PR #4907 from winch-skeleton to main:

This change introduces the initial skeleton for Winch, the "baseline" compiler, discussed https://github.com/bytecodealliance/rfcs/pull/28

The skeleton contains the just the necessary code for the main abstractions to support the following:

• Function prologue and epilogue (excluding stack checks)
• Calculation of local slots
• An initial pass on a generic MacroAssembler interface
• An initial pass on a x64 Assembler
• A simple CLI tools to compile WebAssembly programs (crates/winch/src/main.rs)
• String based emission for programs that use the following WebAssembly instructions: i32.add, local.get, local.set

As of this change, the high-level structure of the compiler is as follows:

      +-----------------------+
       |                       |
       |  +-----------+      +---------------------+
       |  | Compiler  | ---> |         ISA         | ---------------------------------------------------------------+
       |  +-----------+      +---------------------+                                                                |
       |                       |                                                                                    |
       |                       |                                                                                    |
       |                       v                                                                                    |
       |  +-----------+-----+----------------------+     +----------------+     +-------+----------------+-------+  |
       +> | Registers | ABI |        CodeGen       | --> | CodeGenContext | --> | Frame | MacroAssembler | Stack |  |
          +-----------+-----+----------------------+     +----------------+     +-------+----------------+-------+  |
                         ^     |                                                                                    |
                         +-----+------------------------------------------------------------------------------------+
                               |
                               |
                               |
                               v
                             +---------------------+
                             | Register Allocation |
                             +---------------------+

---

Next steps

As mentioned above, this change doesn't use any of Cranelift's backends for code emission, yet. These are the immediate next steps after landing this change:

• Extract the relevant Cranelift pieces into the cranelift_asm crate, which will be used by Winch
• Swap the string based implementation of the x64 assembler and use the assembler provided by cranelift_asm
• Add initial support for arm64 through cranelift_asm
• Increase test coverage, and include tests in CI. Opted to not do that in this change since the emission mechanism will change, so it might be easier to wait and add verification tests once we are using Cranelift's backends.

---

Misc

Winch's CLI can be used by running `cargo run -- path/to/file.wat --target=<target>

Here's a set of WebAssembly programs and their corresponding x64 output:
<details>
<summary>Basic i32 add</summary>

(module
  (export "main" (func $main))

  (func $main (result i32)
    (i32.const 10)
    (i32.const 20)
    i32.add)
)

push rbp
mov rbp, rsp
mov eax, 10
add eax, 20
pop rbp
ret

</details>

<details>
<summary> i32 add with local operators </summary>

(module
  (export "main" (func $main))

  (func $main (result i32)
    (local $foo i32)
    (local $bar i32)
    (i32.const 10)
    (local.set $foo)
    (i32.const 20)
    (local.set $bar)
    (local.get $foo)
    (local.get $bar)
    i32.add
  )
)

push rbp
mov rbp, rsp
sub rsp, 8
mov qword [rsp], 0
mov eax, 10
mov [rsp + 4], eax
mov ecx, 20
mov [rsp], ecx
mov eax, [rsp]
mov edx, [rsp + 4]
add edx, eax
mov rax, rdx
add rsp, 8
pop rbp
ret

</details>

<details>
<summary>Basic i32 with function arguments </summary>

(module
  (export "main" (func $main))

  (func $main (param i32) (param i32) (result i32)
      (local.get 0)
      (local.get 1)
      i32.add
    )
)

push rbp
mov rbp, rsp
sub rsp, 8
mov [rsp + 4], edi
mov [rsp], esi
mov eax, [rsp]
mov ecx, [rsp + 4]
add ecx, eax
mov rax, rcx
add rsp, 8
pop rbp
ret

</details>

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.
• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.
• [ ] This PR contains test cases,
  [message truncated]

Wasmtime GitHub notifications bot (Oct 18 2022 at 18:11):

saulecabrera updated PR #4907 from winch-skeleton to main.

Wasmtime GitHub notifications bot (Oct 18 2022 at 18:15):

saulecabrera edited PR #4907 from winch-skeleton to main:

This change introduces the initial skeleton for Winch, the "baseline" compiler, discussed https://github.com/bytecodealliance/rfcs/pull/28

The skeleton contains the just the necessary code for the main abstractions to support the following:

• Function prologue and epilogue (excluding stack checks)
• Calculation of local slots
• An initial pass on a generic MacroAssembler interface
• An initial pass on a x64 Assembler
• A simple CLI tools to compile WebAssembly programs (crates/winch/src/main.rs)
• String based emission for programs that use the following WebAssembly instructions: i32.add, local.get, local.set

As of this change, the high-level structure of the compiler is as follows:

      +-----------------------+
       |                       |
       |  +-----------+      +---------------------+
       |  | Compiler  | ---> |         ISA         | ---------------------------------------------------------------+
       |  +-----------+      +---------------------+                                                                |
       |                       |                                                                                    |
       |                       |                                                                                    |
       |                       v                                                                                    |
       |  +-----------+-----+----------------------+     +----------------+     +-------+----------------+-------+  |
       +> | Registers | ABI |        CodeGen       | --> | CodeGenContext | --> | Frame | MacroAssembler | Stack |  |
          +-----------+-----+----------------------+     +----------------+     +-------+----------------+-------+  |
                         ^     |                                                                                    |
                         +-----+------------------------------------------------------------------------------------+
                               |
                               |
                               |
                               v
                             +---------------------+
                             | Register Allocation |
                             +---------------------+

---

Next steps

As mentioned above, this change doesn't use any of Cranelift's backends for code emission, yet. These are the immediate next steps after landing this change:

• Extract the relevant Cranelift pieces into the cranelift_asm crate, which will be used by Winch
• Swap the string based implementation of the x64 assembler and use the assembler provided by cranelift_asm
• Add initial support for arm64 through cranelift_asm
• Increase test coverage, and include tests in CI. Opted to not do that in this change since the emission mechanism will change, so it might be easier to wait and add verification tests once we are using Cranelift's backends.

---

Misc

Winch's CLI can be used by running cargo run -- path/to/file.wat --target=<target>

Here's a set of WebAssembly programs and their corresponding x64 output:
<details>
<summary>Basic i32 add</summary>

(module
  (export "main" (func $main))

  (func $main (result i32)
    (i32.const 10)
    (i32.const 20)
    i32.add)
)

push rbp
mov rbp, rsp
mov eax, 10
add eax, 20
pop rbp
ret

</details>

<details>
<summary> i32 add with local operators </summary>

(module
  (export "main" (func $main))

  (func $main (result i32)
    (local $foo i32)
    (local $bar i32)
    (i32.const 10)
    (local.set $foo)
    (i32.const 20)
    (local.set $bar)
    (local.get $foo)
    (local.get $bar)
    i32.add
  )
)

push rbp
mov rbp, rsp
sub rsp, 8
mov qword [rsp], 0
mov eax, 10
mov [rsp + 4], eax
mov ecx, 20
mov [rsp], ecx
mov eax, [rsp]
mov edx, [rsp + 4]
add edx, eax
mov rax, rdx
add rsp, 8
pop rbp
ret

</details>

<details>
<summary>Basic i32 with function arguments </summary>

(module
  (export "main" (func $main))

  (func $main (param i32) (param i32) (result i32)
      (local.get 0)
      (local.get 1)
      i32.add
    )
)

push rbp
mov rbp, rsp
sub rsp, 8
mov [rsp + 4], edi
mov [rsp], esi
mov eax, [rsp]
mov ecx, [rsp + 4]
add ecx, eax
mov rax, rcx
add rsp, 8
pop rbp
ret

</details>

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.
• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.
• [ ] This PR contains test cases,
  [message truncated]

Wasmtime GitHub notifications bot (Oct 18 2022 at 18:36):

saulecabrera edited PR #4907 from winch-skeleton to main:

This change introduces the initial skeleton for Winch, the "baseline" compiler, discussed https://github.com/bytecodealliance/rfcs/pull/28

The skeleton contains the just the necessary code for the main abstractions to support the following:

• Function prologue and epilogue (excluding stack checks)
• Calculation of local slots
• An initial pass on a generic MacroAssembler interface
• An initial pass on a x64 Assembler
• A simple CLI tools to compile WebAssembly programs (crates/winch/src/main.rs)
• String based emission for programs that use the following WebAssembly instructions: i32.add, local.get, local.set

As of this change, the high-level structure of the compiler is as follows:

       +--------------------------------------+
       |                                      |
       |                                      |
       |    +-----------------------+         |
       |    |                       |         |
       |    |  +-----------+      +--------+  |
       |    |  | Compiler  | ---> |  ISA   | -+
       |    |  +-----------+      +--------+
       |    |                       |
       |    |                       |
       |    |                       v
       |    |  +-----------+-----+---------+     +---------------------+     +----------------+     +-------+----------------+-------+
       |    +> | Registers | ABI | CodeGen | --> | Register Allocation | --> | CodeGenContext | --> | Frame | MacroAssembler | Stack |
       |       +-----------+-----+---------+     +---------------------+     +----------------+     +-------+----------------+-------+
       |                      ^     |                                          ^
       +----------------------+     +------------------------------------------+

---

Next steps

As mentioned above, this change doesn't use any of Cranelift's backends for code emission, yet. These are the immediate next steps after landing this change:

• Extract the relevant Cranelift pieces into the cranelift_asm crate, which will be used by Winch
• Swap the string based implementation of the x64 assembler and use the assembler provided by cranelift_asm
• Add initial support for arm64 through cranelift_asm
• Increase test coverage, and include tests in CI. Opted to not do that in this change since the emission mechanism will change, so it might be easier to wait and add verification tests once we are using Cranelift's backends.

---

Misc

Winch's CLI can be used by running cargo run -- path/to/file.wat --target=<target>

Here's a set of WebAssembly programs and their corresponding x64 output:
<details>
<summary>Basic i32 add</summary>

(module
  (export "main" (func $main))

  (func $main (result i32)
    (i32.const 10)
    (i32.const 20)
    i32.add)
)

push rbp
mov rbp, rsp
mov eax, 10
add eax, 20
pop rbp
ret

</details>

<details>
<summary> i32 add with local operators </summary>

(module
  (export "main" (func $main))

  (func $main (result i32)
    (local $foo i32)
    (local $bar i32)
    (i32.const 10)
    (local.set $foo)
    (i32.const 20)
    (local.set $bar)
    (local.get $foo)
    (local.get $bar)
    i32.add
  )
)

push rbp
mov rbp, rsp
sub rsp, 8
mov qword [rsp], 0
mov eax, 10
mov [rsp + 4], eax
mov ecx, 20
mov [rsp], ecx
mov eax, [rsp]
mov edx, [rsp + 4]
add edx, eax
mov rax, rdx
add rsp, 8
pop rbp
ret

</details>

<details>
<summary>Basic i32 with function arguments </summary>

(module
  (export "main" (func $main))

  (func $main (param i32) (param i32) (result i32)
      (local.get 0)
      (local.get 1)
      i32.add
    )
)

push rbp
mov rbp, rsp
sub rsp, 8
mov [rsp + 4], edi
mov [rsp], esi
mov eax, [rsp]
mov ecx, [rsp + 4]
add ecx, eax
mov rax, rcx
add rsp, 8
pop rbp
ret

</details>

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.
• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.
• [ ] This PR contains test cases, if meaningful.
• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (Oct 18 2022 at 18:37):

saulecabrera edited PR #4907 from winch-skeleton to main:

This change introduces the initial skeleton for Winch, the "baseline" compiler, discussed https://github.com/bytecodealliance/rfcs/pull/28

The skeleton contains the just the necessary code for the main abstractions to support the following:

• Function prologue and epilogue (excluding stack checks)
• Calculation of local slots
• An initial pass on a generic MacroAssembler interface
• An initial pass on a x64 Assembler
• A simple CLI tool to compile WebAssembly programs (crates/winch/src/main.rs)
• String based emission for programs that use the following WebAssembly instructions: i32.add, local.get, local.set

As of this change, the high-level structure of the compiler is as follows:

       +--------------------------------------+
       |                                      |
       |                                      |
       |    +-----------------------+         |
       |    |                       |         |
       |    |  +-----------+      +--------+  |
       |    |  | Compiler  | ---> |  ISA   | -+
       |    |  +-----------+      +--------+
       |    |                       |
       |    |                       |
       |    |                       v
       |    |  +-----------+-----+---------+     +---------------------+     +----------------+     +-------+----------------+-------+
       |    +> | Registers | ABI | CodeGen | --> | Register Allocation | --> | CodeGenContext | --> | Frame | MacroAssembler | Stack |
       |       +-----------+-----+---------+     +---------------------+     +----------------+     +-------+----------------+-------+
       |                      ^     |                                          ^
       +----------------------+     +------------------------------------------+

---

Next steps

As mentioned above, this change doesn't use any of Cranelift's backends for code emission, yet. These are the immediate next steps after landing this change:

• Extract the relevant Cranelift pieces into the cranelift_asm crate, which will be used by Winch
• Swap the string based implementation of the x64 assembler and use the assembler provided by cranelift_asm
• Add initial support for arm64 through cranelift_asm
• Increase test coverage, and include tests in CI. Opted to not do that in this change since the emission mechanism will change, so it might be easier to wait and add verification tests once we are using Cranelift's backends.

---

Misc

Winch's CLI can be used by running cargo run -- path/to/file.wat --target=<target>

Here's a set of WebAssembly programs and their corresponding x64 output:
<details>
<summary>Basic i32 add</summary>

(module
  (export "main" (func $main))

  (func $main (result i32)
    (i32.const 10)
    (i32.const 20)
    i32.add)
)

push rbp
mov rbp, rsp
mov eax, 10
add eax, 20
pop rbp
ret

</details>

<details>
<summary> i32 add with local operators </summary>

(module
  (export "main" (func $main))

  (func $main (result i32)
    (local $foo i32)
    (local $bar i32)
    (i32.const 10)
    (local.set $foo)
    (i32.const 20)
    (local.set $bar)
    (local.get $foo)
    (local.get $bar)
    i32.add
  )
)

push rbp
mov rbp, rsp
sub rsp, 8
mov qword [rsp], 0
mov eax, 10
mov [rsp + 4], eax
mov ecx, 20
mov [rsp], ecx
mov eax, [rsp]
mov edx, [rsp + 4]
add edx, eax
mov rax, rdx
add rsp, 8
pop rbp
ret

</details>

<details>
<summary>Basic i32 with function arguments </summary>

(module
  (export "main" (func $main))

  (func $main (param i32) (param i32) (result i32)
      (local.get 0)
      (local.get 1)
      i32.add
    )
)

push rbp
mov rbp, rsp
sub rsp, 8
mov [rsp + 4], edi
mov [rsp], esi
mov eax, [rsp]
mov ecx, [rsp + 4]
add ecx, eax
mov rax, rcx
add rsp, 8
pop rbp
ret

</details>

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.
• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.
• [ ] This PR contains test cases, if meaningful.
• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (Oct 18 2022 at 19:00):

saulecabrera updated PR #4907 from winch-skeleton to main.

Wasmtime GitHub notifications bot (Oct 18 2022 at 19:15):

saulecabrera updated PR #4907 from winch-skeleton to main.

Wasmtime GitHub notifications bot (Oct 18 2022 at 19:17):

saulecabrera edited PR #4907 from winch-skeleton to main:

This change introduces the initial skeleton for Winch, the "baseline" compiler, discussed https://github.com/bytecodealliance/rfcs/pull/28

The skeleton contains the just the necessary code for the main abstractions to support the following:

• Function prologue and epilogue (excluding stack checks)
• Calculation of local slots
• An initial pass on a generic MacroAssembler interface
• An initial pass on a x64 Assembler
• A simple CLI tool to compile WebAssembly programs (crates/winch/src/main.rs)
• String based emission for programs that use the following WebAssembly instructions: i32.add, local.get, local.set

As of this change, the high-level structure of the compiler is as follows:

       +--------------------------------------+
       |                                      |
       |                                      |
       |    +-----------------------+         |
       |    |                       |         |
       |    |  +-----------+      +--------+  |
       |    |  | Compiler  | ---> |  ISA   | -+
       |    |  +-----------+      +--------+
       |    |                       |
       |    |                       |
       |    |                       v
       |    |  +-----------+-----+---------+     +---------------------+     +----------------+     +-------+----------------+-------+
       |    +> | Registers | ABI | CodeGen | --> | Register Allocation | --> | CodeGenContext | --> | Frame | MacroAssembler | Stack |
       |       +-----------+-----+---------+     +---------------------+     +----------------+     +-------+----------------+-------+
       |                      ^     |                                          ^
       +----------------------+     +------------------------------------------+

---

Next steps

As mentioned above, this change doesn't use any of Cranelift's backends for code emission, yet. These are the immediate next steps after landing this change:

• Extract the relevant Cranelift pieces into the cranelift_asm crate, which will be used by Winch
• Swap the string based implementation of the x64 assembler and use the assembler provided by cranelift_asm
• Add initial support for arm64 through cranelift_asm
• Increase test coverage, and include tests in CI. Opted to not do that in this change since the emission mechanism will change, so it might be easier to wait and add verification tests once we are using Cranelift's backends.
• There are a handful of TODOs in the code that are expected to be addressed in the next iterations. I left them there as a reminder mostly; but if preferred they could be removed and placed into issues instead.

---

Misc

Winch's CLI can be used by running cargo run -- path/to/file.wat --target=<target>

Here's a set of WebAssembly programs and their corresponding x64 output:
<details>
<summary>Basic i32 add</summary>

(module
  (export "main" (func $main))

  (func $main (result i32)
    (i32.const 10)
    (i32.const 20)
    i32.add)
)

push rbp
mov rbp, rsp
mov eax, 10
add eax, 20
pop rbp
ret

</details>

<details>
<summary> i32 add with local operators </summary>

(module
  (export "main" (func $main))

  (func $main (result i32)
    (local $foo i32)
    (local $bar i32)
    (i32.const 10)
    (local.set $foo)
    (i32.const 20)
    (local.set $bar)
    (local.get $foo)
    (local.get $bar)
    i32.add
  )
)

push rbp
mov rbp, rsp
sub rsp, 8
mov qword [rsp], 0
mov eax, 10
mov [rsp + 4], eax
mov ecx, 20
mov [rsp], ecx
mov eax, [rsp]
mov edx, [rsp + 4]
add edx, eax
mov rax, rdx
add rsp, 8
pop rbp
ret

</details>

<details>
<summary>Basic i32 with function arguments </summary>

(module
  (export "main" (func $main))

  (func $main (param i32) (param i32) (result i32)
      (local.get 0)
      (local.get 1)
      i32.add
    )
)

push rbp
mov rbp, rsp
sub rsp, 8
mov [rsp + 4], edi
mov [rsp], esi
mov eax, [rsp]
mov ecx, [rsp + 4]
add ecx, eax
mov rax, rcx
add rsp, 8
pop rbp
ret

</details>

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.
• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.
• [ ] This PR contains test cases, if meaningful.
• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (Oct 18 2022 at 19:33):

saulecabrera edited PR #4907 from winch-skeleton to main:

This change introduces the initial skeleton for Winch, the "baseline" compiler, discussed https://github.com/bytecodealliance/rfcs/pull/28

The skeleton contains the just the necessary code for the main abstractions to support the following:

• Function prologue and epilogue (excluding stack checks)
• Calculation of local slots
• An initial pass on a generic MacroAssembler interface
• An initial pass on a x64 Assembler
• A simple CLI tool to compile WebAssembly programs (crates/winch/src/main.rs)
• String based emission for programs that use the following WebAssembly instructions: i32.add, local.get, local.set

As of this change, the high-level structure of the compiler is as follows:

       +--------------------------------------+
       |                                      |
       |                                      |
       |    +-----------------------+         |
       |    |                       |         |
       |    |  +-----------+      +--------+  |
       |    |  | Compiler  | ---> |  ISA   | -+
       |    |  +-----------+      +--------+
       |    |                       |
       |    |                       |
       |    |                       v
       |    |  +-----------+-----+---------+     +---------------------+     +----------------+     +-------+----------------+-------+
       |    +> | Registers | ABI | CodeGen | --> | Register Allocation | --> | CodeGenContext | --> | Frame | MacroAssembler | Stack |
       |       +-----------+-----+---------+     +---------------------+     +----------------+     +-------+----------------+-------+
       |                      ^     |                                          ^
       +----------------------+     +------------------------------------------+

---

Next steps

As mentioned above, this change doesn't use any of Cranelift's backends for code emission, yet. These are the immediate next steps after landing this change:

• Extract the relevant Cranelift pieces into the cranelift_asm crate, which will be used by Winch
• Swap the string based implementation of the x64 assembler and use the assembler provided by cranelift_asm
• Add initial support for arm64 through cranelift_asm
• Improve error handling
• Increase test coverage, and include tests in CI. Opted to not do that in this change since the emission mechanism will change, so it might be easier to wait and add verification tests once we are using Cranelift's backends.
• There are a handful of TODOs in the code that are expected to be addressed in the next iterations. I left them there as a reminder mostly; but if preferred they could be removed and placed into issues instead.

---

Misc

Winch's CLI can be used by running cargo run -- path/to/file.wat --target=<target>

Here's a set of WebAssembly programs and their corresponding x64 output:
<details>
<summary>Basic i32 add</summary>

(module
  (export "main" (func $main))

  (func $main (result i32)
    (i32.const 10)
    (i32.const 20)
    i32.add)
)

push rbp
mov rbp, rsp
mov eax, 10
add eax, 20
pop rbp
ret

</details>

<details>
<summary> i32 add with local operators </summary>

(module
  (export "main" (func $main))

  (func $main (result i32)
    (local $foo i32)
    (local $bar i32)
    (i32.const 10)
    (local.set $foo)
    (i32.const 20)
    (local.set $bar)
    (local.get $foo)
    (local.get $bar)
    i32.add
  )
)

push rbp
mov rbp, rsp
sub rsp, 8
mov qword [rsp], 0
mov eax, 10
mov [rsp + 4], eax
mov ecx, 20
mov [rsp], ecx
mov eax, [rsp]
mov edx, [rsp + 4]
add edx, eax
mov rax, rdx
add rsp, 8
pop rbp
ret

</details>

<details>
<summary>Basic i32 with function arguments </summary>

(module
  (export "main" (func $main))

  (func $main (param i32) (param i32) (result i32)
      (local.get 0)
      (local.get 1)
      i32.add
    )
)

push rbp
mov rbp, rsp
sub rsp, 8
mov [rsp + 4], edi
mov [rsp], esi
mov eax, [rsp]
mov ecx, [rsp + 4]
add ecx, eax
mov rax, rcx
add rsp, 8
pop rbp
ret

</details>

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.
• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.
• [ ] This PR contains test cases, if meaningful.
• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (Oct 18 2022 at 20:15):

saulecabrera updated PR #4907 from winch-skeleton to main.

Wasmtime GitHub notifications bot (Oct 18 2022 at 21:23):

saulecabrera updated PR #4907 from winch-skeleton to main.

Wasmtime GitHub notifications bot (Oct 18 2022 at 22:48):

saulecabrera has marked PR #4907 as ready for review.

Wasmtime GitHub notifications bot (Oct 18 2022 at 23:01):

saulecabrera edited PR #4907 from winch-skeleton to main:

This change introduces the initial skeleton for Winch, the "baseline" compiler, discussed https://github.com/bytecodealliance/rfcs/pull/28

The skeleton contains the just the necessary code for the main abstractions to support the following:

• Function prologue and epilogue (excluding stack checks)
• Calculation of local slots
• An initial pass on a generic MacroAssembler interface
• An initial pass on a x64 Assembler
• A simple CLI tool to compile WebAssembly programs (crates/winch/src/main.rs)
• String based emission for programs that use the following WebAssembly instructions: i32.add, local.get, local.set

This change doesn't fully integrate Winch with Wasmtime yet.

Overview

As of this change, the high-level structure of the compiler is as follows:

       +--------------------------------------+
       |                                      |
       |                                      |
       |    +-----------------------+         |
       |    |                       |         |
       |    |  +-----------+      +--------+  |
       |    |  | Compiler  | ---> |  ISA   | -+
       |    |  +-----------+      +--------+
       |    |                       |
       |    |                       |
       |    |                       v
       |    |  +-----------+-----+---------+     +---------------------+     +----------------+     +-------+----------------+-------+
       |    +> | Registers | ABI | CodeGen | --> | Register Allocation | --> | CodeGenContext | --> | Frame | MacroAssembler | Stack |
       |       +-----------+-----+---------+     +---------------------+     +----------------+     +-------+----------------+-------+
       |                      ^     |                                          ^
       +----------------------+     +------------------------------------------+

---

Next steps

As mentioned above, this change doesn't use any of Cranelift's backends for code emission, yet. These are the immediate next steps after landing this change:

• Extract the relevant Cranelift pieces into the cranelift_asm crate, which will be used by Winch
• Swap the string based implementation of the x64 assembler and use the assembler provided by cranelift_asm
• Add initial support for arm64 through cranelift_asm
• Improve error handling
• Increase test coverage, and include tests in CI. Opted to not do that in this change since the emission mechanism will change, so it might be easier to wait and add verification tests once we are using Cranelift's backends.
• There are a handful of TODOs in the code that are expected to be addressed in the next iterations. I left them there as a reminder mostly; but if preferred they could be removed and placed into issues instead.

---

Misc

Winch's CLI can be used by running cargo run -- path/to/file.wat --target=<target>

Here's a set of WebAssembly programs and their corresponding x64 output:
<details>
<summary>Basic i32 add</summary>

(module
  (export "main" (func $main))

  (func $main (result i32)
    (i32.const 10)
    (i32.const 20)
    i32.add)
)

push rbp
mov rbp, rsp
mov eax, 10
add eax, 20
pop rbp
ret

</details>

<details>
<summary> i32 add with local operators </summary>

(module
  (export "main" (func $main))

  (func $main (result i32)
    (local $foo i32)
    (local $bar i32)
    (i32.const 10)
    (local.set $foo)
    (i32.const 20)
    (local.set $bar)
    (local.get $foo)
    (local.get $bar)
    i32.add
  )
)

push rbp
mov rbp, rsp
sub rsp, 8
mov qword [rsp], 0
mov eax, 10
mov [rsp + 4], eax
mov ecx, 20
mov [rsp], ecx
mov eax, [rsp]
mov edx, [rsp + 4]
add edx, eax
mov rax, rdx
add rsp, 8
pop rbp
ret

</details>

<details>
<summary>Basic i32 with function arguments </summary>

(module
  (export "main" (func $main))

  (func $main (param i32) (param i32) (result i32)
      (local.get 0)
      (local.get 1)
      i32.add
    )
)

push rbp
mov rbp, rsp
sub rsp, 8
mov [rsp + 4], edi
mov [rsp], esi
mov eax, [rsp]
mov ecx, [rsp + 4]
add ecx, eax
mov rax, rcx
add rsp, 8
pop rbp
ret

</details>

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.
• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.
• [ ] This PR contains test cases, if meaningful.
• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (Oct 19 2022 at 19:33):

saulecabrera updated PR #4907 from winch-skeleton to main.

Wasmtime GitHub notifications bot (Oct 19 2022 at 20:42):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Oct 19 2022 at 20:42):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Oct 19 2022 at 20:42):

cfallin created PR review comment:

I think there are two different ways we could go here: a winch crate in its own subdirectory, as cranelift is, or a wasmtime-winch crate in crates/winch with a version number synchronized with the rest of the Wasmtime crates. Given the dependency on wasmtime-environ below I think it's probably more properly considered part of the Wasmtime family of crates, but if eventually factor that out, then it at least in theory could be its own thing usable separately (in the way that Cranelift is).

but tl;dr, a crate in crates/$NAME is part of Wasmtime and is named as wasmtime-$NAME...

Wasmtime GitHub notifications bot (Oct 19 2022 at 20:42):

cfallin created PR review comment:

Comment here too that this is a temporary dependency?

Wasmtime GitHub notifications bot (Oct 19 2022 at 20:42):

cfallin created PR review comment:

word_size should probably be obtained from the MacroAssembler or other platform-specific trait?

Wasmtime GitHub notifications bot (Oct 19 2022 at 20:42):

cfallin created PR review comment:

Can we factor out this logic into a memset helper on the MacroAssembler, that either unrolls a short sequence of stores or calls a helper?

Wasmtime GitHub notifications bot (Oct 19 2022 at 20:42):

cfallin created PR review comment:

I'll note here but applies generally: overall in doc-comments we've tried to stick to capitalized-with-periods-at-end style; many comments here don't end with a ..

Wasmtime GitHub notifications bot (Oct 19 2022 at 20:42):

cfallin created PR review comment:

If we want to break the dependency on Wasmtime, the way to do that would be to have a crates/winch crate analogous to crates/cranelift (wasmtime-cranelift) that pulls in the actual Winch toplevel crate (just winch, or winch-codegen or somesuch) and impls this Wasmtime trait. We can always refactor in that direction later, but if you wanted to do it now I think that could be reasonable as well.

Wasmtime GitHub notifications bot (Oct 19 2022 at 20:42):

cfallin created PR review comment:

s/Aarch/AArch64/

Wasmtime GitHub notifications bot (Oct 19 2022 at 20:42):

cfallin created PR review comment:

References to SpiderMonkey are fine here but for the benefit of readers later, links to the source (via searchfox) might be helpful -- so e.g. starting out with "This algorithm is similar to SpiderMonkey's frame layout logic in LINK... The main differences are: ..."

Wasmtime GitHub notifications bot (Oct 19 2022 at 20:42):

cfallin created PR review comment:

We should assert here that there is only one result type, right? (I think that wasmparser probably accepts multi-value Wasm by default now)

Doing the fully general thing is fine to defer to later, but we'll need to support it at some point; in Cranelift our ABI uses the arg regs (x0-x7 on aarch64) as eight returns and then has a "return value area" on the stack for more than that...

Wasmtime GitHub notifications bot (Oct 19 2022 at 20:42):

cfallin created PR review comment:

We could also have dynamic settings on the TargetIsa and skip the builder abstraction entirely -- in general I'm open to reconsidering complexity here (as more important than exact analogues to Cranelift design).

Wasmtime GitHub notifications bot (Oct 19 2022 at 20:42):

cfallin created PR review comment:

s/endianess/endianness/ (not a real word or in dictionaries! But that's at least how Wikipedia spells it, in addition to target_lexicon)

Wasmtime GitHub notifications bot (Oct 19 2022 at 20:42):

cfallin created PR review comment:

name should be aarch64 here?

Wasmtime GitHub notifications bot (Oct 19 2022 at 20:42):

cfallin created PR review comment:

s/byets/bytes/

Wasmtime GitHub notifications bot (Oct 19 2022 at 20:42):

cfallin created PR review comment:

A general thought about two-arg vs three-arg forms (I recall having some sort of this conversation before, but don't remember all of our conclusions): would it make sense maybe to define the three-arg form as canonical in the trait, and then add a move here in the x64 implementation (mov dst, src1; add dst, src2)? And skip the move if dst == src1? That to me seems a little more general, as x64 can support it with no reduction in code quality (the move otherwise has to be generated by the translator driving the macroassembler) while it allows aarch64 and other three-arg RISCs to possibly generate better code.

Wasmtime GitHub notifications bot (Oct 19 2022 at 20:42):

cfallin created PR review comment:

If we move this crate into winch/codegen/, then the winch/ subdirectory can contain a crate winch-cli or winch-tools, just like Cranelift, and winch/src/main.rs is the main entrypoint for a winch-cli command. I like that a little more than having a main.rs inside Wasmtime's crates/ that is unrelated to Wasmtime. Thoughts?

Wasmtime GitHub notifications bot (Oct 19 2022 at 20:42):

cfallin created PR review comment:

It's worth writing a bit more about how this will extend to cases where we really do want machine-specific lowerings for Wasm ops; SIMD will be a forcing function there, if not before, I think. E.g., do we have a way for the Wasm-op translator to delegate to some machine-specific sequence of calls to masm? Or maybe we just put direct Wasm-op equivalents on MacroAssembler in that case and do the right thing on each platform. (Actually I like the latter approach best, so maybe it's OK that everything goes through this with no other conditionals/hooks.)

Wasmtime GitHub notifications bot (Oct 19 2022 at 20:42):

cfallin created PR review comment:

Ah, here we go; commented on this above but now that I read this, I'm glad it's already captured! Happy to see this kept as a TODO, if you don't want to do the refactor right away (though I suspect it may be an easy change and easier now than later as the instructions are built out)

Wasmtime GitHub notifications bot (Oct 19 2022 at 20:42):

cfallin created PR review comment:

Ah, now I understand why two-arg add is kind of OK actually: Wasm add (and other binops) always consume their args, so it's fine to overwrite dst. That actually makes a lot of sense to me, so maybe it's fine to keep two-arg forms of the masm methods, unless it later hinders lowerings of e.g. SIMD ops or address computations or ... that use temps.

Wasmtime GitHub notifications bot (Oct 19 2022 at 20:42):

cfallin created PR review comment:

A short doc-comment on the approach would be helpful here -- something about how it's a single-pass allocator, taking registers from a freelist until we run out then spill everything (or something like that)?

Wasmtime GitHub notifications bot (Oct 19 2022 at 20:42):

cfallin created PR review comment:

I'm wondering if it's possible to avoid the complexity of these macros -- I don't like them for several reasons:

• It duplicates knowledge here (the whole set of Wasm opcodes) unnecessarily;
• It's very unreadable;
• The dispatching mechanism feels somewhat "magic" as a result, in a bad way (I can't trace what's going on).

I think I'd prefer a toplevel translation driver loop that's something like:

for op in operators {
  match op {
    Operator::Add => self.visit_add();
    ...
  }
}

maybe with that match in a handwritten fn visit(). In other words, just expand out what this macro would generate by hand.

Or said another way: it seems these macros are only used once. Given that, why not expand them in the place that they're used, to reduce complexity?

Wasmtime GitHub notifications bot (Oct 19 2022 at 20:42):

cfallin created PR review comment:

(Ah, reading further below to the x64 version, I see an assert there, so I think it's just missing here)

Wasmtime GitHub notifications bot (Oct 20 2022 at 12:03):

saulecabrera updated PR #4907 from winch-skeleton to main.

Wasmtime GitHub notifications bot (Oct 20 2022 at 12:08):

saulecabrera submitted PR review.

Wasmtime GitHub notifications bot (Oct 20 2022 at 12:08):

saulecabrera created PR review comment:

I'm wondering if it's possible to avoid the complexity of these macros

Yeah, definitely possible. I've addressed this in https://github.com/bytecodealliance/wasmtime/pull/4907/commits/147f901ea630bb39b2a59fa956293c6c232f4e41

FWIW, I like the visitor pattern recently introduced in wasmparser, but when I was exploring introducing it I couldn't find a cleaner way to manage the support/unsupported operator sets, which led to duplication. I'd still like to keep the implementation of the visit function and each of the emitters in the visit.rs module though; I expect that this file is going to get very big as more operators are added.

Wasmtime GitHub notifications bot (Oct 20 2022 at 12:20):

saulecabrera edited PR review comment.

Wasmtime GitHub notifications bot (Oct 20 2022 at 14:43):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Oct 20 2022 at 14:43):

alexcrichton created PR review comment:

Given that winch's goal is compilation speed I might caution against going this direction since the Operator-based approach to both validation and translation is known to be double-digit percentage slower when at least purely validating and would likely have similar impact to the design of winch as well.

I won't disagree that the enum-based approach is clearer to read when only a small handful of opcodes are implemented, but if you'd like I can try to help manage the macro-complexity. For example a reasonable approach might be:

• Use the macro to generate a visitor which recursively calls two other visitors. The first of the two visitors would be the validator itself and the second visitor could be a winch-defined visitor for per-op translations.
• The winch-defined visitor would be defined as usual with manually-written methods per op. You would then need to also maintain a macro with a list of handwritten ops so it could serve as acting as a catch-all for all unimplemented ops to return an error.

Overall I'm hesitant to prioritize readability ergonomics here given the impact on speed. This is all equally applicable to Cranelift-based compilation but the speedup there is likely small-to-unmeasurable given that validation is such a small portion of translation time.

Wasmtime GitHub notifications bot (Oct 20 2022 at 15:26):

saulecabrera submitted PR review.

Wasmtime GitHub notifications bot (Oct 20 2022 at 15:26):

saulecabrera created PR review comment:

I think it's reasonable to do this since this is common for all ISAs and will be cleaner if we put this in the masm. A few clarification questions:

1. With this logic you mean the entire piece to zero out a locals range?
2. By helper, is your idea to have this in the masm module, but not necessarily part of the trait?

Wasmtime GitHub notifications bot (Oct 20 2022 at 15:53):

saulecabrera submitted PR review.

Wasmtime GitHub notifications bot (Oct 20 2022 at 15:53):

saulecabrera created PR review comment:

Thanks for chiming in here @alexcrichton; I wasn't aware of the speed benefits; I wrongly assumed the change was mostly around ergonomics. But taking a closer look at the details and some of the comments, your comment makes sense to me.

I'll try out your suggestion, hopefully it'll be less complex that my previous attempt.

Wasmtime GitHub notifications bot (Oct 20 2022 at 15:57):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Oct 20 2022 at 15:57):

alexcrichton created PR review comment:

I do realize that macros are nontrivial to work with for sure though and I'm still not 100% satisfied with where wasmparser ended up. If you'd like some help I'm happy to assist, or additionally at this point getting something working is probably more prudent than the highest levels of optimization, so I'm happy to try to come work on this small part as a follow-up if you'd prefer too

Wasmtime GitHub notifications bot (Oct 20 2022 at 16:32):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Oct 20 2022 at 16:32):

cfallin created PR review comment:

Given that winch's goal is compilation speed I might caution against going this direction since the Operator-based approach to both validation and translation is known to be double-digit percentage slower when at least purely validating and would likely have similar impact to the design of winch as well.

Ah, OK, I think we're slightly talking past each other, and my hand expansion into a match was based on my misunderstanding of what this was generating (sorry!). My main point though wasn't about the actual matching/control-flow strategy, but about the way we generate it.

Basically my point can be reduced to: there are some macros here; each macro is invoked once; can we inline the macros' expansion at that point instead? (Since this doesn't change what eventually gets compiled into the winch crate, it should be completely performance-neutral.) It didn't seem like the redundancy that the macros reduced (method args, ..) was that significant compared to the complexity of having the macros at all. And my confusion on what they were actually doing is only slightly more evidence of (either my denseness or...) the confusion imparted by macro use :-)

Wasmtime GitHub notifications bot (Oct 20 2022 at 16:48):

saulecabrera submitted PR review.

Wasmtime GitHub notifications bot (Oct 20 2022 at 16:48):

saulecabrera created PR review comment:

Basically my point can be reduced to: there are some macros here; each macro is invoked once; can we inline the macros' expansion at that point instead?

I think that in this case, the approach would be to manually define each method expected by the visitor trait, and error out on the ones that are not supported. This satisfies the readability and the performance aspects, at the cost of having to manually define a method per operator. I'd would still like to explore if there's a middle ground in which we are able to use macros without hindering too much the readability, (e.g. following Alex's suggestion of only having a single list of unsupported operators) and perhaps the complexity can be reduced further with comments? And, thinking a bit more, if we go with this approach, as we support more and more operators, this "unsupported" list is going to shrink over time.

Wasmtime GitHub notifications bot (Oct 20 2022 at 18:19):

saulecabrera updated PR #4907 from winch-skeleton to main.

Wasmtime GitHub notifications bot (Oct 20 2022 at 18:24):

saulecabrera updated PR #4907 from winch-skeleton to main.

Wasmtime GitHub notifications bot (Oct 21 2022 at 22:26):

saulecabrera updated PR #4907 from winch-skeleton to main.

Wasmtime GitHub notifications bot (Oct 21 2022 at 22:29):

saulecabrera updated PR #4907 from winch-skeleton to main.

Wasmtime GitHub notifications bot (Oct 21 2022 at 22:35):

saulecabrera created PR review comment:

Yeah this is a good idea, I've completed this and I'm happy how things are shaping up.

Wasmtime GitHub notifications bot (Oct 21 2022 at 22:35):

saulecabrera submitted PR review.

Wasmtime GitHub notifications bot (Oct 21 2022 at 22:36):

saulecabrera submitted PR review.

Wasmtime GitHub notifications bot (Oct 21 2022 at 22:36):

saulecabrera created PR review comment:

I've tackled this too, I think that it's better to do this earlier rather than later.

Wasmtime GitHub notifications bot (Oct 21 2022 at 22:37):

saulecabrera created PR review comment:

I've tackled this as part of restructuring the multiple Winch crates, so now we have this tool in it's own directory, not related with Wasmtime at all.

ref: https://github.com/bytecodealliance/wasmtime/commit/3c6f23acb5b992fe936ec4bd9dd28f8077bd8580

Wasmtime GitHub notifications bot (Oct 21 2022 at 22:37):

saulecabrera submitted PR review.

Wasmtime GitHub notifications bot (Oct 21 2022 at 22:41):

saulecabrera created PR review comment:

Had a chat with Alex (thanks for the help Alex!) in which we were able to simplify the macro. @cfallin let me know if you have further thoughts on this; I'd like to keep a macro if possible, assuming that it will temporary as more operators are supported. If not I'm happy to manually define all the Visitor traits in a follow up PR. In general I feel that this is cleaner than my previous attempt.

Wasmtime GitHub notifications bot (Oct 21 2022 at 22:41):

saulecabrera submitted PR review.

Wasmtime GitHub notifications bot (Oct 21 2022 at 22:49):

cfallin created PR review comment:

Right, the entire bit to zero a range of memory; and I think it'd be pretty reasonable to have it as a trait method on MacroAssembler that has a default body (which does some basic lowering to either unrolled stores or a loop that will work on any architecture). That also leaves room for more efficient implementations per machine later (e.g. rep movsb on x86 machines with ERMS where that's fast, or something using SIMD, or ...).

Wasmtime GitHub notifications bot (Oct 21 2022 at 22:49):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Oct 21 2022 at 23:12):

saulecabrera edited PR review comment.

Wasmtime GitHub notifications bot (Oct 21 2022 at 23:15):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Oct 21 2022 at 23:15):

cfallin created PR review comment:

OK, yeah, I'm happy with how this looks -- thank you for iterating on this!

Wasmtime GitHub notifications bot (Oct 21 2022 at 23:27):

saulecabrera edited PR review comment.

Wasmtime GitHub notifications bot (Oct 23 2022 at 16:41):

saulecabrera updated PR #4907 from winch-skeleton to main.

Wasmtime GitHub notifications bot (Oct 23 2022 at 16:44):

saulecabrera updated PR #4907 from winch-skeleton to main.

Wasmtime GitHub notifications bot (Oct 23 2022 at 22:44):

saulecabrera updated PR #4907 from winch-skeleton to main.

Wasmtime GitHub notifications bot (Oct 23 2022 at 22:46):

saulecabrera created PR review comment:

Makes sense to me; I've factored this out into a method in the trait with a default implementation. I've also made the code a bit simpler.

Wasmtime GitHub notifications bot (Oct 23 2022 at 22:46):

saulecabrera submitted PR review.

Wasmtime GitHub notifications bot (Oct 23 2022 at 22:49):

saulecabrera updated PR #4907 from winch-skeleton to main.

Wasmtime GitHub notifications bot (Oct 24 2022 at 11:13):

saulecabrera updated PR #4907 from winch-skeleton to main.

Wasmtime GitHub notifications bot (Oct 24 2022 at 15:31):

saulecabrera updated PR #4907 from winch-skeleton to main.

Wasmtime GitHub notifications bot (Oct 24 2022 at 15:33):

saulecabrera updated PR #4907 from winch-skeleton to main.

Wasmtime GitHub notifications bot (Oct 24 2022 at 15:45):

saulecabrera submitted PR review.

Wasmtime GitHub notifications bot (Oct 24 2022 at 15:45):

saulecabrera created PR review comment:

Yes, it's actually obtained from each ISA's ABI. I've modified this to make more obvious, so instead of passing in a u32 to the CodeGen constructor I'm passing in the ABI.

Wasmtime GitHub notifications bot (Oct 24 2022 at 15:46):

saulecabrera updated PR #4907 from winch-skeleton to main.

Wasmtime GitHub notifications bot (Oct 24 2022 at 15:48):

saulecabrera edited PR review comment.

Wasmtime GitHub notifications bot (Oct 24 2022 at 18:03):

saulecabrera updated PR #4907 from winch-skeleton to main.

Wasmtime GitHub notifications bot (Oct 24 2022 at 18:30):

saulecabrera updated PR #4907 from winch-skeleton to main.

Wasmtime GitHub notifications bot (Oct 24 2022 at 18:50):

saulecabrera updated PR #4907 from winch-skeleton to main.

Wasmtime GitHub notifications bot (Oct 24 2022 at 19:54):

saulecabrera updated PR #4907 from winch-skeleton to main.

Wasmtime GitHub notifications bot (Oct 25 2022 at 12:42):

saulecabrera updated PR #4907 from winch-skeleton to main.

Wasmtime GitHub notifications bot (Oct 25 2022 at 12:44):

saulecabrera submitted PR review.

Wasmtime GitHub notifications bot (Oct 25 2022 at 12:44):

saulecabrera created PR review comment:

Done, thanks! Sorry for not noticing this earlier!

Wasmtime GitHub notifications bot (Oct 25 2022 at 12:45):

saulecabrera updated PR #4907 from winch-skeleton to main.

Wasmtime GitHub notifications bot (Oct 25 2022 at 12:59):

saulecabrera updated PR #4907 from winch-skeleton to main.

Wasmtime GitHub notifications bot (Oct 25 2022 at 13:01):

saulecabrera created PR review comment:

That's indeed something that came to mind when looking at this piece of code. I didn't go really deep on this idea though; but I'm going to keep in mind for whe we need to introduce this for Winch.

Wasmtime GitHub notifications bot (Oct 25 2022 at 13:01):

saulecabrera submitted PR review.

Wasmtime GitHub notifications bot (Oct 25 2022 at 13:01):

saulecabrera edited PR review comment.

Wasmtime GitHub notifications bot (Oct 25 2022 at 15:29):

saulecabrera updated PR #4907 from winch-skeleton to main.

Wasmtime GitHub notifications bot (Oct 25 2022 at 15:46):

saulecabrera updated PR #4907 from winch-skeleton to main.

Wasmtime GitHub notifications bot (Oct 25 2022 at 17:50):

saulecabrera updated PR #4907 from winch-skeleton to main.

Wasmtime GitHub notifications bot (Oct 25 2022 at 17:52):

saulecabrera submitted PR review.

Wasmtime GitHub notifications bot (Oct 25 2022 at 17:52):

saulecabrera created PR review comment:

Added, thanks!

Wasmtime GitHub notifications bot (Oct 25 2022 at 18:01):

saulecabrera updated PR #4907 from winch-skeleton to main.

Wasmtime GitHub notifications bot (Oct 25 2022 at 22:18):

saulecabrera updated PR #4907 from winch-skeleton to main.

Wasmtime GitHub notifications bot (Oct 25 2022 at 22:21):

saulecabrera created PR review comment:

I recall having some sort of this conversation before, but don't remember all of our conclusions

Yeah we concluded that three-arg form is probably better and that we should go with it. My original intention was to do this in the next iteration once I add some actual codegen for aarch64 but while doing the refactoring I took care of it.

Wasmtime GitHub notifications bot (Oct 25 2022 at 22:21):

saulecabrera submitted PR review.

Wasmtime GitHub notifications bot (Oct 25 2022 at 22:21):

saulecabrera created PR review comment:

Agreed, I've added this, thanks!

Wasmtime GitHub notifications bot (Oct 25 2022 at 22:21):

saulecabrera submitted PR review.

Wasmtime GitHub notifications bot (Oct 25 2022 at 22:27):

saulecabrera requested cfallin for a review on PR #4907.

Wasmtime GitHub notifications bot (Oct 25 2022 at 22:32):

saulecabrera updated PR #4907 from winch-skeleton to main.

Wasmtime GitHub notifications bot (Oct 25 2022 at 23:05):

saulecabrera updated PR #4907 from winch-skeleton to main.

Wasmtime GitHub notifications bot (Oct 25 2022 at 23:15):

saulecabrera updated PR #4907 from winch-skeleton to main.

Wasmtime GitHub notifications bot (Oct 25 2022 at 23:34):

saulecabrera updated PR #4907 from winch-skeleton to main.

Wasmtime GitHub notifications bot (Oct 25 2022 at 23:36):

saulecabrera updated PR #4907 from winch-skeleton to main.

Wasmtime GitHub notifications bot (Oct 27 2022 at 19:48):

saulecabrera updated PR #4907 from winch-skeleton to main.

Wasmtime GitHub notifications bot (Oct 27 2022 at 20:20):

saulecabrera updated PR #4907 from winch-skeleton to main.

Wasmtime GitHub notifications bot (Oct 27 2022 at 20:42):

saulecabrera updated PR #4907 from winch-skeleton to main.

Wasmtime GitHub notifications bot (Oct 28 2022 at 11:55):

saulecabrera updated PR #4907 from winch-skeleton to main.

Wasmtime GitHub notifications bot (Oct 28 2022 at 11:57):

saulecabrera updated PR #4907 from winch-skeleton to main.

Wasmtime GitHub notifications bot (Oct 28 2022 at 18:48):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Oct 28 2022 at 18:48):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Oct 28 2022 at 18:48):

cfallin created PR review comment:

Here we need an assert that storing just one i32 will bump the start address up to word_size-aligned, right? (I guess one can show this is always the case on 64-bit systems; if not 8-byte aligned, but 4-byte aligned, one can be at most 4 bytes off; but that conclusion isn't generic to word size so I'd rather at least assert it here.)

Wasmtime GitHub notifications bot (Oct 28 2022 at 20:28):

saulecabrera updated PR #4907 from winch-skeleton to main.

Wasmtime GitHub notifications bot (Oct 28 2022 at 20:30):

saulecabrera created PR review comment:

Fixed; and yeah good catch; definitely I had assumed what you pointed out, that it's always the case in 64-bit systems.

Wasmtime GitHub notifications bot (Oct 28 2022 at 20:30):

saulecabrera submitted PR review.

Wasmtime GitHub notifications bot (Oct 28 2022 at 20:31):

saulecabrera updated PR #4907 from winch-skeleton to main.

Wasmtime GitHub notifications bot (Oct 28 2022 at 20:45):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Oct 28 2022 at 21:19):

cfallin merged PR #4907.

Last updated: Oct 23 2024 at 20:03 UTC