Add dynamic swizzles for 128-bit u8/i8 vectors#265
Conversation
Shnatsel
left a comment
Shnatsel
left a comment
There was a problem hiding this comment.
The semver hazard is a concern. The rest is less critical.
But I am excited to see this, because this gives us a real edge over std::simd which requires -Z build-std for dynamic swizzles to perform well: https://doc.rust-lang.org/stable/std/simd/struct.Simd.html#method.swizzle_dyn
And we also can get better performance by not adhering to the "out of bounds is 0" rule, making it opt-in via a bitmask for the users that need it instead.
| fn swizzle_dyn_i8x16(self, a: i8x16<Self>, idxs: u8x16<Self>) -> i8x16<Self> { | ||
| let mut dest = [Default::default(); 16]; | ||
| for (i, idx) in idxs.val.0.iter().copied().enumerate() { | ||
| dest[i] = a.val.0.get(idx as usize).copied().unwrap_or(0); | ||
| } | ||
| dest.simd_into(self) | ||
| } |
There was a problem hiding this comment.
Fallback implementations are usually manually unrolled so that platforms we don't support via intrinsics would have a better shot at autovectorization, see e.g.
fearless_simd/fearless_simd/src/generated/fallback.rs
Lines 1009 to 1114 in df33f0c
I'd prefer to have this fully unrolled style applied here as well.
There was a problem hiding this comment.
Separately, instead of get().unwrap_or(0) I would prefer get((idx % 16) as usize ) which should be easier for the compiler to autovectorize and matches the current x86 behavior. But that's ultimately up to you, I'm not going to block this PR on that.
| fn swizzle_dyn_u8x16(self, a: u8x16<Self>, idxs: u8x16<Self>) -> u8x16<Self> { | ||
| let mut dest = [Default::default(); 16]; | ||
| for (i, idx) in idxs.val.0.iter().copied().enumerate() { | ||
| dest[i] = a.val.0.get(idx as usize).copied().unwrap_or(0); | ||
| } | ||
| dest.simd_into(self) | ||
| } | ||
| #[inline(always)] |
| /// Dynamic swizzle for byte vectors. | ||
| pub trait SimdSwizzleDyn<S: Simd>: SimdBase<S> + Seal { | ||
| fn swizzle_dyn(self, idxs: crate::u8x16<S>) -> Self; | ||
| } | ||
|
|
There was a problem hiding this comment.
The way this trait is defined locks us into u8x16 vectors for indices. Making it more general later is a semver-breaking change.
Something like SimdSwizzleDyn<Idxs, S> or an associated type Indices would be more future-proof, implemented today with u8x16<S> only for some types.
Indices sounds like it would be useful on SimdBase but I guess we can move that to SimdBade later once the implementation matures.
|
What we could easily provide for all vector sizes is swizzling within 128-bit blocks, which maps very naturally to all widths. |
|
Superseded by #266. |
2 participants
Swizzles are a whole can of worm and I do not intend to figure out how we should implement this generically right now. 😄 I think in the long term, we will mostly want to mirror portable_simd where we have
swizzlefor static swizzles (unfortunately this is currently a bit hard to implement due to current limitations of const generics) andswizzle_dynfor dynamic ones.In any case, for now I just want to have a simple method we can use in vello to convert RGBA vectors into BGRA during packing. We know the indices statically so that would be the best, but as mentioned this one's harder to implement so for now I decided to just go with dynamic swizzles instead. In order to keep things simple, I'm limiting this implementation
So despite the complexity of swizzles, I hope adding this is non-controversial. The only controversial part is probably how to deal with OOB indices, but in the interest of getting the best performance I decided to not explicitly zero out invalid ones on x86, instead documenting that the result for those is unspecified.