Add 64-bit integer vectors and operations on them - #253
Conversation
|
The documentation for load/store_interleaved_128 was misleading. Both formulations are valid for 32-bit elements but the 8- and 16-bit elements already behaved differently, following the NEON vld4/vst4 semantics rather than our documented semantics. This misled me into generalizing the op to 64-bit numbers incorrectly. I've changed the implementation back to vld4/vst4 semantics in subsequent commits and updated documentation. |
Add i64/u64 vector types and operations across the generated SIMD backends, with focused int64 coverage and optimized interleaved load/store paths where available.
|
I'm curious, do you think this will overall impact the compile time for the crate a lot, even if none of the 64-bit stuff is used? Have you done any measurements? |
|
It really shouldn't. This is all generic code, so it is not actually instantiated and doesn't turn into MIR or LLVM IR until something actually calls it. The downside of generics is that if we call the same function 5 times you get 5 different instantiations of it so 5x the IR for LLVM to chew through, but in our case we want all the intrinsics inlined anyway so this is unavoidable, generics or not. |
|
Marking this as draft until it's updated to latest main. Some refactoring should also be possible now that the SSE2 PR has laid the groundwork for nicer scalar fallback. |
…Lake is available
…ll scalar fallbacks use it, remove the code that duplicates it that originated in the i64 branch
| if vec_ty.scalar_bits == 64 | ||
| && matches!(vec_ty.scalar, ScalarType::Int | ScalarType::Unsigned) | ||
| && method != "simd_eq" | ||
| { | ||
| return fallback_method(op, vec_ty); | ||
| } | ||
|
|
There was a problem hiding this comment.
I don't like that this fires only when it's not AVX-512 but this is not explicitly under an else or a non-AVX-512 branch. I want to refactor all the early returns out of the generator, but that's best left to a follow-up PR, I don't want to cram even more unrelated changes into this one.
|
This should now be ready for review. |
LaurenzV
left a comment
There was a problem hiding this comment.
Once again mostly relying on tests here, haven't tried to verify the actual logic of each operation. If it's possible, I do think it would be nice to split out the from_fn change, just to keep the diff clean (not like anyone else will ever read it, but still 🙂).
There was a problem hiding this comment.
What about rotate/shift_elements_left/right and swizzle?
There was a problem hiding this comment.
that's covered in https://github.com/linebender/fearless_simd/pull/275/changes#diff-f8c0fbfc8f4c516a0abcc54128c32013ac7838e28285085fef7800fa7cce64c3
so not this PR but the follow-up that expands test coverage, stacked on top of this one
| pub(crate) fn unrolled_array(len: usize, item: impl FnMut(usize) -> TokenStream) -> TokenStream { | ||
| let items = (0..len).map(item).collect::<Vec<_>>(); | ||
| quote! { [#(#items),*] } |
There was a problem hiding this comment.
What's the motivation for this change? Did you verify whether this actually gives better assembly? Is that something that could be reverted for now to make the diff easier to read? Up to you.
There was a problem hiding this comment.
I noticed that these fallbacks are not the same style as the actual Fallback implementation and went ahead and changed it, but I didn't verify that the fully unrolled implementations produce better assembly, I just assumed that the fallbacks are that way for a reason.
There was a problem hiding this comment.
I've checked it for shl op and it seems LLVM unrolls those loops anyway, so it thinks that's profitable but I don't want to bet on LLVM preserving this optimization in the future so I'll keep the manual unrolling.
As far as I can tell these two formulations emit literally identical assembly, at least for shl as the op. I was surprised and suspected an error in my setup, but I double-checked and the results appear to hold.
I've inspected the optimized LLVM IR before lowering into assembly and it seems it gets recognized as a vector operation in both cases.
%values = bitcast <2 x i64> %a to <16 x i8>
%shifts = bitcast <2 x i64> %b to <16 x i8>
%masked = and <16 x i8> %shifts, splat (i8 7)
%result = shl <16 x i8> %values, %masked
| @@ -392,8 +392,25 @@ impl<S: Simd> SimdBase<S> for i8x16<S> { | |||
| block | |||
| } | |||
| #[inline(always)] | |||
| fn from_fn(simd: S, f: impl FnMut(usize) -> i8) -> Self { | |||
| simd.load_array_i8x16(core::array::from_fn(f)) | |||
| fn from_fn(simd: S, mut f: impl FnMut(usize) -> i8) -> Self { | |||
There was a problem hiding this comment.
Piggybacking on my other comment, does this really yield better results than before? Seems a bit suspicious. 😄
There was a problem hiding this comment.
There were some issues with it historically: rust-lang/rust#108765
But I guess it doesn't matter as much today. There's all sorts of considerations like panic safety and such, but measuring this rigorously is quite difficult, since you can put arbitrary functions in there and the properties probably vary depending on what you put there.
There was a problem hiding this comment.
I think we need to use wrapping_neg for negation, no? Also maybe add a test with i64::MIN to ensure it works.
There was a problem hiding this comment.
Yes, it should. That's an issue with the current code, not restricted to 64-bit ints.
There was a problem hiding this comment.
I've changed the scalar fallback behavior in this PR, and updated the tests in #275
… SIMD behavior more precisely in the ::MIN case
~~Stacked on top of linebender#253 which adds tests for 64-bit integer types~~ Rebased onto main after that was merged The first commit moves existing tests around. Tests that were haphazardly strewn about multiple files are now organized by operation, one file = one operation. This makes the tests much easier to navigate and makes the missing test coverage readily apparent. This also reduces the from-scratch build time with `cargo build --tests` from 19s to 14s on my machine. The two subsequent commits add 7k lines of tests to fill in coverage gaps. This was done with Python scripts and a bit of fixup. The generated tests are slightly different from the existing patterns in that they use `array::from_fn` as opposed to handwritten inputs and outputs. If that's a problem, one could probably put up an LLM to bringing them into the same style as the pre-existing tests, if desired.
There are two unrelated changes:
array::from_fn()callsSupersedes #97