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

Vectors of pointers #287

Merged
merged 15 commits into from
Oct 29, 2022
Merged

Vectors of pointers #287

merged 15 commits into from
Oct 29, 2022

Conversation

calebzulawski
Copy link
Member

  • Adds vectors of pointers
  • Changes scatter/gather implementation (but doesn't introduce new pointer API, yet)
  • Modifies Simd::cast, since all casts are no longer valid (e.g. casting f32 to a pointer)

Copy link
Member

@programmerjake programmerjake left a comment

Choose a reason for hiding this comment

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

lgtm, assuming this is changed once simd int <-> ptr casts land in rustc

@calebzulawski
Copy link
Member Author

New intrinsics merged, so this is ready to go.

@workingjubilee
Copy link
Contributor

Huzzah.

Copy link
Member

@programmerjake programmerjake left a comment

Choose a reason for hiding this comment

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

once those two wrapping_sub doc comments are fixed, lgtm

@workingjubilee
Copy link
Contributor

Apologies, going to try to get to this this week.

@calebzulawski
Copy link
Member Author

Okay thanks!

Copy link
Contributor

@workingjubilee workingjubilee left a comment

Choose a reason for hiding this comment

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

Thank you for this! Sorry for being so distracted. Mostly things look good but I have a few questions?

crates/core_simd/src/cast.rs Outdated Show resolved Hide resolved
crates/core_simd/src/cast.rs Outdated Show resolved Hide resolved
@@ -71,3 +73,37 @@ macro_rules! impl_mask {
}

impl_mask! { i8, i16, i32, i64, isize }

impl<T, const LANES: usize> SimdPartialEq for Simd<*const T, LANES>
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems okay. Are there any caveats, e.g. if the pointers get evaluated at const time, that we should document?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure it has any caveats beyond what's already true for const fn or whatever. Did you have anything specific in mind?

Copy link
Contributor

Choose a reason for hiding this comment

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

I am thinking of the pointer indeterminacy problem during CTFE. Things that would share addresses during runtime may not during compile-time function execution, and vice versa.

Copy link
Member Author

Choose a reason for hiding this comment

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

Honestly, not sure what the implications would be other than since the SIMD intrinsics aren't const (yet) so we know any pointers for now will be runtime only. If you do something bad with CTFE before putting it in a vector, that's probably out of scope for now.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it's fair to assume all vector operations should mirror scalar operations in that regard, whatever the implications are.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough. I think it's mostly something we should keep in mind while exploring the API surface.

Comment on lines +305 to +312
#[inline]
fn simd_clamp(self, min: Self, max: Self) -> Self {
assert!(
min.simd_le(max).all(),
"each lane in `min` must be less than or equal to the corresponding lane in `max`",
);
self.simd_max(min).simd_min(max)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The notion of clamping pointers seems somewhat dubious...?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is odd, but pointers already implement Ord 🤷‍♂️

crates/core_simd/src/vector.rs Show resolved Hide resolved
crates/test_helpers/src/lib.rs Show resolved Hide resolved
crates/test_helpers/src/lib.rs Show resolved Hide resolved
crates/test_helpers/src/lib.rs Show resolved Hide resolved
Comment on lines +156 to +163
/// equivalent to `T as U` semantics, specifically for pointers
pub(crate) fn simd_cast_ptr<T, U>(ptr: T) -> U;

/// expose a pointer as an address
pub(crate) fn simd_expose_addr<T, U>(ptr: T) -> U;

/// convert an exposed address back to a pointer
pub(crate) fn simd_from_exposed_addr<T, U>(addr: T) -> U;
Copy link
Member

Choose a reason for hiding this comment

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

It look like currently no tests actually call any of these intrinsics, or am I missing something?

That would explain why portable-simd tests still pass in Miri where these intrinsics are not implemented.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, looking at the tests, I did omit some of the very simple ones like this. They shouldn't be hard to add.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants