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.

Sign up for GitHub

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

Jump to bottom

Stabilize Wasm relaxed SIMD #117468

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Stabilize Wasm relaxed SIMD #117468

wants to merge 1 commit into from
+1 −1

Conversation

daxpedda
Copy link
Contributor

@daxpedda daxpedda commented Oct 31, 2023 •
edited

This PR stabilizes Wasm relaxed SIMD which has already reached phase 4.

Tracking issue: #111196
Implementation PR: rust-lang/stdarch#1393
Documentation: rust-lang/reference#1421
Stdarch: rust-lang/stdarch#1494

Closes #111196.

@rustbot
Copy link
Collaborator

rustbot commented Oct 31, 2023

r? @wesleywiser

(rustbot has picked a reviewer for you, use r? to override)

@rustbot
Copy link
Collaborator

rustbot commented Oct 31, 2023

⚠️ Warning ⚠️

  • These commits modify submodules.

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Oct 31, 2023
@daxpedda
Copy link
Contributor Author

Cc @alexcrichton.

@daxpedda daxpedda mentioned this pull request Oct 31, 2023
Draft
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@daxpedda
Copy link
Contributor Author

daxpedda commented Oct 31, 2023 •
edited

This won't currently compile until rust-lang/stdarch#1494 is merged and updated, so for now this is waiting for the FCP first.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
This was referenced Oct 31, 2023
Merged
Draft
@wesleywiser wesleywiser added the S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. label Nov 1, 2023
@alexcrichton
Copy link
Member

Personally I'd say this is good to go (modulo CI of course), and it's something I forgot to do after the last CG meeting, thanks @daxpedda!

For others this PR has relevant discussion namely some of the background laid you in this comment. The relaxed-simd proposal is a bit different where there's actual instructions to expose with new intrinsics, but this is following in the footsteps of the simd128 proposal and its intrinsics so AFAIK there's no new precedents set here or anything like that, mostly just following preexisting processes.

@daxpedda
Copy link
Contributor Author

daxpedda commented Nov 2, 2023 •
edited

@wesleywiser AFAIU this should probably not be marked as blocked.

It's true that merging is blocked on rust-lang/stdarch#1494, but the FCP is required for rust-lang/stdarch#1494 to be merged in the first place.
It could be argued that this is blocked on #117372, which in turn is blocked by tkaitchuck/aHash#183, but that should probably not block an FCP.

So I would like to request an FCP for stabilizing relaxed SIMD!
Let me know if I misunderstood something.

@Amanieu Amanieu added T-lang Relevant to the language team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Nov 2, 2023
@Amanieu
Copy link
Member

Amanieu commented Nov 2, 2023

This cover the relaxed-simd target feature (lang team) and the related intrinsics in stdarch (libs-api team).

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented Nov 2, 2023 •
edited by scottmcm

Team member @Amanieu has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Nov 2, 2023
@RalfJung
Copy link
Member

RalfJung commented Nov 2, 2023

Repeating what I posed on the tracking issue:

I am confused by the spec here. Quoting from https://github.com/WebAssembly/relaxed-simd/blob/main/proposals/relaxed-simd/Overview.md:

The same instruction at (1) and (2), when given the same inputs, can return two different results.

Okay, so that's normal non-determinism then. Wasm already has that for NaNs when there are non-canonical inputs, which is strangely not acknowledged here (has that changed?), but sure, makes sense.

Some operators are host-dependent, because the set of possible results may depend on properties of the host environment (such as hardware). Technically, each such operator produces a fixed-size list of sets of allowed values. For each execution of the operator in the same environment, only values from the set at the same position in the list are returned, i.e., each environment globally chooses a fixed projection for each operator.

This describes something else! IIUC, it says that the operation, when executed twice in the same environment, must produce the same result. In other words, the non-determinism is fixed once at program startup, and the example givejn just above actually cannot happen within a single program/module execution. This is closer to what we usually call implementation-specific behavior than to full non-determinism.

Can someone explain this seeming contradiction?

@alexcrichton
Copy link
Member

To the best of my knowledge I believe that the contradiction is explained by the fact that the proposal has a long history and changed at one point. I believe your first quoted sentence is the original semantics when the proposal was first drafted and the second quote you listed is the desired semantics.

For example this was the commit that introduced the first quote and it refers to an fpenv idea which was later removed. While I can't speak with complete certainty I'm pretty confident you've discovered a mistake in the overview. This could be confirmed with the rendered specification but that is a broken link at this time. I'm not great at reading the raw spec myself.

@RalfJung
Copy link
Member

RalfJung commented Nov 4, 2023 •
edited

Okay, I've opened WebAssembly/relaxed-simd#153 to hopefully get clarification from the authors.

For Rust, we should probably conservatively treat these as being non-deterministic at each operation. I don't think that should cause any issues? These are just LLVM intrinsics implemented via certain wasm instructions, right?

@daxpedda
Copy link
Contributor Author

daxpedda commented Nov 4, 2023

I don't think that should cause any issues?

No, we do nothing with the output (the part that might not be deterministic).

These are just LLVM intrinsics implemented via certain wasm instructions that, right?

Indeed.

@bors
Copy link
Contributor

bors commented Nov 15, 2023

☔ The latest upstream changes (presumably #117915) made this pull request unmergeable. Please resolve the merge conflicts.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors
Copy link
Contributor

bors commented Dec 15, 2023

☔ The latest upstream changes (presumably #118957) made this pull request unmergeable. Please resolve the merge conflicts.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors
Copy link
Contributor

bors commented Feb 5, 2024

☔ The latest upstream changes (presumably #117372) made this pull request unmergeable. Please resolve the merge conflicts.

@daxpedda daxpedda force-pushed the wasm-relaxed-simd branch 2 times, most recently from 3413080 to b2e6b1b Compare February 6, 2024 10:33
@rustbot
Copy link
Collaborator

rustbot commented Feb 6, 2024

Some changes occurred in src/tools/cargo

cc @ehuss

@daxpedda
Copy link
Contributor Author

daxpedda commented Feb 6, 2024

Apologies, my last commit included some wrong submodule updates, which is fixed now.

Now that #117372 is merged, this is unblocked and CI should pass.

@daxpedda
Copy link
Contributor Author

@wesleywiser would you mind removing the "S-blocked" label?
This PR previously relied on #117372, which is now merged.

@daxpedda daxpedda force-pushed the wasm-relaxed-simd branch 2 times, most recently from d2f3e34 to 4bd0f3d Compare February 16, 2024 13:08
@Amanieu
Copy link
Member

Amanieu commented Feb 16, 2024

ping @T-lang for checkboxes

This only needs lang review for adding a new target feature.

@Dylan-DPC Dylan-DPC removed the S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. label Feb 16, 2024
@bors
Copy link
Contributor

bors commented Feb 19, 2024

☔ The latest upstream changes (presumably #121185) made this pull request unmergeable. Please resolve the merge conflicts.

@bors
Copy link
Contributor

bors commented Feb 24, 2024

☔ The latest upstream changes (presumably #121549) made this pull request unmergeable. Please resolve the merge conflicts.

@bors
Copy link
Contributor

bors commented Mar 26, 2024

☔ The latest upstream changes (presumably #123065) made this pull request unmergeable. Please resolve the merge conflicts.

@daxpedda
Copy link
Contributor Author

I removed the dependency on rust-lang/stdarch#1494, AFAIU this is usually done in a follow-up.

bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 9, 2024
@bors
Auto merge of rust-lang#117457 - daxpedda:wasm-nontrapping-fptoint, r…
8993b30 
…=wesleywiser

Stabilize Wasm target features that are in phase 4 and 5

This stabilizes the Wasm target features that are known to be working and in [phase 4 and 5](https://github.com/WebAssembly/proposals/tree/04fa8c810e1dc99ab399e41052a6e427ee988180).

Feature stabilized:
- [Non-trapping float-to-int conversions](https://github.com/WebAssembly/nontrapping-float-to-int-conversions)
- [Import/Export of Mutable Globals](https://github.com/WebAssembly/mutable-global)
- [Sign-extension operators](https://github.com/WebAssembly/sign-extension-ops)
- [Bulk memory operations](https://github.com/WebAssembly/bulk-memory-operations)
- [Extended Constant Expressions](https://github.com/WebAssembly/extended-const)

Features not stabilized:
- [Multi-value](https://github.com/WebAssembly/multi-value): requires rebuilding `std` rust-lang#73755.
- [Reference Types](https://github.com/WebAssembly/reference-types): no point stabilizing without rust-lang#103516.
- [Threads](https://github.com/webassembly/threads): requires rebuilding `std` rust-lang#77839.
- [Relaxed SIMD](https://github.com/WebAssembly/relaxed-simd): separate PR rust-lang#117468.
- [Multi Memory](https://github.com/WebAssembly/multi-memory): not implemented.

See rust-lang#117457 (comment) for more context.

Documentation: rust-lang/reference#1420
Tracking issue: rust-lang#44839
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 21, 2024
@bors
Auto merge of rust-lang#117457 - daxpedda:wasm-nontrapping-fptoint, r…
b9be3c4 
…=wesleywiser

Stabilize Wasm target features that are in phase 4 and 5

This stabilizes the Wasm target features that are known to be working and in [phase 4 and 5](https://github.com/WebAssembly/proposals/tree/04fa8c810e1dc99ab399e41052a6e427ee988180).

Feature stabilized:
- [Non-trapping float-to-int conversions](https://github.com/WebAssembly/nontrapping-float-to-int-conversions)
- [Import/Export of Mutable Globals](https://github.com/WebAssembly/mutable-global)
- [Sign-extension operators](https://github.com/WebAssembly/sign-extension-ops)
- [Bulk memory operations](https://github.com/WebAssembly/bulk-memory-operations)
- [Extended Constant Expressions](https://github.com/WebAssembly/extended-const)

Features not stabilized:
- [Multi-value](https://github.com/WebAssembly/multi-value): requires rebuilding `std` rust-lang#73755.
- [Reference Types](https://github.com/WebAssembly/reference-types): no point stabilizing without rust-lang#103516.
- [Threads](https://github.com/webassembly/threads): requires rebuilding `std` rust-lang#77839.
- [Relaxed SIMD](https://github.com/WebAssembly/relaxed-simd): separate PR rust-lang#117468.
- [Multi Memory](https://github.com/WebAssembly/multi-memory): not implemented.

See rust-lang#117457 (comment) for more context.

Documentation: rust-lang/reference#1420
Tracking issue: rust-lang#44839
@bors
Copy link
Contributor

bors commented Apr 21, 2024

☔ The latest upstream changes (presumably #117457) made this pull request unmergeable. Please resolve the merge conflicts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@wesleywiser wesleywiser

Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-lang Relevant to the language team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Tracking Issue for WebAssembly relaxed SIMD intrinsics
10 participants
@daxpedda @rustbot @rust-log-analyzer @alexcrichton @Amanieu @rfcbot @RalfJung @bors @wesleywiser @Dylan-DPC