`_mm_movemask_epi8` unexpectedly triggers Undefined Behavior

[Nugine]

playground

use core::arch::x86_64::*;

#[target_feature(enable = "sse2")]
pub unsafe fn is_ascii_sse2(a: &[u8; 16]) -> bool {
    let a = _mm_loadu_si128(a.as_ptr().cast());
    let m = _mm_movemask_epi8(a);
    m == 0
}

fn main() {
    assert!(cfg!(target_feature = "sse2"));
    let s = b"helloworld123456";
    assert!(unsafe { is_ascii_sse2(s) });
}

error: Undefined Behavior: each element of a SIMD mask must be all-0-bits or all-1-bits
    --> /playground/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/../../stdarch/crates/core_arch/src/x86/sse2.rs:1381:5
     |
1381 |     simd_bitmask::<_, u16>(a.as_i8x16()) as u32 as i32
     |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ each element of a SIMD mask must be all-0-bits or all-1-bits
     |
     = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
     = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
     = note: BACKTRACE:
     = note: inside `std::arch::x86_64::_mm_movemask_epi8` at /playground/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/../../stdarch/crates/core_arch/src/x86/sse2.rs:1381:5
note: inside `is_ascii_sse2` at src/main.rs:6:13

See also:

• https://doc.rust-lang.org/stable/core/arch/x86_64/fn._mm_movemask_epi8.html
• https://www.intel.com/content/www/us/en/docs/intrinsics-guide/index.html#text=_mm_movemask_epi8

[RalfJung]

I am fairly sure I was told that simd_bitmask requires each element to be all-0 or all-1... so is that _mm_movemask_epi8 buggy? Cc @workingjubilee

[Nugine]

Related:

• Use simd_bitmask intrinsic in a couple of places stdarch#1331

[bjorn3]

I am fairly sure I was told that simd_bitmask requires each element to be all-0 or all-1... so is that _mm_movemask_epi8 buggy?

Didn't know that. If that is indeed the case please revert rust-lang/stdarch#1331.

[RalfJung]

https://doc.rust-lang.org/nightly/std/simd/struct.Mask.html#method.from_int_unchecked indicates that this is a preconditon of from_int_unchecked, which I took as a sign that the underlying intrinsic has that requirement.

Cc @calebzulawski

[calebzulawski]

It isn't documented anywhere (none of the SIMD intrinsics are, which isn't good) but yes, my understanding is that the various bitmask intrinsics expect all 0 or 1 lanes.

[Nugine]

_mm[256]_movemask_epi8 has defined behavior:

Returns a mask of the most significant bit of each element in a.

(regardless of the other bits)

Changing the requirement may introduce unsoundness into existing crates.

[calebzulawski]

To clarify, I mean the simd_* compiler intrinsics, not the vendor intrinsics. So I don't think it's appropriate to use it here.

[Nugine]

Close as it is not an issue of miri.
Moved to rust-lang/stdarch#1347

[Amanieu]

The documentation in rustc's codegen says that simd_bitmask only looks at the MSB:

https://github.com/rust-lang/rust/blob/bed4ad65bf7a1cef39e3d66b3670189581b3b073/compiler/rustc_codegen_llvm/src/intrinsic.rs#L1081-L1089

[RalfJung]

That's an internal documentation, not necessarily a stable API-side promise.

https://github.com/rust-lang/portable-simd/blob/master/crates/core_simd/src/intrinsics.rs is the closest thing we have to API docs for these intrinsics. It's not terribly detailed, but it is clear on this question

    // truncate integer vector to bitmask
    // `fn simd_bitmask(vector) -> unsigned integer` takes a vector of integers and
    // returns either an unsigned integer or array of `u8`.
    // Every element in the vector becomes a single bit in the returned bitmask.
    // If the vector has less than 8 lanes, a u8 is returned with zeroed trailing bits.
    // The bit order of the result depends on the byte endianness. LSB-first for little
    // endian and MSB-first for big endian.
    //
    // UB if called on a vector with values other than 0 and -1.
    #[allow(unused)]
    pub(crate) fn simd_bitmask<T, U>(x: T) -> U;

[RalfJung]

I'm not sure who is in charge of these intrinsics, btw -- usually intrinsics are t-lang territory, but I don't think they are involved here. This is so far just @rust-lang/project-portable-simd experimenting. As long as that is the case, the intrinsics should probably not be used outside of https://github.com/rust-lang/portable-simd/ . Everybody else should use the APIs exposed in core::simd instead.

[thomcc]

I don't think core::simd should have an explicit _mm_movemask_epi8-style operation (IOW, I don't think core::arch should be implemented in terms of core::simd), since that implies a lot about the underlying representation of the mask -- we want the Mask types to be usable on architectures that use bitmasks instead of full-width masks.

I think we'd say just to use core::arch for this... Using the simd_bitmask intrinsic is probably fine as far as portable-simd is concerned (although I can't speak for everybody), so long as it has tests for the behavior (that we'd trip if we changed something there).