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

Stabilize WebAssembly multivalue, reference-types, and tail-call target features #131080

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

Conversation

alexcrichton
Copy link
Member

@alexcrichton alexcrichton commented Oct 1, 2024

For the multivalue and reference-types features this commit is
similar to #117457 in that it's stabilizing target features specific to
WebAssembly targets. The previous PR left out these two features because
they weren't expected to change much about compiled code so it was
unclear what the rationale was. It has since been discovered
that reference-types can be useful as it changes the binary format of
the call_indirect instruction. Additionally on Zulip there's
a use case of detecting these features at compile time and generating a
compile error to better warn users about features not supported on
engines.

This PR then additionally adds the tail-call feature which corresponds
to the tail-call proposal to WebAssembly. This feature advanced to
"phase 4" in the WebAssembly CG awhile back and has been supported in
LLVM for quite some time now. Engines are finishing up implementations
or have already shipped implementations, so while this is a bit of a
late addition to Rust itself it reflects the current status of
WebAssembly's state of the feature.

A test has been added here not only for these features but other
WebAssembly features as well to showcase that they're usable without
feature gates in stable Rust.

@rustbot
Copy link
Collaborator

rustbot commented Oct 1, 2024

r? @petrochenkov

rustbot has assigned @petrochenkov.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@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. labels Oct 1, 2024
Copy link
Member

@jieyouxu jieyouxu left a comment

Choose a reason for hiding this comment

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

The changes LGTM. As far as I understand it, this stabilizes multivalue and reference-types target features (aka wasm proposals) which are already enabled-by-default through the llvm 19 update.

But let's also have someone on T-compiler double-check.

EDIT: this definitely needs relnotes

@jieyouxu jieyouxu self-assigned this Oct 1, 2024
@alexcrichton
Copy link
Member Author

In case anyone's curious, this is a longer description of what stabilization of these features mean and for multivalue and reference-types there's some more info in this post but the general tl;dr; is that multivalue and reference-types don't affect generated code too too much except for reference-types currently changing the encoding of indirect call instructions. The main motivation for this is to enable users to test for this in Wasm code and emit early-errors if the engine the code is intended to run in doesn't support either proposal.

@ehuss
Copy link
Contributor

ehuss commented Oct 1, 2024

Whenever you get a chance, can you also post a PR to update the docs at https://github.com/rust-lang/reference/blob/9c21beeaa59566d87191392548c421cc7baa941a/src/attributes/codegen.md?plain=1#L276-L284?

@alexcrichton
Copy link
Member Author

certainly!

@CryZe
Copy link
Contributor

CryZe commented Oct 1, 2024

Would it make sense to add Tail Call as well? I recently added wasm-bindgen support, it's in phase 5 and V8, Spidermonkey and wasmtime ship it by default now, with Safari following very soon.

@alexcrichton alexcrichton force-pushed the stabilize-more-wasm-target-features branch from d42160b to 1ee87c9 Compare October 1, 2024 22:54
@alexcrichton alexcrichton changed the title Stabilize WebAssembly multivalue and reference-types target features Stabilize WebAssembly multivalue, reference-types, and tail-call target features Oct 1, 2024
@alexcrichton alexcrichton changed the title Stabilize WebAssembly multivalue, reference-types, and tail-call target features Stabilize WebAssembly multivalue, reference-types, and tail-call target features Oct 1, 2024
@alexcrichton
Copy link
Member Author

Excellent point! I've added that in as well now. I've additionally expanded the commit message, PR description, and PR title to include tail-call as well.

I also realize though that this would be an insta-stable addition. If folks are uncertain about that I'm also happy to mark the feature as unstable for awhile. I'm relatively sure that the LLVM implementation is pretty mature though and @CryZe is right in that most engines are shipping this now, so the only part that might change is the feature name really and tail-call matches both the proposal itself and LLVM. In that sense most of the work for shipping this feature wasn't on Rust's part, all Rust needs to do is name it, so that's the part to insta-stabilize here.

@rust-log-analyzer

This comment has been minimized.

@alexcrichton alexcrichton force-pushed the stabilize-more-wasm-target-features branch from 1ee87c9 to 6209f96 Compare October 2, 2024 03:20
@rustbot
Copy link
Collaborator

rustbot commented Oct 2, 2024

Some changes occurred in tests/ui/check-cfg

cc @Urgau

@alexcrichton alexcrichton force-pushed the stabilize-more-wasm-target-features branch from 6209f96 to 1047029 Compare October 2, 2024 03:23
@jieyouxu jieyouxu removed their assignment Oct 4, 2024
@petrochenkov
Copy link
Contributor

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented Oct 9, 2024

Team member @petrochenkov 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!

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 Oct 9, 2024
@petrochenkov petrochenkov added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 9, 2024
@bors
Copy link
Contributor

bors commented Oct 10, 2024

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

@alexcrichton alexcrichton force-pushed the stabilize-more-wasm-target-features branch from 1047029 to ee1a547 Compare October 11, 2024 01:40
@workingjubilee workingjubilee added the O-wasm Target: WASM (WebAssembly), http://webassembly.org/ label Oct 13, 2024
@alexcrichton
Copy link
Member Author

Wanted to bump this again in case it fell out of folks' inboxes

@bors
Copy link
Contributor

bors commented Oct 27, 2024

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

@alexcrichton alexcrichton force-pushed the stabilize-more-wasm-target-features branch from ee1a547 to b072776 Compare October 28, 2024 15:16
@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Oct 31, 2024
@rfcbot
Copy link

rfcbot commented Oct 31, 2024

🔔 This is now entering its final comment period, as per the review above. 🔔

@bors
Copy link
Contributor

bors commented Nov 6, 2024

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

…` target features

For the `multivalue` and `reference-types` features this commit is
similar to rust-lang#117457 in that it's stabilizing target features specific to
WebAssembly targets. The previous PR left out these two features because
they weren't expected to change much about compiled code so it was
unclear what the rationale was. It has [since been discovered][blog]
that `reference-types` can be useful as it changes the binary format of
the `call_indirect` instruction. Additionally [on Zulip][zulip] there's
a use case of detecting these features at compile time and generating a
compile error to better warn users about features not supported on
engines.

This PR then additionally adds the `tail-call` feature which corresponds
to the [tail-call] proposal to WebAssembly. This feature advanced to
"phase 4" in the WebAssembly CG awhile back and has been supported in
LLVM for quite some time now. Engines are finishing up implementations
or have already shipped implementations, so while this is a bit of a
late addition to Rust itself it reflects the current status of
WebAssembly's state of the feature.

A test has been added here not only for these features but other
WebAssembly features as well to showcase that they're usable without
feature gates in stable Rust.

[blog]: https://blog.rust-lang.org/2024/09/24/webassembly-targets-change-in-default-target-features.html
[zulip]: https://rust-lang.zulipchat.com/#narrow/stream/122651-general/topic/wasm32.20reference-types.20.2F.20multivalue.20in.201.2E82-beta.20not.20enabled/near/473893987
[tail-call]: https://github.com/webassembly/tail-call
@alexcrichton alexcrichton force-pushed the stabilize-more-wasm-target-features branch from b072776 to 3d7bf29 Compare November 6, 2024 18:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. O-wasm Target: WASM (WebAssembly), http://webassembly.org/ S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants