llvm / llvm-project

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

llvm.minnum and llvm.maxnum do not handle -0.0 as required by IEEE-754 #49230

Open 10db7940-d837-414a-8e65-1a0425eb097b opened 3 years ago

10db7940-d837-414a-8e65-1a0425eb097b commented 3 years ago
Bugzilla Link 49886
Version trunk
OS All
CC @topperc,@RKSimon,@rotateright

Extended Description

Note: this applies for both llvm.minnum and llvm.maxnum, although for the most part I'm just going to talk about the min case — both have this bug, though.

The documentation for llvm.minnum.* says:

Follows the IEEE-754 semantics for minNum, except for handling of signaling NaNs But it goes on to say

If the operands compare equal, returns a value that compares equal to both operands. This means that fmin(+/-0.0, +/-0.0) could return either -0.0 or 0.0. These two statements are in contradiction, at as of IEEE-754 2019 (section 9.6, "Minimum and maximum operations"), which states:

minimumNumber(x, y) is x if x<y, y if y < x, and the number if one operand is a number and the other is a NaN. For this operation, −0 compares less than +0. If x = y and signs are the same it is either x or y. If both operands are NaNs, a quiet NaN is returned, according to 6.2. If either operand is a signaling NaN, an invalid operation exception is signaled, but unless both operands are NaNs, the signaling NaN is otherwise ignored and not converted to a quiet NaN as stated in 6.2 for other operations. In particular, "For this operation, −0 compares less than +0".

In Rust (https://github.com/rust-lang/rust/issues/83984) we use llvm.minnum and llvm.maxnum for our normal float min/max operations, and would like these to be compliant with the IEEE-754 functions, with respect to -0.0.

Alternatively, if this cannot be fixed, it would be nice to have a pair of set of min/max functions that do implement the IEEE754 semantics (and also probably update the documentation to say that the handling of -0.0 differs from IEEE-754 as well)

P.S. We cannot use llvm.minimum — While it would handle -0.0 properly, they handle NaNs completely differently, and we've documented the current "returns non-NaN for NaN input" behavior

P.P.S. I had no clue what component to put this under, sorry.

10db7940-d837-414a-8e65-1a0425eb097b commented 3 years ago

I don't think minimumNumber and minNum are the same thing in the IEEE spec I dug out IEEE754 2008 and it turns out that minNum was removed and replaced with minimumNumber, which (for our purposes) essentially is the same function as minNum, but with -0 treated as less than +0.

(I had been confused, and thought that it was just the LLVM docs abbreviating minimumNumber as minNum, apologies)

Does the IEEE description for minimumNumber match the behavior of llvm.minimum? llvm.minimum propagates NaN, and minimumNumber returns the other arg if one is NaN (same as the way llvm.minnum does).

For clarity: in addition to minimumNumber, IEEE754 2019 defines a minimum function that does behaves identically to llvm.minimum. (It defines 2 max functions and 2 min functions)

For Rust this wouldn't be usable here, as we've already guaranteed the that f32::min will return the other argument when one argument is a NaN.

Is there a way to read the updated standard without going through some kind of login/paywall? There are various methods you can use to access research papers, academic publications, etc. Your local library may be one, although there may be certain websites that fulfill similar roles without requiring logins, which may be convenient during lockdown.

rotateright commented 3 years ago

This patch was supposed to update the LangRef: https://reviews.llvm.org/D67507 ...but it's gone nowhere for over a year.

Is there a way to read the updated standard without going through some kind of login/paywall?

topperc commented 3 years ago

Does the IEEE description for minimumNumber match the behavior of llvm.minimum? I don't think minimumNumber and minNum are the same thing in the IEEE spec.