Skip to content

Add v128.const support to Winch#8990

Merged
saulecabrera merged 8 commits intobytecodealliance:mainfrom
jeffcharles:winch-add-v128-const
Jul 24, 2024
Merged

Add v128.const support to Winch#8990
saulecabrera merged 8 commits intobytecodealliance:mainfrom
jeffcharles:winch-add-v128-const

Conversation

@jeffcharles
Copy link
Copy Markdown
Contributor

Adding support for v128.const to Winch. Start of work to add SIMD support to Winch for the x64 architecture.

}

#[wasmtime_test(wasm_features(simd))]
#[wasmtime_test]
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if we want to keep the wasm_features(simd) check on here since this is a SIMD test. I removed it so the test would run on Winch.

;; movss (%rsp), %xmm15
;; addq $4, %rsp
;; movl %r11d, (%rax)
;; movss %xmm15, (%rax)
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a result of changing how memory values are popped from the stack. It now uses the type of the Wasm value to determine the scratch register to use so this now uses the XMM scratch register instead of a GPR.

;; movss (%rsp), %xmm15
;; addq $4, %rsp
;; movl %r11d, 8(%rax)
;; movss %xmm15, 8(%rax)
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto for this change to the disass.

}
Val::Memory(_) => {
let scratch = scratch!(M);
let scratch = scratch!(M, &ty);
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This now needs to handle 128 bit values so using the GPR scratch won't work.

@saulecabrera saulecabrera self-requested a review July 22, 2024 18:29
@jeffcharles jeffcharles marked this pull request as ready for review July 22, 2024 18:56
@jeffcharles jeffcharles requested review from a team as code owners July 22, 2024 18:56
@jeffcharles jeffcharles requested review from elliottt and removed request for a team July 22, 2024 18:56
@saulecabrera saulecabrera removed request for a team and elliottt July 22, 2024 19:13
@github-actions github-actions bot added the winch Winch issues or pull requests label Jul 22, 2024
@github-actions
Copy link
Copy Markdown

Subscribe to Label Action

cc @saulecabrera

Details This issue or pull request has been labeled: "winch"

Thus the following users have been cc'd because of the following labels:

  • saulecabrera: winch

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

Copy link
Copy Markdown
Member

@saulecabrera saulecabrera left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's going on a good direction!

One thing that we need to also update as part of this PR is the stack alignment details in the ABI.

Currently, the stack alignment and slot size are both set to 8 bytes, given that types > 8 bytes were not supported. We'd probably want to ensure that it's now set to 16.

@jeffcharles
Copy link
Copy Markdown
Contributor Author

The merge commit removed the new vector method on the ABI trait. Not sure if you would like me to do the same with the next_vr and associated methods I've added in the X64ABI file.

@jeffcharles jeffcharles requested a review from saulecabrera July 23, 2024 17:55
@saulecabrera
Copy link
Copy Markdown
Member

The merge commit removed the new vector method on the ABI trait. Not sure if you would like me to do the same with the next_vr and associated methods I've added in the X64ABI file.

Yeah it sounds reasonable to me to get rid of next_vr and vector_reg_for 👍🏽

Copy link
Copy Markdown
Member

@saulecabrera saulecabrera left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great to me, thanks!

@saulecabrera saulecabrera added this pull request to the merge queue Jul 24, 2024
Merged via the queue into bytecodealliance:main with commit fa9a948 Jul 24, 2024
@jeffcharles jeffcharles deleted the winch-add-v128-const branch July 24, 2024 19:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

winch Winch issues or pull requests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants