-
Notifications
You must be signed in to change notification settings - Fork 14k
Make SIMD intrinsics available in const-contexts
#147521
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
|
Some changes occurred to the platform-builtins intrinsics. Make sure the cc @antoyo, @GuillaumeGomez, @bjorn3, @calebzulawski, @programmerjake The Miri subtree was changed cc @rust-lang/miri Some changes occurred to the intrinsics. Make sure the CTFE / Miri interpreter |
|
|
|
Cc @Amanieu |
| let r: f32 = simd_reduce_mul_unordered(x); | ||
| assert_eq!(r, -24_f32); | ||
| let x = f32x4::from_array([1., -2., 3., 4.]); | ||
| let r: f32 = simd_reduce_add_ordered(x, 0.); |
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.
I know this wasn't there originally, but maybe we should add a test that requires being added in the proper order to return the right result? something like 1.0e20f32 + 1.0 - 1.0e20 - 1.0 == -1.0
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.
that seems like a nice idea, but I really don't want to add new tests in this PR - this is pretty big by itself anyway
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.
other than that idea for a new test, lgtm
8c71ce0 to
48324c2
Compare
This comment has been minimized.
This comment has been minimized.
|
☔ The latest upstream changes (presumably #148507) made this pull request unmergeable. Please resolve the merge conflicts. |
48324c2 to
76ddb6f
Compare
This comment has been minimized.
This comment has been minimized.
76ddb6f to
d1b2151
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
d1b2151 to
8715316
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. |
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.
Haven't looked through all the tests, will wait until we figure out whether we can just overwrite assert_eq!, from a quick glance they look fine (they were also reviewed previously in #146568 by @tgross35).
| #[rustc_intrinsic] | ||
| #[rustc_nounwind] | ||
| pub unsafe fn simd_add<T>(x: T, y: T) -> T; | ||
| pub const unsafe fn simd_add<T>(x: T, y: T) -> T; |
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.
I am unsure of the process here: does this need a compiler FCP to confirm that, yes, we do want t-libs to be able to start depending on these intrinsics being const?
#146568 was reviewed by Ralf, so I guess that's close to as good as it gets in terms of "wg-const-eval is fine with this".
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.
I don't know, @Amanieu do we need to do some kind of libs fcp??
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.
The team in charge of intrinsics is t-lang. They will have to be involved when stabilizing any of this, i.e., when adding rustc_intrinsic_const_stable_indirect to an intrinsic.
For the unstable part, notifying @rust-lang/wg-const-eval is enough. From my side this is fine: the only part that needs closer scrutiny is the fact that these are float operations, but we made them behave just like their scalar equivalents which seems right to me.
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.
Ah so we can continue on this 🎉
Trevor left two comments but didn't indicate whether they even looked at the entire rest of the PR. |
8715316 to
e48c21b
Compare
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.
I've reviewed the tests now, generally happy with them.
Trevor left two comments but didn't indicate whether they even looked at the entire rest of the PR.
Yeah, I should've said "skimmed".
Co-authored-by: Mads Marquart <[email protected]>
|
Thanks for the answers, feel free to r=me when you've rebased, and when we've figured out if we've followed the right process. |
successor to #146568, this PR actually makes the SIMD intrinsics
const, and modifies the tests to test the const-eval implementationsr? @tgross35 ig (although feel free to reassign, this is not anything targeted really)