Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

winch: Support abs and neg for f32 and f64 on x64 #6982

Merged
merged 5 commits into from
Sep 10, 2023

Conversation

elliottt
Copy link
Member

@elliottt elliottt commented Sep 8, 2023

Adds support for f32.abs, f64.abs, f32.neg, and f64.neg to winch. Additionally, this adds a gpr_to_xmm function to the x64 backend to allow loading a constant and moving it to an xmm register. It also reserves xmm15 as the scratch xmm register.

@elliottt elliottt requested a review from a team as a code owner September 8, 2023 22:09
@elliottt elliottt requested review from fitzgen and removed request for a team September 8, 2023 22:09
Copy link
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.

Generally LGTM, thanks! A couple of minor comments:

  • Spec tests: we could copy the <f32|f64>.abs test cases from tests/spec_testsuite/<f32|f64>_bitwise.wast into tests/misc_testsuite/winch/<f32|f64>_bitwise.wast to get test for these instructions additional to the filetests. This process is a bit manual at the moment, unfortunately, but the idea is that once Winch reaches support for Core Wasm this process should go away almost entirely.
  • Add f32.abs and f64.abs to the list of supported instructions to the differential fuzzer in https://github.com/bytecodealliance/wasmtime/blob/main/fuzz/fuzz_targets/differential.rs#L289 (I run the differential fuzzer locally from time to time. The idea is to enable fuzzing by default once Winch reaches parity with Core Wasm).

winch/codegen/src/isa/x64/asm.rs Outdated Show resolved Hide resolved
winch/codegen/src/isa/x64/masm.rs Show resolved Hide resolved
@github-actions github-actions bot added the winch Winch issues or pull requests label Sep 9, 2023
@github-actions
Copy link

github-actions bot commented Sep 9, 2023

Subscribe to Label Action

cc @saulecabrera

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.

@elliottt elliottt requested review from a team as code owners September 9, 2023 06:37
@elliottt elliottt changed the title winch: Support f32.abs and f64.abs on x64 winch: Support abs and neg for f32 and f64 on x64 Sep 9, 2023
@github-actions github-actions bot added the fuzzing Issues related to our fuzzing infrastructure label Sep 9, 2023
@github-actions
Copy link

github-actions bot commented Sep 9, 2023

Subscribe to Label Action

cc @fitzgen

This issue or pull request has been labeled: "fuzzing"

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

  • fitzgen: fuzzing

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

Learn more.

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

LGTM, thanks!

@saulecabrera saulecabrera added this pull request to the merge queue Sep 10, 2023
Merged via the queue into bytecodealliance:main with commit 9f00198 Sep 10, 2023
18 checks passed
saulecabrera added a commit to saulecabrera/wasmtime that referenced this pull request Sep 11, 2023
This change is a small refactoring to some of the MacroAssembler functions to
use `Reg` instead of `RegImm` where appropriate (e.g. when the operand is a
destination).

@elliottt pointed this out while working on bytecodealliance#6982

This change also changes the signature of `float_abs` and `float_neg`, which can
be simplified to take a single register.
github-merge-queue bot pushed a commit that referenced this pull request Sep 12, 2023
This change is a small refactoring to some of the MacroAssembler functions to
use `Reg` instead of `RegImm` where appropriate (e.g. when the operand is a
destination).

@elliottt pointed this out while working on #6982

This change also changes the signature of `float_abs` and `float_neg`, which can
be simplified to take a single register.
alexcrichton pushed a commit that referenced this pull request Sep 12, 2023
* winch: Support f32.abs and f64.abs on x64

Co-authored-by: Nick Fitzgerald <[email protected]>

* Add an implementation of f32.neg and f64.neg

* Enable spec tests for winch with f{32,64}.{neg,abs}

* Enable differential fuzzing for f{32,64}.{neg,abs} for winch

* Comments from code review

---------

Co-authored-by: Nick Fitzgerald <[email protected]>
eduardomourar pushed a commit to eduardomourar/wasmtime that referenced this pull request Sep 13, 2023
)

* winch: Support f32.abs and f64.abs on x64

Co-authored-by: Nick Fitzgerald <[email protected]>

* Add an implementation of f32.neg and f64.neg

* Enable spec tests for winch with f{32,64}.{neg,abs}

* Enable differential fuzzing for f{32,64}.{neg,abs} for winch

* Comments from code review

---------

Co-authored-by: Nick Fitzgerald <[email protected]>
eduardomourar pushed a commit to eduardomourar/wasmtime that referenced this pull request Sep 13, 2023
This change is a small refactoring to some of the MacroAssembler functions to
use `Reg` instead of `RegImm` where appropriate (e.g. when the operand is a
destination).

@elliottt pointed this out while working on bytecodealliance#6982

This change also changes the signature of `float_abs` and `float_neg`, which can
be simplified to take a single register.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fuzzing Issues related to our fuzzing infrastructure winch Winch issues or pull requests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants