Use specialized traits to generalize lane comparisons

[calebzulawski]

This fixes the issue where lane comparisons are determined by PartialEq and PartialOrd, which isn't correct. It also allows min, max, clamp, and horizontal_{min,max} to be generic over element type. This means that this PR also adds min, max, and clamp to integers.

The new traits SimdEq and SimdOrd probably should be part of the public interface, to allow calling the functions generically, but the functions in those traits should never be stabilized (and should probably be hidden, once that bug is worked out). Both of these traits are sealed by SimdElement.

We can use the results of this PR to inform the conversion of other functions to generic over element type.

[Lokathor]

If they should be public so people can use them generically then why should the methods never be stabilized?

[calebzulawski]

The traits are used in the bounds of the member fns of Simd, so you may want to check T: SimdOrd in order to be able to call Simd::<T, _>::min, but you shouldn't call <T as SimdOrd>::min directly.

[Lokathor]

what is the harm if someone did?

[calebzulawski]

There's no harm, it just complicates the API. There's no good reason to expose it because then we have the liberty to change it in the future. I suggested hiding them to simplify the docs as well.

The trait is effectively a marker to map type to intrinsic. A similar trait is std::convert::FloatToInt, which has a hidden intrinsic method as well.

[calebzulawski]

It's worth noting that Simd::min and SimdOrd::min are completely identical, so I don't see any good reason to provide the latter, in any situation where you have SimdOrd you can just call Simd::min.

[Lokathor]

I just (1) am fundamentally against perma-unstable anything (2) the kind of person that frequently implements traits for newtypes via macro, and i would usually like the possibility to implement all of a base type's traits on the newtype as well, should it be appropriate.

[calebzulawski]

In this case, these traits are just mapping intrinsics to types. They're sealed so users can't implement them on newtypes.

[programmerjake]

lgtm, though the Simd methods could just call the intrinsics directly if we wanted, rather than calling the methods on SimdOrd which then calls the intrinsics.

[programmerjake]

so, kinda like:

pub trait SimdOrd: ... {}

impl SimdOrd for f32 {}

impl<T: SimdOrd, const LANES: usize> Simd<T, LANES> {
    pub fn max(self, rhs: Self) -> Self {
        unsafe { max_intrinsic(self, rhs) }
    }
}

[calebzulawski]

That isn't possible in some cases, since the implementations for different types aren't the same (for example, min and max are different for integers and floats). I expect some other functions will work this way, too. This also makes it much easier in the future to add new element types that may have very different implementations (like the NonZero integers, which would probably convert to the underlying type and defer to their implementation).

[programmerjake]

I expect we will definitely want fp min/max intrinsics (llvm may have issues recognizing the patterns)...so, while we're at it, just have int min/max intrinsics too.

for this pr, yeah, we can just leave it with the calling SimdOrd methods, for future extension and for min/max right now.

[workingjubilee]

NonZero types that interact with their "base" type using ops that return a type other than bool have ops that return the NonZero type, not the underlying type.

[calebzulawski]

All I'm saying is that Simd::<NonZeroU8, _>::horizontal_max can't call the intrinsic directly, it'll call something like NonZeroU8::new_unchecked(self.get().horizontal_max()), and therefore the trait fn is necessary. The point of this PR is to be more explicit about implementations for every type, rather than assuming everything uses the intrinsics.

[workingjubilee]

There's no harm, it just complicates the API. There's no good reason to expose it because then we have the liberty to change it in the future. I suggested hiding them to simplify the docs as well.

If there's no harm in having them exposed, then we should simply have them exposed, unless we actually intend to remove them later. And if this proposal is serious, then we do not. I disagree that it simplifies the docs: #[doc(hidden)] is not actually a promise that no one can see or use the function, and hiding the methods needlessly obfuscates the code to Rust programmers. I am aware that people on crates.io use that as a convention, but it isn't how Rust's stability promises work regarding the traits std implements.

This is true even if the trait is also sealed, and there is no reason to leave these traits as permanently unstable. People should be able to depend on the trait after stabilization for the types for which it is implemented for.

[calebzulawski]

I understand your sentiment but I disagree with what these traits mean. SimdEq exists purely for the purpose of gating Simd::lanes_eq to types that actually implement equality, and for allowing us to define that implementation per type. The interface that users call should always be Simd::lanes_eq. SimdEq::lanes_eq exists as an implementation detail. We should be free to remove it, change it, etc, without worrying if any users are using it.

Identically, something that's already in std: when you convert an f32 to an int you use f32::to_int_unchecked, not <f32 as FloatToInt<{integer}>>::to_int_unchecked, because the latter is an implementation detail, a trait wrapper around an intrinsic. Making that part of the public rust API by stabilizing it (and removing the #[doc(hidden)]) is unnecessary and bloats the API with no benefit to the user (and adds some confusion because now there are two duplicate functions!).

[workingjubilee]

There is a tracking issue for that trait's stabilization.
rust-lang/rust#67057

So no, I do not believe that reasoning applies.

[Lokathor]

I would argue the other way: Simd::lanes_eq exists because we don't want to fill the prelude with more stuff and we don't yet have intrinsic trait impls.

[calebzulawski]

I guess whether or not it's part of the API is somewhat immaterial to this PR--I wish it weren't but it doesn't make a huge difference either way. Like FloatToInt, I'd prefer if we make the functions hidden purely from the documentation perspective (isn't documentation the whole reason we went down this road of traits in the first place?) but if we must stabilize them I won't argue against it and I still think this PR is the right way to go.

[workingjubilee]

Regarding SimdAbs:

I suggested traits because I felt we should have {Simd?}Float and {Simd?}{S,U,}Int, not SimdAbs, and that the traits should define that the vectors they are implemented for implement a natural subset of the behaviors of floats or integers, with very well-defined variations. That would keep the number of traits down to a sane and simple scope. I did offer a SimdAbs impl variation, briefly, but I no longer am inclined to such. Even so, I am not perturbed by the possible asymmetry.

[thomcc]

This is not a good direction to go in, IMO.

As far as I can tell, the only reason to have SimdEq and SimdOrd is to support user types in Simd, which we should just not support. If you recall, we were careful to require that vectorized comparison (and essentially all other operations) was semantically the same as the scalar comparison in a loop — which isn't something we get for free (portable NaN handling has a cost on e.g. x86).

Keep in mind: even having SimdElement at all is already a step beyond most similar features in the stdlib (compare with core::sync::atomic, scalar integers/floats, ...), so we're already much more flexible and generic than we need to be.

People who want Simd<UserType, N> can write a transparent wrapper around Simd<Scalar, N> (where UserType is a transparent wrapper around Scalar), and convert/transmute on use. Again, precedent here is core::sync::atomic which we're already way more flexible and generic than.

Similarly, Simd<NonZeroInt, _> sounds like a headache for everybody. People who want to have niches in their vectors are more than capable of using [NonZeroInt; N] and convert to a vector on use. People who want to semantically forbid zeros can just wrap Simd<Int, _>.

TBH, given the bikeshedding and discussion about hypothetical future use cases we won't be able to add compatibly... I can see why the stdlib usually opts not to make traits for these things (or opts not to make them public). Designing a trait like this without excluding some hypothetical future use cases is both impossible, and seen as the Worst Possible Thing.

[calebzulawski]

Okay, I'm going to roll the discussions from the comment thread and the main thread into this comment, to go through all the critiques in one place.

Are these traits required at all?
SimdEq is roughly equivalent to PartialEq, but there is no way to provide min, max, and clamp for floats, because PartialOrd does not provide those functions! They are part of Ord, but we can't use that either, since it is not implemented for f32 or f64. Even if we did use PartialOrd for the comparisons, we would still need a separate trait for these functions. Not to mention all of the other functions (saturating math, abs, sqrt, etc) that don't correspond to an existing trait in std will need their own std::simd trait, anyway. In the future I imagine having something like SimdSaturating (saturating arithmetic), SimdSigned (for abs, checking signs, etc), and then perhaps SimdInt, SimdFloat, for more specific operations (conversions, sqrt, trig, etc).

User types
This PR has nothing to do with user types. These traits are sealed by SimdElement and I (personally) have no desire to ever support user types.

Library newtypes
NonZero integers is just one good example, I think another good example is std::num::Saturating. Considering how common saturating arithmetic is in SIMD, I could see that being very commonly used. NonNull is also a good candidate, considering it has some useful semantic meaning beyond providing niches. It's not a type yet, but I've seen some people advocate for NonNan float types, which would also help std::simd significantly. I'm not suggesting we rush to support every single primitive newtype that exists, but I don't think a design that expressly prohibits them is a good idea, either.

Implementing newtypes by extending intrinsics
In the thread above, @workingjubilee suggested that instead of individually implementing each function as done in this PR, you could instead extend the intrinsics to support newtypes. While this is theoretically possible, I don't think this is the way to go. Consider the following example:

Imagine you want to add support for an arbitrary newtype (let's use all of the NonZero integers, and assume #[repr(simd)] has been appropriately updated to support newtypes as well). Now you must modify all of the following:

• the LLVM backend intrinsics
• the cranelift backend intrinsics
• the hypothetical scalar polyfill library

My concerns with this approach:

• newtypes are promoted to something "special" in the compiler, somewhere near the level of primitives
• newtype logic that can be easily represented in plain rust in std::simd (like simd_abs(x.get())) must now instead be duplicated in each backend
• implementing a new backend would require an inordinate amount of knowledge about each individual newtype

While this approach could theoretically work, I think it makes more sense for the intrinsics to only use primitive types, and handle logic around newtypes within std::simd. Using traits like in this PR allows you to customize how the various functions work for each newtype and maintain a clear distinction between newtypes at the library level and primitives at the compiler level.

[workingjubilee]

In the thread above, @workingjubilee suggested that instead of individually implementing each function as done in this PR, you could instead extend the intrinsics to support newtypes.

I do not desire this, I stated that it is what limits the current interface, and that we originally designed SimdElement to mean "A machine scalar type", exactly matching the bounds in typeck as we found them. I do not desire this proposed extension, and I do not desire this infinite increase to the scope of our work. I do not want you to say I "suggested" something when I enumerate alternate paths, as it would have to mean I desire it, or even think it is a good idea, for it to actually be a suggestion. I have stated this boundary multiple times. Please do not transgress it again.

I agree with Thom. We should not go down this path.
NonZeroInt is just another user type: it merely happens that the user is std.
And there is no clear reason to support Simd<Saturating<i32>, N> instead of Saturating<Simd<i32, N>>.

[calebzulawski]

Sorry, when I said suggest I didn't mean "advocate", I meant "propose to be considered".

You're right that newtypes could potentially be handled by wrapping Simd, but that further complicates passing through the more complicated APIs like scatter/gather. The nice thing about Simd is that it's a container. Again, what I am getting at is that I'm not ready to make the decision today to prohibit using newtypes within a vector.

Maybe it was my mistake picking the comparison functions first, but I wanted to target the most complicated ones first. If we don't go down this path, can I ask how we should implement min, max, and clamp?

[workingjubilee]

Sorry, when I said suggest I didn't mean "advocate", I meant "propose to be considered".

Thank you.

I prefer "enumerate", as it is appropriately tediously dry and void of implication beyond the monotonic iteration of a thinking machine over multiple variations.

[calebzulawski]

After a discussion with @workingjubilee, I've made some changes that I believe clarify the purpose and reasoning behind these changes.

There are two traits:

• SimdPartialEq, which indicates that a type can be used with vector PartialEq operations
• SimdPartialOrd, which indicates that a type can be used with vector PartialOrd operations

These traits indicate vector semantics, marking which vector operations can be done. The implementations of these operations are handled by sealed traits.

The reasoning behind these changes is simply that different types need different implementations, and relying on PartialEq and PartialOrd does not convey enough information to implement everything. Type wrappers such as the NonZero integers are one example, but even primitive integers and floats are significantly different and require different implementations. clamp, for example, stands out as it's not possible to write a generic implementation for both integers and floats.

[programmerjake]

generally lgtm, though we should consider renaming lanes_eq to eq (and other similar functions) or max to lanes_max (and other similar functions) so the lane-wise operations are named consistently.