Revert "LangRef: Clarify llvm.minnum and llvm.maxnum about sNaN and signed zero (#112852)" #138451
Conversation
…igned zero (llvm#112852)" This reverts commit 363b059. I don't agree with the change itself, but even if we want to do it, it needs to be phased in a lot more carefully than it was. You can't just change the semantics in LangRef without actually dealing with the consequences of the change. Revert to the status quo for now. Fixes llvm#138303.
nikic
requested review from
andykaylor,
arsenm,
dtcxzyw,
efriedma-quic,
fhahn,
jcranmer-intel and
wzssyqa
|
@llvm/pr-subscribers-llvm-ir @llvm/pr-subscribers-llvm-selectiondag Author: Nikita Popov (nikic) ChangesThis reverts commit 363b059. I'm not a big fan of the change itself (because it is incoherent with LLVM's general sNaN semantics), but even if we want to do it, it needs to be phased in a lot more carefully than what has actually happened. You can't just change the semantics in LangRef and then completely ignore the consequences of the change on optimization behavior. Revert to the status quo for now. If we actually want to do this, we first need to ensure that the new minimumnum intrinsics have optimization and codegen parity with the old minnum, and ensure produces are migrated to them in advance, and then we need to land the LangRef changes as part of a patch stack that adjusts optimizations to respect the new semantics. Fixes #138303. Full diff: https://github.com/llvm/llvm-project/pull/138451.diff 2 Files Affected:
diff --git a/llvm/docs/LangRef.rst b/llvm/docs/LangRef.rst
index c8cc38c23cff3..2dd297a795c26 100644
--- a/llvm/docs/LangRef.rst
+++ b/llvm/docs/LangRef.rst
@@ -16936,7 +16936,7 @@ versions of the intrinsics respect the exception behavior.
- qNaN, invalid exception
* - ``+0.0 vs -0.0``
- - +0.0(max)/-0.0(min)
+ - either one
- +0.0(max)/-0.0(min)
- +0.0(max)/-0.0(min)
@@ -16980,30 +16980,21 @@ type.
Semantics:
""""""""""
-Follows the semantics of minNum in IEEE-754-2008, except that -0.0 < +0.0 for the purposes
-of this intrinsic. As for signaling NaNs, per the minNum semantics, if either operand is sNaN,
-the result is qNaN. This matches the recommended behavior for the libm
-function ``fmin``, although not all implementations have implemented these recommended behaviors.
-
-If either operand is a qNaN, returns the other non-NaN operand. Returns NaN only if both operands are
-NaN or if either operand is sNaN. Note that arithmetic on an sNaN doesn't consistently produce a qNaN,
-so arithmetic feeding into a minnum can produce inconsistent results. For example,
-``minnum(fadd(sNaN, -0.0), 1.0)`` can produce qNaN or 1.0 depending on whether ``fadd`` is folded.
-IEEE-754-2008 defines minNum, and it was removed in IEEE-754-2019. As the replacement, IEEE-754-2019
-defines :ref:`minimumNumber <i_minimumnum>`.
+Follows the IEEE-754 semantics for minNum, except for handling of
+signaling NaNs. This match's the behavior of libm's fmin.
-If the intrinsic is marked with the nsz attribute, then the effect is as in the definition in C
-and IEEE-754-2008: the result of ``minnum(-0.0, +0.0)`` may be either -0.0 or +0.0.
+If either operand is a NaN, returns the other non-NaN operand. Returns
+NaN only if both operands are NaN. If the operands compare equal,
+returns either one of the operands. For example, this means that
+fmin(+0.0, -0.0) returns either operand.
-Some architectures, such as ARMv8 (FMINNM), LoongArch (fmin), MIPSr6 (min.fmt), PowerPC/VSX (xsmindp),
-have instructions that match these semantics exactly; thus it is quite simple for these architectures.
-Some architectures have similiar ones while they are not exact equivalent. Such as x86 implements ``MINPS``,
-which implements the semantics of C code ``a<b?a:b``: NUM vs qNaN always return qNaN. ``MINPS`` can be used
-if ``nsz`` and ``nnan`` are given.
-
-For existing libc implementations, the behaviors of fmin may be quite different on sNaN and signed zero behaviors,
-even in the same release of a single libm implemention.
+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_maxnum:
@@ -17040,30 +17031,20 @@ type.
Semantics:
""""""""""
-Follows the semantics of maxNum in IEEE-754-2008, except that -0.0 < +0.0 for the purposes
-of this intrinsic. As for signaling NaNs, per the maxNum semantics, if either operand is sNaN,
-the result is qNaN. This matches the recommended behavior for the libm
-function ``fmax``, although not all implementations have implemented these recommended behaviors.
-
-If either operand is a qNaN, returns the other non-NaN operand. Returns NaN only if both operands are
-NaN or if either operand is sNaN. Note that arithmetic on an sNaN doesn't consistently produce a qNaN,
-so arithmetic feeding into a maxnum can produce inconsistent results. For example,
-``maxnum(fadd(sNaN, -0.0), 1.0)`` can produce qNaN or 1.0 depending on whether ``fadd`` is folded.
+Follows the IEEE-754 semantics for maxNum except for the handling of
+signaling NaNs. This matches the behavior of libm's fmax.
-IEEE-754-2008 defines maxNum, and it was removed in IEEE-754-2019. As the replacement, IEEE-754-2019
-defines :ref:`maximumNumber <i_maximumnum>`.
+If either operand is a NaN, returns the other non-NaN operand. Returns
+NaN only if both operands are NaN. If the operands compare equal,
+returns either one of the operands. For example, this means that
+fmax(+0.0, -0.0) returns either -0.0 or 0.0.
-If the intrinsic is marked with the nsz attribute, then the effect is as in the definition in C
-and IEEE-754-2008: the result of maxnum(-0.0, +0.0) may be either -0.0 or +0.0.
-
-Some architectures, such as ARMv8 (FMAXNM), LoongArch (fmax), MIPSr6 (max.fmt), PowerPC/VSX (xsmaxdp),
-have instructions that match these semantics exactly; thus it is quite simple for these architectures.
-Some architectures have similiar ones while they are not exact equivalent. Such as x86 implements ``MAXPS``,
-which implements the semantics of C code ``a>b?a:b``: NUM vs qNaN always return qNaN. ``MAXPS`` can be used
-if ``nsz`` and ``nnan`` are given.
-
-For existing libc implementations, the behaviors of fmin may be quite different on sNaN and signed zero behaviors,
-even in the same release of a single libm implemention.
+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_minimum:
@@ -19942,8 +19923,12 @@ The '``llvm.vector.reduce.fmax.*``' intrinsics do a floating-point
matches the element-type of the vector input.
This instruction has the same comparison semantics as the '``llvm.maxnum.*``'
-intrinsic. If the intrinsic call has the ``nnan`` fast-math flag, then the
-operation can assume that NaNs are not present in the input vector.
+intrinsic. That is, the result will always be a number unless all elements of
+the vector are NaN. For a vector with maximum element magnitude 0.0 and
+containing both +0.0 and -0.0 elements, the sign of the result is unspecified.
+
+If the intrinsic call has the ``nnan`` fast-math flag, then the operation can
+assume that NaNs are not present in the input vector.
Arguments:
""""""""""
@@ -19971,8 +19956,12 @@ The '``llvm.vector.reduce.fmin.*``' intrinsics do a floating-point
matches the element-type of the vector input.
This instruction has the same comparison semantics as the '``llvm.minnum.*``'
-intrinsic. If the intrinsic call has the ``nnan`` fast-math flag, then the
-operation can assume that NaNs are not present in the input vector.
+intrinsic. That is, the result will always be a number unless all elements of
+the vector are NaN. For a vector with minimum element magnitude 0.0 and
+containing both +0.0 and -0.0 elements, the sign of the result is unspecified.
+
+If the intrinsic call has the ``nnan`` fast-math flag, then the operation can
+assume that NaNs are not present in the input vector.
Arguments:
""""""""""
@@ -22262,7 +22251,7 @@ This is an overloaded intrinsic.
Overview:
"""""""""
-Predicated floating-point IEEE-754-2008 minNum of two vectors of floating-point values.
+Predicated floating-point IEEE-754 minNum of two vectors of floating-point values.
Arguments:
@@ -22311,7 +22300,7 @@ This is an overloaded intrinsic.
Overview:
"""""""""
-Predicated floating-point IEEE-754-2008 maxNum of two vectors of floating-point values.
+Predicated floating-point IEEE-754 maxNum of two vectors of floating-point values.
Arguments:
@@ -23610,7 +23599,10 @@ result type. If only ``nnan`` is set then the neutral value is ``-Infinity``.
This instruction has the same comparison semantics as the
:ref:`llvm.vector.reduce.fmax <int_vector_reduce_fmax>` intrinsic (and thus the
-'``llvm.maxnum.*``' intrinsic).
+'``llvm.maxnum.*``' intrinsic). That is, the result will always be a number
+unless all elements of the vector and the starting value are ``NaN``. For a
+vector with maximum element magnitude ``0.0`` and containing both ``+0.0`` and
+``-0.0`` elements, the sign of the result is unspecified.
To ignore the start value, the neutral value can be used.
@@ -23677,7 +23669,10 @@ result type. If only ``nnan`` is set then the neutral value is ``+Infinity``.
This instruction has the same comparison semantics as the
:ref:`llvm.vector.reduce.fmin <int_vector_reduce_fmin>` intrinsic (and thus the
-'``llvm.minnum.*``' intrinsic).
+'``llvm.minnum.*``' intrinsic). That is, the result will always be a number
+unless all elements of the vector and the starting value are ``NaN``. For a
+vector with maximum element magnitude ``0.0`` and containing both ``+0.0`` and
+``-0.0`` elements, the sign of the result is unspecified.
To ignore the start value, the neutral value can be used.
@@ -28349,7 +28344,7 @@ The third argument specifies the exception behavior as described above.
Semantics:
""""""""""
-This function follows the IEEE-754-2008 semantics for maxNum.
+This function follows the IEEE-754 semantics for maxNum.
'``llvm.experimental.constrained.minnum``' Intrinsic
@@ -28381,7 +28376,7 @@ The third argument specifies the exception behavior as described above.
Semantics:
""""""""""
-This function follows the IEEE-754-2008 semantics for minNum.
+This function follows the IEEE-754 semantics for minNum.
'``llvm.experimental.constrained.maximum``' Intrinsic
diff --git a/llvm/include/llvm/CodeGen/ISDOpcodes.h b/llvm/include/llvm/CodeGen/ISDOpcodes.h
index 80ef32aff62ae..9bd1ebb2abd55 100644
--- a/llvm/include/llvm/CodeGen/ISDOpcodes.h
+++ b/llvm/include/llvm/CodeGen/ISDOpcodes.h
@@ -1024,20 +1024,13 @@ enum NodeType {
LRINT,
LLRINT,
- /// FMINNUM/FMAXNUM - Perform floating-point minimum maximum on two values,
- /// following IEEE-754 definitions except for signed zero behavior.
+ /// FMINNUM/FMAXNUM - Perform floating-point minimum or maximum on two
+ /// values.
///
- /// If one input is a signaling NaN, returns a quiet NaN. This matches
- /// IEEE-754 2008's minNum/maxNum behavior for signaling NaNs (which differs
- /// from 2019).
+ /// In the case where a single input is a NaN (either signaling or quiet),
+ /// the non-NaN input is returned.
///
- /// These treat -0 as ordered less than +0, matching the behavior of IEEE-754
- /// 2019's minimumNumber/maximumNumber.
- ///
- /// Note that that arithmetic on an sNaN doesn't consistently produce a qNaN,
- /// so arithmetic feeding into a minnum/maxnum can produce inconsistent
- /// results. FMAXIMUN/FMINIMUM or FMAXIMUMNUM/FMINIMUMNUM may be better choice
- /// for non-distinction of sNaN/qNaN handling.
+ /// The return value of (FMINNUM 0.0, -0.0) could be either 0.0 or -0.0.
FMINNUM,
FMAXNUM,
@@ -1051,9 +1044,6 @@ enum NodeType {
///
/// These treat -0 as ordered less than +0, matching the behavior of IEEE-754
/// 2019's minimumNumber/maximumNumber.
- ///
- /// Deprecated, and will be removed soon, as FMINNUM/FMAXNUM have the same
- /// semantics now.
FMINNUM_IEEE,
FMAXNUM_IEEE,
|
arsenm
left a comment
arsenm
left a comment
There was a problem hiding this comment.
The operation is fundamentally broken. You cannot handwave signaling nans away like other operations. We are not going to make forward progress unless we fix the definition. Signaling nans do not matter in the slightest in the real world. I do not consider any signaling validation failure to be an emergency worth reverting over
The operation as specified by IEEE-754-2008 is indeed fundamentally broken, which is why we did not use that definition...
This seems like an argument to do the revert and then leave it alone entirely? The whole change is exclusively about the behavior with signaling NaNs -- but unfortunately also affects the general optimization properties of the intrinsic (such as associativity). The impact on existing middle-end optimizations was not properly analyzed when the change was made, let alone actually implemented (in the multiple months since the LangRef patch landed). As seen in recent PRs and issues, the current state where LangRef and the middle-end do not agree is causing unnecessary confusion, which is why I think it is important to revert it now, before the situation becomes worse. We can still do the change, but it needs to be implemented in a more coordinated way, just like all other changes to IR semantics. (An alternative I would put up for consideration is to remove maxnum entirely, auto-upgrading the intrinsic to maximumnum nsz to match the previous semantics. Not sure anyone actually wants llvm.maxnum with the new semantics?) |
But we're still stuck dealing with it forever. It's functionally what most targets were codegenning it as.
I don't see a way this will ever not be confusing. The codegen never matched on most targets. If we at least have it documented how it should work, we can converge towards the end state.
Yes, I think it is what most people want but not because it makes any sense. This is the instruction that's been implemented in billions of devices. The new fminimumnum/fmaximumnum require wasting cycles for quieting in codegen, and it's still useful to have raw access to the underlying operation. We have very limited capacity to optimize out unnecessary quieting given the fuzzy snan semantics. We can't just look at the floating point producers and assume they will quiet. Since nobody actually cares about signaling nans, and do care about the best possible code, most use cases are better served by minnum/maxnum in practice. We have an AMDGPU only combine that tries to prune these out, but it doesn't work all that well due to the usual DAG reasons (and common usage of unanalyzable sources like function arguments and loads). We could do a little better if we strengthened the DAG/gMIR semantics to not permit dropping canonicalizes, but there's a cost to diverging from the IR rules |
It will always be somewhat confusing because the operations are confusing, but we don't have to add our own LLVM confusion on top, where LangRef and the middle-end don't match...
I do see your point. Another alternative, which is more consistent with LLVM's overall treatment of sNaN and also your "nobody actually cares about sNaN" perspective, is to specify that minnum/maxnum with an sNaN argument may non-deterministically convert it to a qNaN. This means we are allowed to lower minnum/maxnum to operations that either follow or don't follow the IEEE-754-2008 sNaN behavior. This would effectively specify the current backend behavior. I haven't carefully thought through what the implication of that for the middle-end would be -- I think we'd still lose associativity, but not some of the other things. |
That's my interpretation of what the (unclear) OpenCL spec rule (but not what the conformance test presently implements). One issue with fuzzing out the intrinsic like this is you now can't reliably implement minimumnum/maximumnum on top of minnum/maxnum. That's critical in the codegen versions of the intrinsics, but I could imagine wanting to perform the expansion in the IR for optimization. e.g. we could avoid canonicalizes somewhere important like in a loop by inserting explicit canonicalize at the unanalyzable source, and replacing the loop op with minnum |
|
Yes, currently it is not consistent between architectures, and I am working to fix them. |
fhahn
left a comment
fhahn
left a comment
There was a problem hiding this comment.
Yes, currently it is not consistent between architectures, and I am working to fix them. Since we cannot implement them in a single so huge patch, we need update the docs and then the code.
I might be missing something, but wouldn't it be an option to first add the new intrinsics, then work on fixing codegen for them on the major targets and then update LangRef for maxnum/minnum and change Clang's fmax/fmin to use the new intrinsics?
In any order, there will be some inconsistencies.
|
phoebewang
left a comment
phoebewang
left a comment
There was a problem hiding this comment.
I'm voting for this. If we have disputes for any additional changes, it's always good to revert to the initial states at least.
|
I am a hard no this. We've never been in a consistent state on this and this makes negative progress towards getting into a sensible state. Reverting the langref doesn't address any of the open bugs. TLDR:
|
Sorry, I don't agree. We are not just reverting the langref for bugs. The inconsistent state is exactly the reason we need to revert it. Can you describe what a sensible state would be? I don't see a consensus about it. Without that, we don't even know what is the right direction. My point is minnum / maxnum is one of floating-point math operations, applied to the general rules of |
|
This is absolutely required to fix #169122. One can't just change the LangRef if the new text contradicts existing practice in backends (and optimizations, see #138303). Also, #170067 re-introduces an odd case into the LLVM IR semantics where qNaN and sNaN have vastly different behavior:
Consequently, |
The behaviour of constant-folding maxnum(sNaN, x) and minnum(sNaN, x) has become controvertial, and there are ongoing discussions about which behaviour we want to specify in the LLVM IR LangRef. See: - llvm#170082 - llvm#168838 - llvm#138451 - llvm#170067 - https://discourse.llvm.org/t/rfc-a-consistent-set-of-semantics-for-the-floating-point-minimum-and-maximum-operations/89006 This patch removes optimizations and constant-folding support for maxnum(sNaN, x) but keeps it folded/optimized for qNaNs. This should allow for some more flexibility so the implementation can conform to either the old or new version of the semantics specified without any changes. As far as I am aware, optimizations involving constant sNaN should generally be edge-cases that rarely occur, so here should hopefully be very little real-world performance impact from disabling these optimizations.
The behaviour of constant-folding `maxnum(sNaN, x)` and `minnum(sNaN, x)` has become controversial, and there are ongoing discussions about which behaviour we want to specify in the LLVM IR LangRef. See: - #170082 - #168838 - #138451 - #170067 - https://discourse.llvm.org/t/rfc-a-consistent-set-of-semantics-for-the-floating-point-minimum-and-maximum-operations/89006 This patch removes optimizations and constant-folding support for `maxnum(sNaN, x)` but keeps it folded/optimized for `qNaN`. This should allow for some more flexibility so the implementation can conform to either the old or new version of the semantics specified without any changes. As far as I am aware, optimizations involving constant `sNaN` should generally be edge-cases that rarely occur, so here should hopefully be very little real-world performance impact from disabling these optimizations.
The behaviour of constant-folding `maxnum(sNaN, x)` and `minnum(sNaN, x)` has become controversial, and there are ongoing discussions about which behaviour we want to specify in the LLVM IR LangRef. See: - llvm/llvm-project#170082 - llvm/llvm-project#168838 - llvm/llvm-project#138451 - llvm/llvm-project#170067 - https://discourse.llvm.org/t/rfc-a-consistent-set-of-semantics-for-the-floating-point-minimum-and-maximum-operations/89006 This patch removes optimizations and constant-folding support for `maxnum(sNaN, x)` but keeps it folded/optimized for `qNaN`. This should allow for some more flexibility so the implementation can conform to either the old or new version of the semantics specified without any changes. As far as I am aware, optimizations involving constant `sNaN` should generally be edge-cases that rarely occur, so here should hopefully be very little real-world performance impact from disabling these optimizations.
The behaviour of constant-folding `maxnum(sNaN, x)` and `minnum(sNaN, x)` has become controversial, and there are ongoing discussions about which behaviour we want to specify in the LLVM IR LangRef. See: - llvm#170082 - llvm#168838 - llvm#138451 - llvm#170067 - https://discourse.llvm.org/t/rfc-a-consistent-set-of-semantics-for-the-floating-point-minimum-and-maximum-operations/89006 This patch removes optimizations and constant-folding support for `maxnum(sNaN, x)` but keeps it folded/optimized for `qNaN`. This should allow for some more flexibility so the implementation can conform to either the old or new version of the semantics specified without any changes. As far as I am aware, optimizations involving constant `sNaN` should generally be edge-cases that rarely occur, so here should hopefully be very little real-world performance impact from disabling these optimizations.
The behaviour of constant-folding `maxnum(sNaN, x)` and `minnum(sNaN, x)` has become controversial, and there are ongoing discussions about which behaviour we want to specify in the LLVM IR LangRef. See: - llvm#170082 - llvm#168838 - llvm#138451 - llvm#170067 - https://discourse.llvm.org/t/rfc-a-consistent-set-of-semantics-for-the-floating-point-minimum-and-maximum-operations/89006 This patch removes optimizations and constant-folding support for `maxnum(sNaN, x)` but keeps it folded/optimized for `qNaN`. This should allow for some more flexibility so the implementation can conform to either the old or new version of the semantics specified without any changes. As far as I am aware, optimizations involving constant `sNaN` should generally be edge-cases that rarely occur, so here should hopefully be very little real-world performance impact from disabling these optimizations.
7 participants
This reverts commit 363b059.
I'm not a big fan of the change itself (because it is incoherent with LLVM's general sNaN semantics), but even if we want to do it, it needs to be phased in a lot more carefully than what has actually happened. You can't just change the semantics in LangRef and then completely ignore the consequences of the change on optimization behavior.
Revert to the status quo for now. If we actually want to do this, we first need to ensure that the new minimumnum intrinsics have optimization and codegen parity with the old minnum, and ensure producers of the minnum intrinsics are migrated to them in advance (note that minnum is currently also considered canonical, so that would have to change as well), and then we need to land the LangRef changes as part of a patch stack that adjusts optimizations to respect the new semantics.
Fixes #138303.