-
Notifications
You must be signed in to change notification settings - Fork 15.2k
Revert "LangRef: Clarify llvm.minnum and llvm.maxnum about sNaN and signed zero (#112852)" #168838
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
phoebewang
wants to merge
1
commit into
llvm:main
Choose a base branch
from
phoebewang:maxnum
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+54
−69
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…igned zero (llvm#112852)" This reverts commit 363b059. This is a follow up of llvm#166912. Sorry for not noticing the change at the beginning, but I disagree with both sNaN and signed zero semantics change. I have 3 justifications: - llvm.minnum and llvm.maxnum are common intrinsics, we cannot change the definition just because "some architectures" support the changed semantic. For example, X86 min/max instructions neither distinguish sNaN nor signed zero. We have to add couples of extra instructions to match with the new definition, which makes the intrinsics less efficient. But efficient is not the reason for the objection. I object because such cost is unnecessary; - As the example ``minnum(fadd(sNaN, -0.0), 1.0)`` shows, minnum/maxnum themself cannot guarantee consistent result if multiple FP arithmetic operations involved. It makes the sacrifice of performance totally unnecessary. `Behavior of Floating-Point NaN values` notes all NaNs can be treated as quiet NaNs unless using Constrained Floating-Point Intrinsics. So the cost is only worth for constrained minnum/maxnum ones if we want to define them; - Signed zero handling is unnecessary either, because even the C functions don't require it. If any other front ends require, they can use the existing fminnum_ieee/fmaxnum_ieee or define new intrinsics;
Member
|
@llvm/pr-subscribers-llvm-selectiondag @llvm/pr-subscribers-llvm-ir Author: Phoebe Wang (phoebewang) ChangesThis reverts commit 363b059. This is a follow up of #166912. Sorry for not noticing the change at the beginning, but I disagree with both sNaN and signed zero semantics change. I have 3 justifications:
Full diff: https://github.com/llvm/llvm-project/pull/168838.diff 2 Files Affected:
diff --git a/llvm/docs/LangRef.rst b/llvm/docs/LangRef.rst
index 734778f73af5f..d88846a416a52 100644
--- a/llvm/docs/LangRef.rst
+++ b/llvm/docs/LangRef.rst
@@ -17290,7 +17290,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)
@@ -17334,30 +17334,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 similar 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 implementation.
+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:
@@ -17394,30 +17385,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 similar 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 implementation.
+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:
@@ -20301,8 +20282,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:
""""""""""
@@ -20330,8 +20315,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:
""""""""""
@@ -22712,7 +22701,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:
@@ -22761,7 +22750,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:
@@ -24060,7 +24049,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.
@@ -24127,7 +24119,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.
@@ -28999,7 +28994,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
@@ -29031,7 +29026,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 cdaa916548c25..6b6bfd4a48fa2 100644
--- a/llvm/include/llvm/CodeGen/ISDOpcodes.h
+++ b/llvm/include/llvm/CodeGen/ISDOpcodes.h
@@ -1048,20 +1048,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,
@@ -1075,9 +1068,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,
|
🐧 Linux x64 Test Results
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This reverts commit 363b059.
This is a follow up of #166912. Sorry for not noticing the change at the beginning, but I disagree with both sNaN and signed zero semantics change.
I have 3 justifications:
minnum(fadd(sNaN, -0.0), 1.0)shows, minnum/maxnum themself cannot guarantee consistent result if multiple FP arithmetic operations involved. It makes the sacrifice of performance totally unnecessary.Behavior of Floating-Point NaN valuesnotes all NaNs can be treated as quiet NaNs unless using Constrained Floating-Point Intrinsics. So the cost is only worth for constrained minnum/maxnum ones if we want to define them;