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 Wasm target features that are in phase 4 and 5 #117457

Merged
merged 1 commit into from Apr 21, 2024

Conversation

daxpedda
Copy link
Contributor

@daxpedda daxpedda commented Oct 31, 2023

This stabilizes the Wasm target features that are known to be working and in phase 4 and 5.

Feature stabilized:

Features not stabilized:

See #117457 (comment) for more context.

Documentation: rust-lang/reference#1420
Tracking issue: #44839

@rustbot
Copy link
Collaborator

rustbot commented Oct 31, 2023

r? @cjgillot

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

@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 31, 2023
@daxpedda
Copy link
Contributor Author

Cc @alexcrichton.

@alexcrichton
Copy link
Member

AFAIK a stabilization policy is not documented anywhere for WebAssembly target features, but in my opinion this is a good PR to land.

As a bit of background on this in case anyone's curious: WebAssembly target features in LLVM relate to WebAssembly proposals and are frequently named after those proposals. WebAssembly proposals go through a phased design process similar to JS and once a feature reaches "phase 4" it's effectively done and ready for everyone to use. That's when browsers start shipping the feature ungated for example. There is no "standard" for what to call these proposals beyond "toolchain conventions" which, at this time, is "whatever LLVM called it". In that sense the choice of feature name here matches what one would use on the command line for C/C++ when compiling WebAssembly.

This particular proposal, along with others, as pointed out, is "stage 5" which means it's long since done and overdue for stabilization on our end.

@cjgillot
Copy link
Contributor

@alexcrichton do you mind taking the review?

Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

Certainly! I fear I may not have bors permissions any more though, but I can offer a github approval nonetheless

@daxpedda daxpedda changed the title Stabilize nontrapping-fptoint Stabilize Wasm target features that are in phase 4 and 5 Oct 31, 2023
@daxpedda
Copy link
Contributor Author

I decided to add the remaining features into the same PR as they don't require any further work in the Rust compiler.

@traviscross traviscross added I-lang-nominated The issue / PR has been nominated for discussion during a lang team meeting. T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Nov 1, 2023
@nikomatsakis
Copy link
Contributor

nikomatsakis commented Nov 1, 2023

@alexcrichton can you clarify the impact of stabilizing these items? What impact does it have on what Rust code people are allowed to write? Do these affect the quality of codegen only? Does that imply that a broader set of Rust code will compile on the wasm target? How do people specify which target features they want to be using?

I'm generally in favor of doing this, but it would be good to understand what exactly it means.

@nikomatsakis
Copy link
Contributor

In particular I am also trying to figure out if this is T-compiler, T-lang, or both in terms of who needs to commit to stabilization.

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Nov 1, 2023

@rfcbot fcp merge

I am going to assume that this is both a lang and compiler RFC, and hence I move to merge, based on what @alexcrichton had to say here. Here is my understanding of what we are stabilizing:

  • The ability to specify a certain set of webassembly features. These features have no direct impact on the compiler but are passed through to the backend (LLVM, at present) and enable it to make use of wasm features. They may affected quality of codegen and/or perhaps the ability for some kinds of code to be compiled, this is not entirely clear to me and I would appreciate clarification.
  • The names of these features comes from webassembly spec processes and they are all in "stage 4", which means effectively done and also available in browsers without feature gates etc. So no reason to think the names or meaning will change going forward.

The primary concern I can imagine, if the above is correct, is that the compiler doesn't handle the flags correctly or that this may allow some Rust code to reach WASM that "ought not to" (not sure how that latter part would happen though unless there was a bug somewhere else in rustc that enabled it, so i.e. the first problem).

@rfcbot
Copy link

rfcbot commented Nov 1, 2023

Team member @nikomatsakis 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 1, 2023
@tmandry
Copy link
Member

tmandry commented Nov 1, 2023

I assume this would include the ability to specify those features in both compiler flags and #[target_feature], though I can't remember if we stabilized the latter for WASM or not.

Also @nikomatsakis it sounds like the exact names being used actually come from LLVM. I don't see any reason to differ on those though.

In any case this all sounds fine to me, so

@rfcbot reviewed

@alexcrichton
Copy link
Member

Sure yeah I can try to document and clarify a few things here. I'll reiterate
some bits I mentioned
above
as well.

For background here the WebAssembly Community Group (CG) is the main technical
governing body for WebAssembly at this time. There is a process in place for
adding new features to WebAssembly and it is documented in this
repository
. As proposals go through
their stages it requires adding support to toolchains and languages which often
means implementing this in LLVM. For Rust that means that there's a question of
when to stabilize and how to support new features of WebAssembly.

Proposals which have reached at least stage 4 are generally considered done. At
that point browsers start shipping features ungated for example. Historically
this at least been my personal loose threshold of when to stabilize features for
Rust. Any feature in phase 3 or lower would, in my opinion, not be a candidate
for stabilization in Rust.

All of the features stabilized here are phase 4 and beyond (many at phase 5).
This means that the threshold for stability in WebAssembly itself is achieved
and the question now turns to Rust to how to expose these features. Features in
WebAssembly are often new instructions but sometimes manifest as new syntactic
forms in a WebAssembly binary. Features also do not work like native platforms
because for a WebAssembly binary to be considered valid to execute it must be
valid in its entirety. In contrast a native binary can contain code that the
current processor doesn't support so long as it doesn't execute it. In that
sense native runtime feature detection is not possible in WebAssembly unlike how
it is for native platforms.

Ok so how does any of this affect Rust. Rust supports compilation to WebAssembly
through LLVM, and in general needs to express the ability to customize the
output binary to handle the presence or non-presence of these features. For
example for performance-related features users may wish to have builds both with
and without the feature. This is achieved right now via a few means:

  1. Via -Ctarget-feature=+foo. AFAIK this isn't gated and is passed through
    straight to LLVM, so this works today and does not require stabilization.
    This works well for WebAssembly since if a feature is enabled for one
    function it may as well be enabled for the whole binary since if one function
    gets it all others will be able to have access to it as well.

  2. Via #[target_feature(enable = "foo")]. This is not stable today and is what
    would be stabilized here. This is where the story is a little murky for
    WebAssembly since unlike native platforms not much is gained from enabling a
    feature per-function rather than at the whole module level. That being said
    this is required for correctness in the case of simd128 for example (e.g.
    LLVM can't codegen intrinsics unless that feature is enabled) and can still
    be useful in some niche situations.

  3. Testing via #[cfg(target_feature = "foo")] currently doesn't work today
    since only when these attributes are stable are the cfg directives defined.
    This can be useful in general but isn't too useful for many of the features
    stabilized here.

So given all that, this PR largely boils down to enabling #[cfg(target_feature = "foo")] and #[target_feature(enable = "foo")] on stable. These aren't the
most useful things in the world but can be useful in some niche situations. What
follows is a more detailed description of what all these features are.

"bulk-memory"

The bulk memory
proposal

for WebAssembly can more-or-less be thought of as adding a memcpy instruction
to WebAssembly (or memmove, I forget which allows overlap). There's other bits
and bobs though for "data segments" and constructs in WebAssembly which are not
relevant to Rust-the-language directly. Why users might want this proposal:

  • The memory.copy WebAssembly is much faster than memcpy-written-in-wasm for
    large data copies.
  • Multithreaded WebAssembly uses features added in this proposal.

"extended-const"

The extended const proposal
enables new forms of constant expressions in WebAssembly. This has no effect on
Rust itself and has nothing to do with Rust const. This is not useful nor
exposed to Rust code at all through LLVM and is only used, as far as I know, for
dynamic linking. The dynamic linking story in Rust for WebAssembly is not
well-defined and has not been fleshed out. Nevertheless though it's stable in
WebAssembly and implemented in LLVM and will be integral for any future
experimentation with dynamic linking for example. This may also be useful for
other more niche situations of users trying to craft particular shapes of wasm
modules.

"mutable-globals"

The mutable global proposal is
largely centered around JS integration of WebAssembly. Honestly I'm not really
sure why this is exposed through LLVM. This has no effect on generated code as
far as I know and is basically only required if you want to experiment with
threads. Any threads-using target for WebAssembly is required to enable this
proposal (e.g. it's a hard requirement check in the LLD linker).

"nontrapping-fptoint"

The non-trapping float-to-int conversions
proposal

affects how f32 as i32 is translated in Rust for example. A comparison can be
seen on godbolt.org of how the generated code
differs. For any application bottlenecked on performance of this operation this
would be critical in getting the best performance.

"sign-ext"

The sign extension operations
proposal

added a few instructions related to sign extending 8/16-bit values to 32/64-bit
values to the base instruction set. A comparison here can also be seen on
godbolt.org
. Note that this feature is enabled
by default in the WebAssembly targets today so it's actually somewhat difficult
to disable.

@ehuss
Copy link
Contributor

ehuss commented Nov 1, 2023

I assume this would include the ability to specify those features in both compiler flags and #[target_feature]

-C target-feature generally doesn't restrict which options you can use. This is only stabilizing the #[target_feature] attribute values. Previous to this PR, only simd128 was stable in the attribute.

@petrochenkov
Copy link
Contributor

I'm going to rubber stamp this if wasm experts are on board with this stabilization.
@rfcbot reviewed

@traviscross traviscross removed the I-lang-nominated The issue / PR has been nominated for discussion during a lang team meeting. label Nov 8, 2023
@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.

@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.

@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Apr 9, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 9, 2024
…=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 9, 2024

⌛ Testing commit 1d46f00 with merge 8993b30...

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Apr 9, 2024

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Apr 9, 2024
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Apr 11, 2024
@rustbot
Copy link
Collaborator

rustbot commented Apr 16, 2024

Some changes occurred in tests/ui/check-cfg

cc @Urgau

@daxpedda
Copy link
Contributor Author

@wesleywiser apologies for the delay.

@bors
Copy link
Contributor

bors commented Apr 16, 2024

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

@bors
Copy link
Contributor

bors commented Apr 18, 2024

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

@cjgillot
Copy link
Contributor

@bors r=wesleywiser

@cjgillot cjgillot assigned wesleywiser and unassigned cjgillot Apr 20, 2024
@bors
Copy link
Contributor

bors commented Apr 20, 2024

📌 Commit 6a52fee has been approved by wesleywiser

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 20, 2024
@bors
Copy link
Contributor

bors commented Apr 21, 2024

⌛ Testing commit 6a52fee with merge b9be3c4...

@bors
Copy link
Contributor

bors commented Apr 21, 2024

☀️ Test successful - checks-actions
Approved by: wesleywiser
Pushing b9be3c4 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 21, 2024
@bors bors merged commit b9be3c4 into rust-lang:master Apr 21, 2024
13 checks passed
@rustbot rustbot added this to the 1.79.0 milestone Apr 21, 2024
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (b9be3c4): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
2.8% [1.1%, 5.3%] 3
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 2.8% [1.1%, 5.3%] 3

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
3.5% [3.5%, 3.5%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 672.201s -> 671.949s (-0.04%)
Artifact size: 315.29 MiB -> 315.25 MiB (-0.01%)

github-actions bot pushed a commit to rust-lang/miri that referenced this pull request Apr 23, 2024
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` #73755.
- [Reference Types](https://github.com/WebAssembly/reference-types): no point stabilizing without #103516.
- [Threads](https://github.com/webassembly/threads): requires rebuilding `std` #77839.
- [Relaxed SIMD](https://github.com/WebAssembly/relaxed-simd): separate PR #117468.
- [Multi Memory](https://github.com/WebAssembly/multi-memory): not implemented.

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

Documentation: rust-lang/reference#1420
Tracking issue: rust-lang/rust#44839
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. finished-final-comment-period The final comment period is finished for this PR / Issue. merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet