wasmtime / PR #5581 winch: Use cranelift-codegen x64 back... · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #5581 winch: Use cranelift-codegen x64 back...

Wasmtime GitHub notifications bot (Jan 17 2023 at 14:45):

saulecabrera opened PR #5581 from winch-codegen-use-x64-backend to main:

This change substitutes the string based emission mechanism with cranelift-codegen's x64 backend.

This change _does not_:

Introduce new functionality in terms of supported instructions.
Change the semantics of the assembler/macroassembler in terms of the logic to emit instructions.

The most notable differences between this change and the previous version are:

Handling of shared flags and ISA-specific flags, which for now are left with the default value.
Simplification of instruction emission per operand size: previously the assembler defined different methods depending on the operand size (e.g. mov for 64 bits, and movl for 32 bits). This change updates such approach so that each assembler method takes an operand size as a parameter, reducing duplication and making the code more concise and easier to integrate with the x64's Inst enum.
Introduction of a disassembler for testing purposes.

As of this change, Winch generates the following code for the following test programs:

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

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

   0:   55                      push    rbp
   1:   48 89 e5                mov rbp, rsp
   4:   b8 0a 00 00 00          mov eax, 0xa
   9:   83 c0 14                add eax, 0x14
   c:   5d                      pop rbp
   d:   c3                      ret

(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
        ))

   0:   55                      push    rbp
   1:   48 89 e5                mov rbp, rsp
   4:   48 83 ec 08             sub rsp, 8
   8:   48 c7 04 24 00 00 00 00 mov qword ptr [rsp], 0
  10:   b8 0a 00 00 00          mov eax, 0xa
  15:   89 44 24 04             mov dword ptr [rsp + 4], eax
  19:   b8 14 00 00 00          mov eax, 0x14
  1e:   89 04 24                mov dword ptr [rsp], eax
  21:   8b 04 24                mov eax, dword ptr [rsp]
  24:   8b 4c 24 04             mov ecx, dword ptr [rsp + 4]
  28:   01 c1                   add ecx, eax
  2a:   48 89 c8                mov rax, rcx
  2d:   48 83 c4 08             add rsp, 8
  31:   5d                      pop rbp
  32:   c3                      ret

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

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

   0:   55                      push    rbp
   1:   48 89 e5                mov rbp, rsp
   4:   48 83 ec 08             sub rsp, 8
   8:   89 7c 24 04             mov dword ptr [rsp + 4], edi
   c:   89 34 24                mov dword ptr [rsp], esi
   f:   8b 04 24                mov eax, dword ptr [rsp]
  12:   8b 4c 24 04             mov ecx, dword ptr [rsp + 4]
  16:   01 c1                   add ecx, eax
  18:   48 89 c8                mov rax, rcx
  1b:   48 83 c4 08             add rsp, 8
  1f:   5d                      pop rbp
  20:   c3                      ret

<!--

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 (Jan 17 2023 at 14:45):

saulecabrera requested cfallin for a review on PR #5581.

Wasmtime GitHub notifications bot (Jan 17 2023 at 15:14):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Jan 17 2023 at 15:14):

bjorn3 created PR review comment:

isa::Builder has a triple() method.

Wasmtime GitHub notifications bot (Jan 17 2023 at 22:23):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Jan 17 2023 at 22:23):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Jan 17 2023 at 22:23):

cfallin created PR review comment:

Could this dep be defined at the workspace level (and shared with clif-util) similar to the others above?

Wasmtime GitHub notifications bot (Jan 17 2023 at 22:23):

cfallin created PR review comment:

pre-existing but do we want to use .and_then(|| self.emit_body(...)) and similar below as well? Otherwise if there's an error emitting the prologue we still emit the rest, then eventually return the error.

Wasmtime GitHub notifications bot (Jan 17 2023 at 22:23):

cfallin created PR review comment:

s/Crate/Create/ (and s/a x64/an x64/ if I'm being pedantic, though not too important!)

Wasmtime GitHub notifications bot (Jan 17 2023 at 23:33):

saulecabrera submitted PR review.

Wasmtime GitHub notifications bot (Jan 17 2023 at 23:33):

saulecabrera created PR review comment:

I considered it, but didn't because clif-util declares capstone as optional, bound to the disas feature, and as far as I know workspace dependencies can't be optional.

Wasmtime GitHub notifications bot (Jan 17 2023 at 23:37):

saulecabrera submitted PR review.

Wasmtime GitHub notifications bot (Jan 17 2023 at 23:37):

saulecabrera created PR review comment:

Yeah, I noticed that, but I don't have a concrete use-case for it at the moment. I'll add one when needed.

Wasmtime GitHub notifications bot (Jan 17 2023 at 23:38):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Jan 17 2023 at 23:38):

cfallin created PR review comment:

Ah, right, that's fine then.

Wasmtime GitHub notifications bot (Jan 17 2023 at 23:55):

saulecabrera updated PR #5581 from winch-codegen-use-x64-backend to main.

Wasmtime GitHub notifications bot (Jan 17 2023 at 23:56):

saulecabrera submitted PR review.

Wasmtime GitHub notifications bot (Jan 17 2023 at 23:56):

saulecabrera created PR review comment:

Actually upon verification it works fine, my bad. I confused using a workspace dependency as optional vs _declaring_ a workspace dependency as optional; and the latter is what doesn't work.

Wasmtime GitHub notifications bot (Jan 17 2023 at 23:57):

saulecabrera submitted PR review.

Wasmtime GitHub notifications bot (Jan 17 2023 at 23:57):

saulecabrera created PR review comment:

Fixed!

Wasmtime GitHub notifications bot (Jan 17 2023 at 23:57):

saulecabrera submitted PR review.

Wasmtime GitHub notifications bot (Jan 17 2023 at 23:57):

saulecabrera created PR review comment:

Fixed!

Wasmtime GitHub notifications bot (Jan 18 2023 at 11:58):

saulecabrera merged PR #5581.

Last updated: Dec 23 2024 at 12:05 UTC