Try using real `_mm_mulhi_epu16` now that it's no longer unsafe

[Shnatsel]

Rust 1.87 made SIMD intrinsics that don't touch pointers safe to call: rust-lang/stdarch#1714

Due to the requirement to match libwebp output bit-for-bit we're using an emulation of the _mm_mulhi_epu16 intrinsic used by libwebp:

image-webp/src/vp8.rs

Lines 971 to 974 in 93baf7d

	/// `_mm_mulhi_epu16` emulation used in `Frame::fill_rgb` and `Frame::fill_rgba`.
	fn mulhi(v: u8, coeff: u16) -> i32 {
	((u32::from(v) * u32::from(coeff)) >> 8) as i32
	}

It would be interesting to try replacing it with the actual _mm_mulhi_epu16 now that it doesn't need unsafe and see if that changes the assembly in any way (hopefully for the better).

Here's the current state on godbolt, and it produces quite a lot of assembly: https://rust.godbolt.org/z/3afeY35EG

[Shnatsel]

This also opens the door to straight up porting libwebp functions implemented in SSE and NEON intrinsics.

For example, libwebp's SSE2 equivalent of the math from our fill_rgb function can be found here: https://github.com/webmproject/libwebp/blob/8534f53960befac04c9631e6e50d21dcb42dfeaf/src/dsp/yuv_sse2.c#L25-L68

[fintelia]

Are there safe intrinsics for converting a slice to/from SSE or NEON vectors?

[Shnatsel]

Intrinsics starting with _mm_set are used for populating the __m128i type with values. They are safe to call, so we can just use those combined with .chunks_exact(), just like we already do for autovectorization.

If that messes with optimizations somehow, then bytemuck will also work, but treating in-memory values as __m128i will require alignment, so we will need a scalar loop both at the start and the end of the array as opposed to just at the end when we can use unaligned loads.

[Shnatsel]

According to Intel documentation, _mm_mulhi_epu16 lowers into the pmulhuw instruction. That instruction already appears twice in the assembly created by the autovectorizer, see the Godbolt link above. LLVM instruction selection keeps surprising me!

There may still be benefit to porting the libwebp explicit SIMD to Rust, either piecemeal or wholesale, and seeing if that does anything. We won't even need any kind of runtime dispatch if we stick to SSE2 and NEON since both of those are guaranteed to be available on 64-bit architectures.

[fintelia]

One place that might benefit from explicit SIMD is the subtract_green transform. I saw a modest end-to-end improvement just by adding a std::simd implementation via this patch.

[awxkee]

I've seen _mm_mulhi_epu16 run 300–500% slower than _mm_mulhi_epi16 on AMD CPUs a few times probably because it's microcoded on some chips. And these are modern CPUs, so I'm trying to stick to signed arithmetic where possible.

I'm not suggesting anything, just a sad fact how it works sometimes.

[Shnatsel]

I looked at the subtract_green transform and figured it should autovectorize easily. Turns out that no, it doesn't.

So here's the portable_simd version: https://rust.godbolt.org/z/GGe1KTq6M

The only thing that actually emitted vector instructions in my testing was a direct translation of the portable SIMD version. I haven't benchmarked it but it looks terrifying: https://rust.godbolt.org/z/Gz6fPhr59

[okaneco]

Safe SIMD intrisics are exciting.
Unfortunately, it doesn't seem like there's a safe way to convert back from the vector register type to an array without something like bytemuck.

The good news is that using _mm_set/_mm_setr with chunks_exact seems to optimize fine.

I wrote the subtract_green transform in Compiler Explorer with intrinsics and the only difference seems to be some instruction ordering.
https://rust.godbolt.org/z/MbGahcG3q

Using bytemuck::must_cast instead of mem::transmute on the playground appeared to produce the same assembly.

---

Another feature to look forward to for MSRV bump is the stabilization of the as_chunks in 1.88.

Since it returns arrays, it should be a straight improvement over chunks_exact in most cases. It allows for irrefutably destructuring arrays and fewer bounds checks since the compiler doesn't need coaxing about the length which can be helpful for more optimization.

The only issue I've seen is that it can sometimes aggressively unroll loops.

SIMD subtract_green written with as_chunks_mut: https://rust.godbolt.org/z/xYb4dM8n5
We don't need the unreachable branches and can pattern match on the tail of the array. The tail handling gets handled differently than chunks_exact.

[Shnatsel]

The portable_simd version was never merged, so this implementation in terms of intrinsics be a valuable addition!

IDCT might be another good target. The implementation we have now produces scalar code on SSE2.

On the other hand I'm not looking forward to hand-writing intrinsics for x86, NEON and maybe WASM separately. https://docs.rs/wide/latest/wide/ looks promising as a thin abstraction/portability layer on top of intrinsics, but I can already see it's missing the rotate instructions and is overall rather barebones. On the other hand its simplicity makes it easy to audit, same as bytemuck; if the maintainers strong prefer this approach, we could contribute more operations upstream. MSRV wasn't bumped to 1.87 so there's plenty of unsafe going around, but it's isolated to the safe_arch crate.

There's also https://docs.rs/pulp/0.21.5/pulp/ which exposes more SIMD operations, and even provides a form of runtime dispatch. It contains some unsafe since it didn't bump MSRV to Rust 1.87 yet, and some of the operations are implemented using inline assembly rather than intrinsics.

[okaneco]

Is there interest in image-rs hosting its own crate for safe, unaligned load/store SIMD wrappers?
These intrinsics are straightforward to use correctly, and the scope would be limited to only the architectures the project would use.

I worked on a crate which does that and would be happy to "hand it off" if so.

Background

This issue inspired me to create a now-rejected¹ libs-team ACP rust-lang/libs-team#601 for some kind of safe conversion between arrays and SIMD types.

The libs-team is amenable to a safe wrapper API but wants to see the ecosystem prove it out first.

After the rejection, I started working on a minimal crate that could eventually be upstreamed which works on regular array references, is no_std, and has no dependencies.
https://github.com/okaneco/safe_unaligned_simd

Looking at the existing third party wrappers, I'm a little hesitant as they mainly seem to be higher level abstractions, have a lot of code/code gen to audit, and suited more for compile-time than runtime. I would rather use intrinsics or wait for portable_simd to stablize (fearless_simd also looks interesting but it's too early).

Crate implementation details

I've implemented all the x86 instructions I planned to for SSE, SSE2, and AVX load/store intrinsics resulting in 38 functions, which covers SSE2 and AVX2. Test coverage makes up the majority of lines of code.

The safety conditions are partially encoded in the function signature by taking Rust references as arguments and casting the references to raw pointers. Miri should be run in CI eventually.

Unsafe usage ends up mostly being (&[T; N]).as_ptr().cast()/ptr::from_mut(x).cast() for passing to the stdarch intrinsics.

The main design choice is a sealed marker trait for the _si128 or _si256 "bag of bits" loads/stores.
https://github.com/okaneco/safe_unaligned_simd/blob/c01aaeb4adb36c16ca922ec6cafa67baa76ec814/src/x86_64.rs#L25
_mm_storeu_si128 implementation. Conveniently, the traits don't need to be imported for users to call these functions.
https://github.com/okaneco/safe_unaligned_simd/blob/c01aaeb4adb36c16ca922ec6cafa67baa76ec814/src/x86_64/sse2.rs#L167

I believe NEON load/stores are all unaligned and they have more sensible types so it wouldn't need any traits.

---

With this, the only unsafe code that would need to be written for SIMD is the call to the target feature function after verifying the feature exists with the feature detection macro.

Footnotes

1. It seemed to get sidetracked on drawbacks of one specific implementation and then waiting for portable_simd to stabilize.
   There was brief movement in the portable_simd zulip discussing stabilizing the types for conversion but it's still unknown how long before they're ready to stabilize anything (trait rework to be done). ↩

[197g]

We could host it, you only shouldn't expect that to imply a volume of outside contributions by itself (although I'd hope so). I've also been looking for a better safely wrapped way to write processing code other than portable-simd given the interface in image-texel and the fact that experiments comparing that to autovectorization have been a mixed bag or disastrous in a few instances. An alternative approach that proves better performance would be cool to see. Independence of portable-simd may actually be a sub-goal here, they focus on providing a common subset algorithms while we mainly want platform-specific optimal assembly in a safe way. Writing different implementations for different CPUs and targets isn't itself the big problem, maintaining safety and correctness between them is.

[197g]

@okaneco Regarding the design, I appreciate how complementary release of slice::as_chunks is with regards to that API design. Combined we can construct most unaligned iteration with SIMD loads from any input and output array; with bytemuck the type family of possible inputs again grows tremendously.

---

As a form of wish list, there are also a few other instruction class that I've come across as potentially valuable for processing data in images (and otherwise) which could fit well into a similar design:

• a few more loads, i.e. https://doc.rust-lang.org/stable/core/arch/x86/fn._mm256_broadcast_sd.html, https://doc.rust-lang.org/stable/core/arch/x86/fn._mm256_loadu2_m128.html were not made available. Maybe we should just await Rust here though. Combining loads might be most useful of the bunch and should be a straight-forward extension of the implementation.
• important but harder are masked gather and store: https://doc.rust-lang.org/stable/core/arch/x86/fn._mm256_mask_i32gather_epi32.html However, a bounds-checked version of this could be made safe and might be effective enough when combined with optimization (llvm models quite a few vectorized instructions) or just not as expensive for the compare+or building the verified mask.
• providing maskload as a fn(_: &[_]) -> __m256i with an automatic mask: https://doc.rust-lang.org/stable/core/arch/x86/fn._mm256_maskload_epi32.html
• (The previous two designs could also take different index types that are bounded by type system tricks but that is maybe too intricate. The bounds would need to be tied to the slice argument making this a subset of dependent typing. Already implemented that for normal slice indexing, it's sometimes faster but also not greatly practical)
• non temporal loads and stores: https://doc.rust-lang.org/stable/core/arch/x86/fn._mm256_stream_load_si256.html but do note the caveat as documented: https://doc.rust-lang.org/stable/core/arch/x86/fn._mm_sfence.html (which applies to ARM equally). I do think it's possible to wrap this with a scope-API comparable to std::thread::scope that consumes mutably borrowed output arrays for before allowing you to store to them, and ensures an sfence before the scope ends and you could access them again. But please verify this independently, I'd want to be abundantly cautious here.

Well if we had all of that, I'm pretty sure most of our needs would be covered. (Well apart from instruction set vendors and OS's having to agree that the cpuid bits will remain valid across scheduler decisions on heterogeneous systems. I did have a nice chat about that at RustWeek and at least Intel is aware of the problem here and the generic cpuid result is the common subset of all clusters. Sucks for performance but makes our cpu-feature discovery dispatch sound).

[okaneco]

Independence of portable-simd may actually be a sub-goal here

I agree and that was part of the motivation. It still struggles with loads/stores and is more of an enhanced auto-vectorization than explicit SIMD for some use cases.

I think contiguous load/stores cover most use cases for existing image encoder/decoder applications.

Regarding the wish list

_mm256_loadu2_ and vbroadcast

I did include the _mm256_loadu2_* and _mm256_storeu2_* families so we're good there unless I missed something else.

Good point on broadcasts: they're trivial to implement but I got sidetracked...
I noticed that the avx variants missed being marked as safe and submitted a PR to do that a few days ago.
rust-lang/stdarch#1834
Their CI has been broken since then due to a rustc change in the process of being reverted.

(edit: implemented AVX broadcast intrinsics here okaneco/safe_unaligned_simd@3e3f163)

VMASKMOV and VGATHER

For the gather and masked load/store, I had qualms about providing safe interfaces due to the reference I could find. Searching "memory mapped" in the following PDF, on pages 5-248, 5-261, 5-267, 5-273, 5-279:

Intel® Architecture Instruction Set Extensions Programming Reference - February 2012
https://www.scss.tcd.ie/jeremy.jones/CS4021/41417-319433-012.pdf

VMASKMOV

VMASKMOV should not be used to access memory mapped I/O as the ordering of the
individual loads or stores it does is implementation specific.

VGATHERDPD/VGATHERQPD, VGATHERDPS/VGATHERQPS, VPGATHERDD/VPGATHERQD, VPGATHERDQ/VPGATHERQQ

• This instruction should not be used to access memory mapped I/O as the
  ordering of the individual loads it does is implementation specific, and some
  implementations may use loads larger than the data element size or load
  elements an indeterminate number of times.

Definitely room to experiment with better APIs for those, but it should probably still be marked unsafe I think.

I also didn't want to touch the Non-Temporal intrinsics in the MVP because I believed them to still be outside of the Rust memory model. Maybe we can handle them safely but that would need more research/expert consultation.

[Shnatsel]

Having a & reference without an UnsafeCell in the underlying type, such as &[u8], guarantees that the data will not change behind your back.

Or at least it should. Rust memory mapping wrappers blithely ignore that and deref to &[u8] anyway because all the correct alternatives like Cell<u8> are far too slow.