Add `[T]::as_simd(_mut)` by scottmcm · Pull Request #91479 · rust-lang/rust (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation16 Commits2 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

SIMD-style optimizations are the most common use for [T]::align_to(_mut), but that's unsafe. So these are safe wrappers around it, now that we have the Simd type available, to make it easier to use.

impl [T] { pub fn as_simd(&self) -> (&[T], &[Simd<T, LANES>], &[T]); pub fn as_simd_mut(&mut self) -> (&mut [T], &mut [Simd<T, LANES>], &mut [T]); }

They're cfg'd out for miri because the simd module as a whole is unavailable there.

SIMD-style optimizations are the most common use for [T]::align_to(_mut), but that's unsafe. So these are safe wrappers around it, now that we have the Simd type available, to make it easier to use.

impl [T] {
    pub fn as_simd<const LANES: usize>(&self) -> (&[T], &[Simd<T, LANES>], &[T]);
    pub fn as_simd_mut<const LANES: usize>(&mut self) -> (&mut [T], &mut [Simd<T, LANES>], &mut [T]);
}

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

Choose a reason for hiding this comment

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

This looks fine, modulo the question it raises and the nits.

@@ -3434,6 +3436,87 @@ impl [T] {
}
}

/// Split a slice into a prefix, a middle of aligned simd types, and a suffix.

Choose a reason for hiding this comment

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

/// Split a slice into a prefix, a middle of aligned simd types, and a suffix.
/// Split a slice into a prefix, a middle of aligned SIMD types, and a suffix.

Comment on lines 3441 to 3444

/// This is a safe wrapper around [`slice::align_to`], so has the same weak
/// preconditions as that method. Notably, you must not assume any particular
/// split between the three parts: it's legal for the middle slice to be
/// empty even if the input slice is longer than `3 * LANES`.

Choose a reason for hiding this comment

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

Since this wrapper is safe, this should clarify that this is an invalid assumption for unsafe code, as the function can no longer be relied upon to maintain invariants in the same way. Safe code trying to "rely" on it would be "merely" logically incorrect.

Choose a reason for hiding this comment

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

Oh, I wrote "preconditions" here when I meant "postconditions" 🤦

unsafe { self.align_to() }
}

/// Split a slice into a prefix, a middle of aligned simd types, and a suffix.

Choose a reason for hiding this comment

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

/// Split a slice into a prefix, a middle of aligned simd types, and a suffix.
/// Split a slice into a prefix, a middle of aligned SIMD types, and a suffix.

Comment on lines +3489 to +3491

// SAFETY: The simd types have the same layout as arrays, just with
// potentially-higher alignment, so the de-facto transmutes are sound.
unsafe { self.align_to() }

Choose a reason for hiding this comment

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

Sound, yes, but it would likely exhibit unexpected behavior if LANES is an odd number (like 3), as that would potentially result in bytes no longer being read (because the stride of the type will be as if LANES is 4). This is not supported yet, but we have not yet ruled this possibility out. This function therefore introduces an unanswered question. Should we:

allow people to transmute data into padding
add additional bounds on this if we relax those on LANES
rule out types like Simd<f32, 3> entirely?

Choose a reason for hiding this comment

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

Well I guess my assert_eq on the sizes (line 3487) is more useful than I'd thought, as if 3-simd is padded out to 4-simd it'd make the function just panic instead of skipping things.

The other possibility would be to have it just return everything in the prefix for that case, since align_to is allowed to do that anyway (for MIRI), and it'd just be slower than desired if someone uses the non-power-of-two sizes.

Choose a reason for hiding this comment

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

I didn't consider that, but it is indeed also an option!
This is definitely a bridge we can cross when we actually come to it, I just wanted to note the bridge is indeed up ahead.

apiraino added the T-libs

Relevant to the library team, which will review and decide on the PR/issue.

label

dtolnay added T-libs-api

Relevant to the library API team, which will review and decide on the PR/issue.

and removed T-libs

Relevant to the library team, which will review and decide on the PR/issue.

labels

lgtm once all the comments are resolved

yaahc added S-waiting-on-author

Status: This is awaiting some action (such as code changes or more information) from the author.

and removed S-waiting-on-review

Status: Awaiting review from the assignee but also interested parties.

labels

@workingjubilee I've pushed updates; please take a look and see if they satisfactorily address your comments.

And since it's LGTM from yaahc,
@bors delegate=workingjubilee

Yeah, this looks fine!
@bors r+ rollup=always

📌 Commit e4c44c5 has been approved by workingjubilee

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-author

Status: This is awaiting some action (such as code changes or more information) from the author.

labels

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request

…bilee

Add [T]::as_simd(_mut)

SIMD-style optimizations are the most common use for [T]::align_to(_mut), but that's unsafe. So these are safe wrappers around it, now that we have the Simd type available, to make it easier to use.

impl [T] {
    pub fn as_simd<const LANES: usize>(&self) -> (&[T], &[Simd<T, LANES>], &[T]);
    pub fn as_simd_mut<const LANES: usize>(&mut self) -> (&mut [T], &mut [Simd<T, LANES>], &mut [T]);
}

They're cfg'd out for miri because the simd module as a whole is unavailable there.

bors added a commit to rust-lang-ci/rust that referenced this pull request

…askrgr

Rollup of 7 pull requests

Successful merges:

rust-lang#90521 (Stabilize destructuring_assignment)
rust-lang#91479 (Add [T]::as_simd(_mut))
rust-lang#91584 (Improve code for rustdoc-gui tester)
rust-lang#91886 (core: minor Option doc correction)
rust-lang#91888 (Handle unordered const/ty generics for object lifetime defaults)
rust-lang#91905 (Fix source code page sidebar on mobile)
rust-lang#91906 (Removed in_band_lifetimes from library\proc_macro)

Failed merges:

r? @ghost @rustbot modify labels: rollup

They're cfg'd out for miri because the simd module as a whole is unavailable there.

The SIMD module is only commented out for doctests, but otherwise available... but I guess that is enough to require these extra cfg here. Which is a shame because I only wanted to disable the tests in Miri, not actually hide any APIs. If there are better ways to disable doctests in a part of a crate, I'd be interested to learn about them. :)

Labels

S-waiting-on-bors

Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

Relevant to the library API team, which will review and decide on the PR/issue.