-
-
Notifications
You must be signed in to change notification settings - Fork 14.4k
Mark const SIMD intrinsics as indirectly stable #149648
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
base: main
Are you sure you want to change the base?
Conversation
|
cc @rust-lang/wg-const-eval Some changes occurred to the intrinsics. Make sure the CTFE / Miri interpreter cc @rust-lang/miri, @RalfJung, @oli-obk, @lcnr Some changes occurred to the platform-builtins intrinsics. Make sure the cc @antoyo, @GuillaumeGomez, @bjorn3, @calebzulawski, @programmerjake |
|
So you plan to soon const-stabilize functions using these intrinsics? |
|
I won't say soon (it would require t-libs-api approval), but yes the plan is eventually to stabilize them |
|
Usually we do the t-lang FCP for const intrinsics as part of that stabilization, not preemptively. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For some of these I am not sure they are ready for stabilization.
Also, please give an overview of the test coverage we have for these being invoked from const-eval.
|
r? @RalfJung |
|
@rustbot author I would propose to remove all controversial cases, then we can proceed to FCP. |
|
Reminder, once the PR becomes ready for a review, use |
|
Apologize for the delay. @RalfJung do the above-mentioned caveats also apply to |
|
Yeah, they do. |
9a84c00 to
a87092b
Compare
This comment has been minimized.
This comment has been minimized.
|
@rustbot ready I have removed the attributes from |
|
I have nominated this for t-lang discussion. It'd be good to update the PR description to give the required context to team members visiting this during triage: which new capabilities are exposed to const code here, and what does the test coverage look like? |
|
Summary: this makes it possible for functions that indirectly call these SIMD intrinsics to be called from stable const-eval code. The lang decision when things are marked as @rfcbot merge lang |
|
Team member @joshtriplett 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. |
|
To reiterate what @RalfJung said, it'd still be good to see this:
|
|
Ah sorry, I guess I should have waited for someone (specifically, @sayantn :) to do that before nominating. I was hoping it would be taken care of quickly enough to happen before t-lang gets around to look at the PR. |
|
@RalfJung sorry for the delay. I have updated the description, could you check if it is ok? |
a87092b to
5e7ef31
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
Instead of listing which intrinsics are not affected, could you list the ones that are? Remember, the reader has likely ~never looked at these intrinsics before.
That makes me wonder why we should go through the trouble of approving these attributes.^^
Given that the stdarch tests check that the right instructions get emitted even with the "generic" version, how could they possibly not behave identically? (Also, calling this "generic" is confusing since Rust generics are something very different. I'd call it "portable" or "target-independent".) |
I might tweak this ask to "in addition to" rather than "instead of". It is valuable seeing what's not here, but it'd be good to say explicitly what is.
If you could speak more to this in particular, that would be helpful. I.e., I'd like to know more about the motivation and what specifically we're unblocking by accepting this. What do we want to do that will be possible after we approve this? An example would help. |
recently most SIMD intrinsics were made available in const contexts in #147521. This PR makes the indirectly stable to use in const contexts.
cc @RalfJung @rust-lang/lang @rust-lang/wg-const-eval
This exposes most SIMD intrinsics to be used in stable
constfunctions, except for the followingsimd_f{min,max}andsimd_reduce_{min,max}: See @RalfJung's commentsimd_reduce_{add,mul}_unordered: These are not evenconstyet, due to concerns about possible unsoundness due to non-determinism in float operationssimd_expose_provenance: Rust doesn't allow ptr->int casts in const code yetsimd_relaxed_fma: Same nondet concerns, also the scalar versionfmuladdis not even indirectly stable yetsimd_f{sin,cos,exp,exp2,log10,log2,log}: Not const yetThese all have const tests in
tests/ui/simd(added in #147521), and some of these are currently also being tested in x86 stdarch (we have already converted some intrinsics to be available inconst, and the stdarch testsuite automatically tests the const code).I want to emphasize that I am not aiming to stabilize any of the const stdarch intrinsics in near future. The reason being that it is difficult to fix perf regressions in generic SIMD code (normally stdarch uses LLVM intrinsics, but we are aiming to convert as many as possible to generic SIMD. Unfortunately that sometimes leads to perf regressions, which might force us to go back to LLVM intrinsics). There is
const_eval_select, but it is still a pain to use (considering that we have to ensure that the 2 alternatives behave identically to be sound). A recent such problem surfaced in #150560, and fixed in rust-lang/stdarch#1985.