s390x: incorrect use of LLVM intrinsics for vec_min/max on floats

[RalfJung]

The s390x vec_min/vec_max intrinsics on floats are meant to behave like the IEEE 754-2008 minNum/maxNum operation (with -0.0 < +0.0). However, we are currently using the llvm.minnum/llvm.maxnum intrinsic for them, which has different behavior for SNaN: LLVM is allowed to fold minnum(SNaN, x) into x but the intrinsic is supposed to always return a NaN for that situation. (I don't know if LLVM will actually ever use this freedom, but it is in the spec so who knows what it does in the future.)

We should be using llvm.s390.vfminsb and friends for this (as in 6efd06e), but that also does not work because the underlying hardware instruction is only available when the vector-enhancements-1 target feature is enabled, and whole llvm.minnum has a fallback that gets used when that target feature is missing, llvm.s390.* does not.

At the moment, I don't think there is a good way to fix this: the odd behavior of vec_min having to be available even when the underlying instruction is not available means that a fallback is needed, and that can only really be implemented in LLVM (or with a special hack in rustc codegen), but LLVM has no portable intrinsic that has the right SNaN behavior and the arch-specific intrinsic does not have the fallback behavior we need. I wonder what Clang does. (EDIT: see here for what clang does.)

Cc @uweigand

[RalfJung]

@nikic do you know if LLVM ever uses the freedom that minnum(SNaN, x) may return x? The fold added in llvm/llvm-project#184125 makes it return QNaN when LLVM knows that one input is SNaN. Are there any folds for the case where one input is a NaN but we don't know if it's signaling or quiet?

I guess guaranteeing a NaN result from an SNaN input also is a problem for lowering to a libcall as the C standard is extremely vague when it comes to SNaN behavior -- and for instance musl treats all NaN the same. I don't know if libcalls are ever used for vector inputs but the semantics are of course the same for all variants.

[uweigand]

The implementation of llvm.minnum can be provided by the LLVM target back-end, as long as the semantics are covered by the options allowed by the LLVM IR spec. On s390x, we can choose to always implement llvm.minnum for vector types using our instruction with mode M=0, whenever that instruction is available. (We currently choose M=4, but that is arguably a bug and should probably be changed anyway.)

If we do that, then I think it might be reasonable for Rust to rely on the knowledge that on s390x (and building for z14 or later), the llvm.minnum intrinsic provides additional guarantees that are compatible with even if not required by the LLVM IR spec. Would that approach make sense?

[folkertdev]

Parsing this, you're saying that you'd like to change LLVM so that llvm.minnum is equivalent to llvm.s390.vfmin{s, d}b (assuming llvm.s390.vfmin{s, b}b is available), right?

I think that would work well, and we'd just have to document that on our side to prevent confusion in the future. Maybe test it too in rust-lang/rust where we can properly test the assembly output.

[RalfJung]

@uweigand that reasoning is incorrect. LLVM applies target-independent optimizations before these intrinsics reach the backend, and those optimizations can do anything that's compatible with the semantics described in the LangRef. Additional guarantees provided by the backend are entirely useless on the LLVM IR level. As an LLVM frontend, the only guarantees we can actually use are the ones described in the LLVM LangRef.

[uweigand]

@uweigand that reasoning is incorrect. LLVM applies target-independent optimizations before these intrinsics reach the backend, and those optimizations can do anything that's compatible with the semantics described in the LangRef. Additional guarantees provided by the backend are entirely useless on the LLVM IR level. As an LLVM frontend, the only guarantees we can actually use are the ones described in the LLVM LangRef.

Fair enough. I had assumed common optimizations would always follow IEEE semantics, and the choice option is really only for the benefit of back-ends. But of course you're right that there's no guarantee.

We could in theory implement a fallback for llvm.s390.vfminsb in LLVM, but that would be a bit odd - these low-level intrinsics are intended to match a specific instruction, are mapped to that instruction, and are not available if the instruction is not part of the target feature set. That's the case for all of them - making an exception for just these seems strange.

Usually, front-ends make the decision whether to use the instruction based on target features. If I understand correctly, the issue with Rust is that use of the intrinsic happens in a library, not in the compiler itself? (In the clang implementation, it's actually in a header file, which gets compiled together with target code, using the desired feature set ...)

Maybe I'm missing something, but how is this handled for the other feature-specific vec_ intrinsic? For one, all the vector float overloads of all vec_ intrinsics require vector-enhancements-1. And of course vector-enhancements-2 and vector-enhancements-3 all enable some additional vec_ intrinsics as well ...

If the only problem is that vec_min is available but emulated on z13 in our C extension, maybe this doesn't have to be the case for the Rust version? I'd be fine with the whole intrinsic requiring vector-enhancements-1 if that simplifies the implementation.

[RalfJung]

Usually, front-ends make the decision whether to use the instruction based on target features. If I understand correctly, the issue with Rust is that use of the intrinsic happens in a library, not in the compiler itself? (In the clang implementation, it's actually in a header file, which gets compiled together with target code, using the desired feature set ...)

Translation from Rust source code to MIR happens once, for the pre-built library. The vector target feature is not enabled there since it's not a default feature of the target. Later when a crate is compiled with RUSTFLAGS enabling more features, we translate that MIR to LLVM IR and LLVM sees that the enhancement-1 feature is present and uses it.

If the only problem is that vec_min is available but emulated on z13 in our C extension, maybe this doesn't have to be the case for the Rust version? I'd be fine with the whole intrinsic requiring vector-enhancements-1 if that simplifies the implementation.

Indeed, having two forms of the intrinsic (instruction vs fallback) is what makes this special.
So yeah we could just say that vec_min on f32 is UB when enhancements-1 is missing (just like it's already UB when "vector" is missing). I have no idea how programmers would know about this though, we also clearly need better documentation.

[uweigand]

Usually, front-ends make the decision whether to use the instruction based on target features. If I understand correctly, the issue with Rust is that use of the intrinsic happens in a library, not in the compiler itself? (In the clang implementation, it's actually in a header file, which gets compiled together with target code, using the desired feature set ...)

Translation from Rust source code to MIR happens once, for the pre-built library. The vector target feature is not enabled there since it's not a default feature of the target. Later when a crate is compiled with RUSTFLAGS enabling more features, we translate that MIR to LLVM IR and LLVM sees that the enhancement-1 feature is present and uses it.

If the only problem is that vec_min is available but emulated on z13 in our C extension, maybe this doesn't have to be the case for the Rust version? I'd be fine with the whole intrinsic requiring vector-enhancements-1 if that simplifies the implementation.

Indeed, having two forms of the intrinsic (instruction vs fallback) is what makes this special. So yeah we could just say that vec_min on f32 is UB when enhancements-1 is missing (just like it's already UB when "vector" is missing). I have no idea how programmers would know about this though, we also clearly need better documentation.

To be clear, vec_min would not even be available with f64 either. For most other vector (floating-point) instructions, we only supported the f64 variant with plain vector and added f32 with enhancements-1. That's why the f32 overloads of all intrinsincs require enhancements-1.

But the min/max instructions were completely new with enhancements-1 to begin with. In the C version of vec_min/max, like all other intrinsics they're not available for f32, but we provided the emulation for f64 to make it look more similar to the other intrinsics we had at the time - in hindsight that probably wasn't the smartest decision ...

Certainly agreed on the documentation aspect.

[RalfJung]

Ah, I must have misinterpreted https://www.ibm.com/docs/en/cpp-oel-zos/2.0.0?topic=arithmetic-vec-max-vector-maximum then... that seems to say that they are all available on z13 except for float which needs z14. I thought z13 = vector, z14 = vector-enhancements-1 but I guess that is wrong then.

[uweigand]

Ah, I must have misinterpreted https://www.ibm.com/docs/en/cpp-oel-zos/2.0.0?topic=arithmetic-vec-max-vector-maximum then... that seems to say that they are all available on z13 except for float which needs z14.

Well, yes - what this is intended to say is:

• all integer versions are available (and using native instructions) on z13
• float is only available (and using native instructions) on z14, will result in compile-time error on z13
• double is available but emulated on z13, and remains available but using native instructions on z14

Violating the condition in the MIN ARCH column will always result in a compile-time error, but if the condition is satisfied, the operation may still be emulated under certain conditions called out in the descriptive text. Maybe this could be expressed in some better way ...

I thought z13 = vector, z14 = vector-enhancements-1 but I guess that is wrong then.

No, that's correct.

[RalfJung]

Ah, now I get it, thanks. :)

I'm surprised it's a compile-time error. I assume runtime feature detection is supported, so just because the feature is not globally enabled doesn't mean it can't be used in some function that is (elsewhere, not visible to the compiler) runtime-gated on a feature check? I guess the compiler can check that based on the attribute of the function that invokes the vendor intrinsic. Kind of a shame that C is safer than Rust here... but that's a separate discussion.

[uweigand]

I'm surprised it's a compile-time error. I assume runtime feature detection is supported, so just because the feature is not globally enabled doesn't mean it can't be used in some function that is (elsewhere, not visible to the compiler) runtime-gated on a feature check? I guess the compiler can check that based on the attribute of the function that invokes the vendor intrinsic. Kind of a shame that C is safer than Rust here... but that's a separate discussion.

Well, in C if you want to have the compiler use a target feature in any function, the feature has to be enabled at compile-time at least for this function - even if use of that function is then gated at runtime. Enabling the feature for a function in C normally means putting the function into a separate source file built with the appropriate options. This will then affect the intrinsics (pulled from a header file) used by this file as well.

[ There is some facility to enable flags per-function using the attribute ((target_options)) GCC/LLVM extension. That is not a fully complete solution and in particular does not work for intrinsics defined in a header file. ]

[nikic]

I'd like to push back against the premise of this issue. If I understand correctly, you are basically suggesting that Rust's general NaN semantics (which are basically the same as LLVM's general NaN semantics) do not apply to target intrinsics. Why would that be the case?

Applying strict IEEE 754 sNaN semantics only really makes sense if everything applies them. I don't see why we would want to apply them for target intrinsics only. The arguments here are basically the same as to why the LLVM minnum/maxnum semantics are specified the way they are -- anything else is incoherent with the global FP NaN model.

Please keep in mind that this issue is larger than just FP min/max. For example, are you suggesting that we can't have target intrinsics lowering to simd_fmul or simd_fadd, because these have different sNaN semantics for 1.0 and -0.0 operands? I don't think we want to go there.

[RalfJung]

@nikic that seems like a discussion for rust-lang/rust#153990.
tl;dr -- my understanding is that we allow the NaN bit patterns to differ from what "real hardware" would do, but whether the result is a NaN or not must match the vendor intent. (IEEE semantics aren't very relevant here IMO, this is deep in vendor-controlled territory.)