Change vectors to be generic over element type. #154
Conversation
calebzulawski
requested review from
workingjubilee,
Lokathor and
programmerjake
There was a problem hiding this comment.
Most important thing to note upfront: despite the pretty significant implementation changes, the API is basically unchanged.
The API has undergone a significant change, IMO, in that it is a lot easier to use generically in most ways. Admittedly that's not what everyone might typically be thinking by API, but because SIMD often involves type shenanigans, I think it is relevant, eh?
Which is to say I think this will probably be a vast improvement for actual use also.
I have very small legibility remarks but I think this is good to merge.
| #[doc = concat!("# use core::", stringify!($ty), "::{MIN, MAX};")] | ||
| /// let x = Simd::from_array([MIN, -2, 3, MAX]); | ||
| /// let unsat = -x; | ||
| /// let sat = x.saturating_neg(); | ||
| #[doc = concat!("assert_eq!(unsat, ", stringify!($name), "::from_array([MIN, 2, -3, MIN + 1]));")] | ||
| #[doc = concat!("assert_eq!(sat, ", stringify!($name), "::from_array([MAX, 2, -3, MIN + 1]));")] | ||
| /// assert_eq!(unsat, Simd::from_array([MIN, 2, -3, MIN + 1])); | ||
| /// assert_eq!(sat, Simd::from_array([MAX, 2, -3, MIN + 1])); |
There was a problem hiding this comment.
This diff alone is honestly pretty much enough to sell me on this on its own.
| pub struct Mask<Element, const LANES: usize>( | ||
| <LaneCount<LANES> as SupportedLaneCount>::BitMask, | ||
| PhantomData<Element>, | ||
| ) |
There was a problem hiding this comment.
I am finding the use of a non-terse type variable syntax (Element instead of E) has been inspiring me to look for the struct or trait it represents and get confused when I can't find any. This is not so big a deal with const generics because those are visibly called out.
There was a problem hiding this comment.
That's a good point. The standard library tends to use a single letter. I would probably just opt for T if we go that route.
There was a problem hiding this comment.
Decided to go with T, like arrays and Vec.
|
i think if you make a type alias and then impl methods on that type alias, rustdoc will collect the methods there. However, that would halfway undo the PR |
|
Even if that is true, is that desirable? It may be a temporary "solution" but in some ways I think it's more confusing because we want people to use |
|
In practice many things still can't be generic over your element type anyway. Slightly jiggering things to make rustdoc behave a little better doesn't make it any worse. |
Co-authored-by: Jubilee <[email protected]>
|
Given there are multiple routes of exploration for improving the docs I am okay with deferring them somewhat to a later PR, even if they matter. |
Most important thing to note upfront: despite the pretty significant implementation changes, the API is basically unchanged.
This PR results in changing
SimdI32<4>toSimd<i32, 4>,Mask8<2>toMask<i8, 2>, etc.This results in a pretty substantial reduction of the API, which now only contains 6 things (not counting type aliases):
Simd,Mask,SimdElement,MaskElement,LaneCount, andSupportedLaneCount.This allows functions such as
splat,shuffle,gather_or,lanes_eq,select, etc to be implemented generically once for all vectors, rather than once for each element type (with tons of macros 🤢). Users can still call these functions in the same way as before this PR--only the implementation is different.There is one notable downside: rustdoc really does not know how to handle this situation and the page for the
Simdtype is massive. This PR does not attempt to unify implementations over similar types, such as all float types or all unsigned integers. A future PR changing some of the functions to trait functions could reduce this and make those functions a little more discoverable. Alternatively, there might be some improvements we can suggest/add to rustdoc to help wrangle the extra functions.