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

pub use std::simd::StdFloat; #93146

Merged
merged 24 commits into from
Feb 3, 2022
Merged

Conversation

workingjubilee
Copy link
Member

@workingjubilee workingjubilee commented Jan 21, 2022

Syncs portable-simd up to commit rust-lang/portable-simd@03f6fbb,
Diff: rust-lang/portable-simd@533f0fc...03f6fbb

This sync requires a little bit more legwork because it also introduces a trait into std::simd, so that it is no longer simply a reexport of core::simd. Out of simple-minded consistency and to allow more options, I replicated the pattern for the way core::simd is integrated in the first place, however this is not necessary if it doesn't acquire any interdependencies inside std: it could be a simple crate reexport. I just don't know yet if that will happen or not.

To summarize other misc changes:

  • Shifts no longer panic, now wrap on too-large shifts (like Simd integers usually do!)
  • mask16x32 will now be many i16s, not many i32s... 🙃
  • #[must_use] is spread around generously
  • Adjusts division, float min/max, and Mask::{from,to}_array internally to be faster
  • Adds the much-requested Simd::cast::<U> function (equivalent to simd.to_array().map(|lane| lane as U))

workingjubilee and others added 15 commits December 8, 2021 18:08
For all other operators, we use wrapping logic where applicable.
This is another case it applies. Per rust-lang#91237, we may
wish to specify this as the natural behavior of `simd_{shl,shr}`.
This should perform a SIMD check for whether or not we can div/rem,
so that we can panic several times faster!
This approaches reducing macro nesting in a slightly different way.
Instead of just flattening details, make one macro apply another.
This allows specifying all details up-front in the first macro
invocation, making it easier to audit and refactor in the future.
Refactor ops.rs with wrapping shifts

This approaches reducing macro nesting in a slightly different way. Instead of just flattening details, make one macro apply another. This allows specifying all details up-front in the first macro invocation, making it easier to audit and refactor in the future.

This refactor also has some functional changes. Only one is a true behavior change, however:
- The visible one is that SIMD shifts are now wrapping, not panicking on overflow
- `core::simd` now has a lot more instances of `#[must_use]`, which merely lints
- div/rem now perform a SIMD check but remain as before, which should improve performance but be invisible
While consulting with Simulacrum on how to make available the float
functions that currently require runtime support for `Simd<f32, N>` and
`Simd<f64, N>`, we realized breaking coherence with the classic approach
of lang items was, since `{core,std}::simd::Simd` is a `ty::Adt`, likely
to be quite a bit nasty. The project group has a long-term plan for how
to get around this kind of issue and move the associated functions into
libcore, but that will likely take time as well. Since all routes
forward are temporally costly, we probably will skip the lang item
approach entirely and go the "proper" route, but in the interests of
having something this year for people to play around with, this
extension trait was whipped up.

For now, while it involves a lot of fairly internal details most users
shouldn't have to care about, I went ahead and fully documented the
situation for any passerby to read on the trait, as the situation is
quite unusual and puzzling to begin with.
 impl std::simd::StdFloat

 This introduces an extension trait to allow use of floating point methods
 that need runtime support. It is *excessively* documented because its mere
 existence is quite vexing, as the entire thing constitutes a leakage of
 implementation details into user observable space. Eventually the entire
 thing will ideally be folded into core and restructured to match the rest
 of the library, whatever that structure might look like at the time. This
 is preferred in lieu of the "lang item" path because any energy the lang
 items require (and it will be significant, by Simulacrum's estimation) is
 better spent on implementing our libmvec.
This significantly simplifies codegen and should improve mask perf.

Co-authored-by: Jacob Lifshay <programmerjake@gmail.com>
The way the macro expands, it may sometimes infer
"this is a uint, but doesn't impl Neg???"
Also, I made the "wrong path for intrinsics" error.
These fixes allow integration into libcore.
@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jan 21, 2022
@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 21, 2022
@rust-log-analyzer

This comment has been minimized.

Make available the remaining float intrinsics that require runtime support
from a platform's libm, and thus cannot be included in a no-deps libcore,
by exposing them through a sealed trait, `std::simd::StdFloat`.

We might use the trait approach a bit more in the future, or maybe not.
Ideally, this trait doesn't stick around, even if so.
If we don't need to intermesh it with std, it can be used as a crate,
but currently that is somewhat uncertain.
@workingjubilee
Copy link
Member Author

Apologies if you had already started review, I pulled in the latest commits from portable-simd. I don't plan to move the sync point ahead again, it's just that we received so many requests for this Simd::cast function since we first merged into nightly, and we only recently cleared the other barriers for it, so I wanted to have it included as soon as you're ready.

@Mark-Simulacrum
Copy link
Member

Just to confirm: these changes are seeing upstream review, right? Typically the subtree part of a subtree bump doesn't need (detailed) review, since it's already been reviewed.

The std changes here look OK to me from an integration perspective -- r=me after confirming that the other changes have been reviewed upstream (and you're not desiring another pass or specific examination of any of them)

It may make sense to just glob re-export some part of std_simd rather than specifically exporting the StdFloat trait, but there's advantages to this approach to and it seems OK for now.

@workingjubilee
Copy link
Member Author

workingjubilee commented Jan 31, 2022

'kayo! I will make a note of that in the future!

And yes indeed, we are making sure we review each other's work here. I only really wanted to make sure the way I pushed things into std this time around seemed okay.
Thank you!
@bors r=Mark-Simulacrum

@bors
Copy link
Contributor

bors commented Jan 31, 2022

📌 Commit e96159e has been approved by Mark-Simulacrum

@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 Jan 31, 2022
@matthiaskrgr
Copy link
Member

@bors rolllup=never (in case of perf impact)

@matthiaskrgr
Copy link
Member

@bors rollup=never

@bors
Copy link
Contributor

bors commented Feb 3, 2022

⌛ Testing commit e96159e with merge 796bf14...

@bors
Copy link
Contributor

bors commented Feb 3, 2022

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing 796bf14 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 3, 2022
@bors bors merged commit 796bf14 into rust-lang:master Feb 3, 2022
@rustbot rustbot added this to the 1.60.0 milestone Feb 3, 2022
@rust-highfive
Copy link
Collaborator

📣 Toolstate changed by #93146!

Tested on commit 796bf14.
Direct link to PR: #93146

💔 miri on windows: test-pass → test-fail (cc @RalfJung @eddyb @oli-obk).

rust-highfive added a commit to rust-lang-nursery/rust-toolstate that referenced this pull request Feb 3, 2022
Tested on commit rust-lang/rust@796bf14.
Direct link to PR: <rust-lang/rust#93146>

💔 miri on windows: test-pass → test-fail (cc @RalfJung @eddyb @oli-obk).
@bors bors mentioned this pull request Feb 3, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (796bf14): comparison url.

Summary: This benchmark run did not return any relevant results.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

@workingjubilee workingjubilee deleted the use-std-simd branch February 3, 2022 18:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants