Fix inaccurate std::intrinsics::simd documentation

[folkertdev]

This addresses two issues:

• the docs on comparison operators (simd_gt etc.) said they only work for floating-point vectors, but they work for integer vectors too.
• the docs on various functions that use a mask did not document that the mask must be a signed integer vector. Unsigned integer vectors would cause invalid behavior when the mask vector is widened (unsigned integers would use zero extension, producing incorrect results).

r? @workingjubilee

[rustbot]

Some changes occurred to the intrinsics. Make sure the CTFE / Miri interpreter
gets adapted for the changes, if necessary.

cc @rust-lang/miri, @rust-lang/wg-const-eval

Some changes occurred to the platform-builtins intrinsics. Make sure the
LLVM backend as well as portable-simd gets adapted for the changes.

cc @antoyo, @GuillaumeGomez, @bjorn3, @calebzulawski, @programmerjake

[calebzulawski]

Maybe outside the scope of this PR, but this would need to be checked in codegen and emit an ICE

[folkertdev]

what exactly? you get a monomorphization error when these rules are violated. They are not the prettiest errros, but they are not ICEs either. Unless I'm missing something. e.g. https://godbolt.org/z/f5j5re5z8

[saethlin]

Yep, that's a post-mono error.

[calebzulawski]

Oh, good. I misunderstood and thought you meant the intrinsic was permitting invalid behavior.

[folkertdev]

yeah this is just bringing the docs up to date with the behavior that we already have and enforce.

[RalfJung]

Looking at this in the LLVM backend I don't even see such a widening cast. This code converts the masks to i1 vectors which LLVM seems to use for them, and it does that with lshr and trunc. The lshr is entirely unnecessary for correctness as we require the input to be all-1 or all-0, but

    /// The rust simd semantics are that each element should either consist of all ones or all zeroes,
    /// but this information is not available to llvm. Truncating the vector effectively uses the lowest bit,
    /// but codegen for several targets is better if we consider the highest bit by shifting.

But I can't find anything that would go wrong with unsigned integers.

[folkertdev]

I personally agree, in that an intrinsic (that normal users should never have to worry about), we could either demand that the lane width matches the other arguments, or that the mask uses sign extension no matter the signedness of the argument.

From what I can tell the codegen backends already handle this, because e.g. in llvm (and from what i can see, also cranelift and gcc) the integers are just a bunch of bits.

The counter-argument was that performing sign extension on what rust believes is a vector with unsigned types would violate type safety.

[RalfJung]

The counter-argument was that performing sign extension on what rust believes is a vector with unsigned types would violate type safety.

I don't know what you mean by that. There's no sign extension happening, as far as I can tell. Also, all the backends have to do is implement the intended mask semantics, which is described in a bitwise way. If they use sign extension as part of that, that's completely fine. There's no type safety violation here.

[folkertdev]

I agree, the backends support it, and the intrinsic could (and in my opinion should) support it too. I'm just relaying the response I got when I suggested exactly that https://rust-lang.zulipchat.com/#narrow/channel/257879-project-portable-simd/topic/add.20.60simd_max.60.20and.20.60simd_min.60/near/502647748

[RalfJung]

Let's move this discussion to Zulip: https://rust-lang.zulipchat.com/#narrow/channel/257879-project-portable-simd/topic/On.20the.20sign.20of.20masks

[workingjubilee]

cool! looks good to me. test improvement stuff sounds nice as followup if people want but isn't required here and now.

@bors r+ rollup

[bors]

📌 Commit 854e9f4 has been approved by workingjubilee

It is now in the queue for this repository.