search

llvm / llvm-project

The LLVM Project is a collection of modular and reusable compiler and toolchain technologies.
http://llvm.org
Other
27.99k stars 11.55k forks source link

llvm.minnum should be lowered to fminimum_numf instead of fminf #93033

Open wzssyqa opened 3 months ago

wzssyqa commented 3 months ago

In https://llvm.org/docs/LangRef.html#llvm-minnum-intrinsic

Unlike the IEEE-754 2008 behavior, this does not distinguish between signaling and quiet NaN inputs. If a target’s implementation follows the standard and returns a quiet NaN if either input is a signaling NaN, the intrinsic lowering is responsible for quieting the inputs to correctly return the non-NaN input (e.g. by using the equivalent of llvm.canonicalize).

While in https://www.gnu.org/software/libc/manual/html_node/Misc-FP-Arithmetic.html

If an argument is a quiet NaN, the other argument is returned. If both arguments are NaN, or either is a signaling NaN, NaN is returned.
wzssyqa commented 3 months ago 
define float @mins(float %x, float %y) {
   %r = tail call float @llvm.minnum.f32(float %x, float %y)
   ret float %r
 }

Glibc introduce fminimum_numf since 2021. It may be a trouble.

dtcxzyw commented 3 months ago

cc @arsenm @andykaylor @jcranmer-intel

arsenm commented 3 months ago

Did the glibc behavior also change for fmin/fmax? I remember testing this long ago.

LLVM doesn't pretend to handling signaling nans correctly for non-strictfp functions. We also should rename the minnum/maxnum intrinsics; the name was always wrong. We should instead have something like minnum2019, minnum2008, and fmin for the range of behaviors

wzssyqa commented 3 months ago

Did the glibc behavior also change for fmin/fmax? I remember testing this long ago.

Ohh, yes, it does. https://sourceware.org/git/?p=glibc.git;a=commit;h=90f0ac10a74b2d43b5a65aab4be40565e359be43 I guess it's due to C23 changes: https://www.open-std.org/jtc1/sc22/wg14/www/docs/n3096.pdf Page 270

LLVM doesn't pretend to handling signaling nans correctly for non-strictfp functions. We also should rename the minnum/maxnum intrinsics; the name was always wrong. We should instead have something like minnum2019, minnum2008, and fmin for the range of behaviors

I guess that we should fellow the naming scheme with IEEE754, if we'd like to support the same behavior of IEEE. In 2008, it has minNUM, which cares about sNaN; In 2019, it has minimumNUM introduced, and minNUM is dropped.

If we try to support our flavor operations, I think that we should use a quite different naming scheme, such as fmin_num etc.

wzssyqa commented 3 months ago

Ohh, maybe we should update the documents of [‘llvm.minnum.*’ Intrinsic](https://llvm.org/docs/LangRef.html#id2157) and drop

Unlike the IEEE-754 2008 behavior, this does not distinguish between signaling and quiet NaN inputs. If a target’s implementation follows the standard and returns a quiet NaN if either input is a signaling NaN, the intrinsic lowering is responsible for quieting the inputs to correctly return the non-NaN input (e.g. by using the equivalent of llvm.canonicalize).

I guess this claims existed due to that the fmin(3) had this behavior before C2X.

arsenm commented 3 months ago

Did the glibc behavior also change for fmin/fmax? I remember testing this long ago.

Ohh, yes, it does. https://sourceware.org/git/?p=glibc.git;a=commit;h=90f0ac10a74b2d43b5a65aab4be40565e359be43 I guess it's due to C23 changes: https://www.open-std.org/jtc1/sc22/wg14/www/docs/n3096.pdf Page 270

fminimum and fmaximum are new and separate operations. They are not a change to the behavior of fmin/fmax. Do you have a reference to a change in the fmin implementation, specifically?

LLVM doesn't pretend to handling signaling nans correctly for non-strictfp functions. We also should rename the minnum/maxnum intrinsics; the name was always wrong. We should instead have something like minnum2019, minnum2008, and fmin for the range of behaviors

I guess that we should fellow the naming scheme with IEEE754, if we'd like to support the same behavior of IEEE. In 2008, it has minNUM, which cares about sNaN; In 2019, it has minimumNUM introduced, and minNUM is dropped.

It's not dropped, it's now called minimumNumber which coexists with minimum. It is stronger than the 2008 definition since it definitively orders -0 as less than +0

wzssyqa commented 3 months ago

Did the glibc behavior also change for fmin/fmax? I remember testing this long ago.

Ohh, yes, it does. https://sourceware.org/git/?p=glibc.git;a=commit;h=90f0ac10a74b2d43b5a65aab4be40565e359be43 I guess it's due to C23 changes: https://www.open-std.org/jtc1/sc22/wg14/www/docs/n3096.pdf Page 270

fminimum and fmaximum are new and separate operations. They are not a change to the behavior of fmin/fmax. Do you have a reference to a change in the fmin implementation, specifically?

https://sourceware.org/git/?p=glibc.git;a=blobdiff;f=manual/arith.texi;h=edb9cfdafbd3119bf785802b0bcfb58f28590cf5;hp=6a158e624d8f48c8290e527efd5a4dcf09c6d7ac;hb=90f0ac10a74b2d43b5a65aab4be40565e359be43;hpb=5bf07e1b3a74232bfb8332275110be1a5da50f83

It's in the diff of manual/arith.texi. Or maybe it is just a documentation update.

arsenm commented 3 months ago

https://godbolt.org/z/z7WG3q3Gq glibc does seem to be following the IEEE sNaN behavior. On MacOS, it does not and treats the snan the same as nan.

wzssyqa commented 3 months ago

So, my proposal is to emit fminimum_numf if we are using -std=gnu23/c23.

https://github.com/llvm/llvm-project/pull/93159

wzssyqa commented 3 months ago

It's not dropped, it's now called minimumNumber which coexists with minimum. It is stronger than the 2008 definition since it definitively orders -0 as less than +0

In the documentation of IEEE754-2019, there is:

NOTE — The minNum and maxNum operations of the 2008 version of the standard have been replaced by
the recommended operations of 9.6.

And there is no other talking about minNum or maxNum.

arsenm commented 3 months ago 
NOTE — The minNum and maxNum operations of the 2008 version of the standard have been replaced by
the recommended operations of 9.6.

If you actually look at 9.6, it has this: 9.6 Minimum and maximum operations 9.6.0, which lists minimum, minimumNumber, maximum, and maximumNumber. So yes, "minNum" was removed and replaced with "minimum" and "minimumNumber"

So mostly this is a question of wrangling the behavior of different implementations. The LangRef and some of the lowerings will need to change. The LangRef behavior as written matches the behavior on macOS. The apparent glibc behavior on x86 matches the IEEE sNaN handling. So some combination of the lowering and the LangRef will need to change.

The AArch64 ISA documentation isn't enlightening me on the instruction behavior there.

Additionally, OpenCL says "fmin and fmax behave as defined by C99 and may not match the IEEE 754-2008 definition for minNum and maxNum with regard to signaling NaNs. Specifically, signaling NaNs may behave as quiet NaNs."

I would be happy if we could get the minnum behavior to match the IEEE 2019 behavior, matching the sNaN behavior and also strengthening the signed 0 assumption. The outlier lowerings would then have to adjust

wzssyqa commented 3 months ago

Maybe we can define a group of new intrinsics, and keep fminnum there to keep compatible. We can have some work on frontend (clang) to emit different IR intrinsics for different platform.

My suggestion is:

fmin_c99
fmin_c23
fminimum
fminimum_num
arsenm commented 3 months ago

fmin_c99 fmin_c23

Was there an actual change here, or just clarification? I still don't see the diff that says the behavior changed. I think this was always the implemented behavior in glibc. I'd prefer to just fix minnum to match IEEE and then if necessary at fmin for the ignore-snan case? Or we can just fix the lowering for macOS

fminimum

We already have this

wzssyqa commented 3 months ago

Was there an actual change here, or just clarification? I still don't see the diff that says the behavior changed. I think this was always the implemented behavior in glibc. I'd prefer to just fix minnum to match IEEE and then if necessary at fmin for the ignore-snan case? Or we can just fix the lowering for macOS

I'd prefer it too. For macOS, I have a test with your code: https://godbolt.org/z/z7WG3q3Gq It has same behavior as IEEE2008's minNUM.

Darwin MacBook-Air-2 23.4.0 Darwin Kernel Version 23.4.0: Fri Mar 15 00:12:41 PDT 2024; root:xnu-10063.101.17~1/RELEASE_ARM64_T8103 arm64
> clang -v                                                                                                                                                         /tmp
Apple clang version 15.0.0 (clang-1500.3.9.4)
Target: arm64-apple-darwin23.4.0
Thread model: posix
InstalledDir: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin
jcranmer-intel commented 3 months ago

fmin_c99 fmin_c23

Was there an actual change here, or just clarification? I still don't see the diff that says the behavior changed. I think this was always the implemented behavior in glibc. I'd prefer to just fix minnum to match IEEE and then if necessary at fmin for the ignore-snan case? Or we can just fix the lowering for macOS

The normative text hasn't changed, but the example code for fmax was changed to add in a canonicalize step. C11 doesn't mention fmin in Annex F as mapping to anything in IEEE 754, but C2x does explicitly state that fmin maps to the superseded minNum operation of IEEE 754-2008. (Also note that C11 explicitly does not define sNaN behavior, while C23 defines optional support for sNaN--see F.2.1 in both versions).

To clear up potential confusion here: there are several variants of minimum, based on NaN-behavior and -0/+0 comparisons.

IEEE operation IEEE version C name -0 < +0 op(qNaN, y) op(sNaN, y)
minNum <= 2008 fmin recommended y qNaN + FE_INVALID
minimum >= 2019 fminimum required qNaN qNaN + FE_INVALID
minimumNumber >= 2019 fminimum_num required y y + FE_INVALID

Note that all three operations have distinct behavior, although fmin and fminimum_num differ only in sNaN behavior, which LLVM does not generally respect anyways, at least without strictfp and constrained intrinsics. Also many implementations implement fmin incorrectly (including, as I've just looked up right now, llvm-libc); glibc fixed its fmin implementation about 8 years ago: https://sourceware.org/bugzilla/show_bug.cgi?id=20947

We don't have an llvm intrinsic for fminimum_num yet.

jcranmer-intel commented 3 months ago

Er, sorry, minNum should be == 2008 for IEEE version, as IEEE 754-1985 didn't have a minNum operation.

arsenm commented 3 months ago

glibc fixed its fmin implementation about 8 years ago: https://sourceware.org/bugzilla/show_bug.cgi?id=20947

So yes, when the intrinsics were added (and later then clarified to have the libm sNaN behavior), glibc had the wrong behavior. I guess that means we really should just make minnum match the IEEE behavior and add something new for the old

The OpenCL footnote also wasn't here last I looked, haven't bothered to track down where that appeared.

arsenm commented 2 months ago

Introducing a new intrinsic doesn't solve the current intrinsic behavior being broken

wzssyqa commented 2 months ago

Introducing a new intrinsic doesn't solve the current intrinsic behavior being broken

Sure. I add minimum_num first due to that some arch has minimumNumber hardware instructions. They will be happy to use these new intrinsics. So we can reuse the current code to implement them.

And I am working on change the fminf ones now, and I will submit a new PR.