riscv: fmax/fmin sNaN fix

riscv: fmax/fmin sNaN fix

[DJ Delorie]

RISC-V's FPU follows the IEEE spec, not the POSIX spec.  This patch
adds handling for sNaN cases where the two specs differ.

	* sysdeps/riscv/rvd/s_fmax.c (__fmax): Handle sNaNs correctly.
	* sysdeps/riscv/rvd/s_fmin.c (__fmin): Likewise.
	* sysdeps/riscv/rvf/s_fmaxf.c (__fmaxf): Likewise.
	* sysdeps/riscv/rvf/s_fminf.c (__fminf): Likewise.

diff --git a/sysdeps/riscv/rvd/s_fmax.c b/sysdeps/riscv/rvd/s_fmax.c
index ef8f1344ce..22e91bfc4b 100644
--- a/sysdeps/riscv/rvd/s_fmax.c
+++ b/sysdeps/riscv/rvd/s_fmax.c
@@ -17,12 +17,19 @@
    <http://www.gnu.org/licenses/>.  */
 
 #include <math.h>
+#include <math_private.h>
 #include <libm-alias-double.h>
 
 double
 __fmax (double x, double y)
 {
-  asm ("fmax.d %0, %1, %2" : "=f" (x) : "f" (x), "f" (y));
-  return x;
+  double res;
+
+  if (__glibc_unlikely ((_FCLASS (x) | _FCLASS (y)) & _FCLASS_SNAN))
+    return x + y;
+  else
+    asm ("fmax.d %0, %1, %2" : "=f" (res) : "f" (x), "f" (y));
+
+  return res;
 }
 libm_alias_double (__fmax, fmax)
diff --git a/sysdeps/riscv/rvd/s_fmin.c b/sysdeps/riscv/rvd/s_fmin.c
index c6ff24cefb..7b35230cac 100644
--- a/sysdeps/riscv/rvd/s_fmin.c
+++ b/sysdeps/riscv/rvd/s_fmin.c
@@ -17,12 +17,19 @@
    <http://www.gnu.org/licenses/>.  */
 
 #include <math.h>
+#include <math_private.h>
 #include <libm-alias-double.h>
 
 double
 __fmin (double x, double y)
 {
-  asm ("fmin.d %0, %1, %2" : "=f" (x) : "f" (x), "f" (y));
-  return x;
+  double res;
+
+  if (__glibc_unlikely ((_FCLASS (x) | _FCLASS (y)) & _FCLASS_SNAN))
+    return x + y;
+  else
+    asm ("fmin.d %0, %1, %2" : "=f" (res) : "f" (x), "f" (y));
+
+  return res;
 }
 libm_alias_double (__fmin, fmin)
diff --git a/sysdeps/riscv/rvf/s_fmaxf.c b/sysdeps/riscv/rvf/s_fmaxf.c
index 3293f2f41c..63f7e3d664 100644
--- a/sysdeps/riscv/rvf/s_fmaxf.c
+++ b/sysdeps/riscv/rvf/s_fmaxf.c
@@ -17,12 +17,19 @@
    <http://www.gnu.org/licenses/>.  */
 
 #include <math.h>
+#include <math_private.h>
 #include <libm-alias-float.h>
 
 float
 __fmaxf (float x, float y)
 {
-  asm ("fmax.s %0, %1, %2" : "=f" (x) : "f" (x), "f" (y));
-  return x;
+  float res;
+
+  if (__glibc_unlikely ((_FCLASS (x) | _FCLASS (y)) & _FCLASS_SNAN))
+    return x + y;
+  else
+    asm ("fmax.s %0, %1, %2" : "=f" (res) : "f" (x), "f" (y));
+
+  return res;
 }
 libm_alias_float (__fmax, fmax)
diff --git a/sysdeps/riscv/rvf/s_fminf.c b/sysdeps/riscv/rvf/s_fminf.c
index e4411f04b2..82cca4e37d 100644
--- a/sysdeps/riscv/rvf/s_fminf.c
+++ b/sysdeps/riscv/rvf/s_fminf.c
@@ -17,12 +17,19 @@
    <http://www.gnu.org/licenses/>.  */
 
 #include <math.h>
+#include <math_private.h>
 #include <libm-alias-float.h>
 
 float
 __fminf (float x, float y)
 {
-  asm ("fmin.s %0, %1, %2" : "=f" (x) : "f" (x), "f" (y));
-  return x;
+  float res;
+
+  if (__glibc_unlikely ((_FCLASS (x) | _FCLASS (y)) & _FCLASS_SNAN))
+    return x + y;
+  else
+    asm ("fmin.s %0, %1, %2" : "=f" (res) : "f" (x), "f" (y));
+
+  return res;
 }
 libm_alias_float (__fmin, fmin)

Re: riscv: fmax/fmin sNaN fix

[Szabolcs Nagy]

On 20/02/18 02:57, DJ Delorie wrote:
> 
> RISC-V's FPU follows the IEEE spec, not the POSIX spec.  This patch
                            ^^^^^^^^^
which one?
(the next ieee revision will have different min/max operations)

Re: riscv: fmax/fmin sNaN fix

[DJ Delorie]

Szabolcs Nagy <szabolcs.nagy@arm.com> writes:
> On 20/02/18 02:57, DJ Delorie wrote:
>> 
>> RISC-V's FPU follows the IEEE spec, not the POSIX spec.  This patch
>                             ^^^^^^^^^
> which one?
> (the next ieee revision will have different min/max operations)

The one that doesn't match POSIX :-)

The RISC-V fpu does this:

fmax (sNAN,4) -> 4

GLIBC expects this:

fmax (sNAN,4) -> qNAN

(hmm... maybe it follows posix, not ieee... whatever, it doesn't do what
glibc expects)

Note that the RISC-V ISA spec 2.2 documents the sNAN->qNAN behavior,
there's a patch for 2.3 that corrects it to match actual hardware.

Re: riscv: fmax/fmin sNaN fix

[DJ Delorie]

Szabolcs Nagy <szabolcs.nagy@arm.com> writes:
> On 20/02/18 02:57, DJ Delorie wrote:
>> 
>> RISC-V's FPU follows the IEEE spec, not the POSIX spec.  This patch
>                             ^^^^^^^^^
> which one?
> (the next ieee revision will have different min/max operations)

FYI origin commit is:
https://github.com/riscv/riscv-glibc/pull/44/commits/ea8c2f70ed9685f8c220e09b28098fefbc96179a

Re: riscv: fmax/fmin sNaN fix

[Richard Henderson]

On 02/20/2018 02:26 AM, Szabolcs Nagy wrote:
> On 20/02/18 02:57, DJ Delorie wrote:
>>
>> RISC-V's FPU follows the IEEE spec, not the POSIX spec.  This patch
>                            ^^^^^^^^^
> which one?
> (the next ieee revision will have different min/max operations)

The next (201x) one.

r~

Re: riscv: fmax/fmin sNaN fix

[Joseph Myers]

On Tue, 20 Feb 2018, Szabolcs Nagy wrote:

> On 20/02/18 02:57, DJ Delorie wrote:
> > 
> > RISC-V's FPU follows the IEEE spec, not the POSIX spec.  This patch
>                            ^^^^^^^^^
> which one?
> (the next ieee revision will have different min/max operations)

The point (as per 
<https://sourceware.org/ml/libc-alpha/2018-01/msg01084.html>) is that fmax 
and fmin in TS 18661-1 bind to maxNum and minNum.  Those operations are to 
be removed in IEEE 754-2018 so the C functions will have semantics that no 
longer have a corresponding IEEE operation (much like e.g. nextafter / 
nexttoward, which correspond to the Nextafter operation recommended in 
IEEE 754-1985 but removed in IEEE 754-2008).  Instead, the new minimum, 
minimumNumber, maximum and maximumNumber operations are proposed to have 
new functions fminimum, fminimum_num, fmaximum, fmaximum_num (and likewise 
*mag* functions) - but so far there is no public draft of those proposed 
TS changes (which might in any case more likely be dealt with in the C2x 
process rather than through a new revision of the TS being produced).

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: riscv: fmax/fmin sNaN fix

[Szabolcs Nagy]

On 20/02/18 17:22, DJ Delorie wrote:
> 
> Szabolcs Nagy <szabolcs.nagy@arm.com> writes:
>> On 20/02/18 02:57, DJ Delorie wrote:
>>>
>>> RISC-V's FPU follows the IEEE spec, not the POSIX spec.  This patch
>>                              ^^^^^^^^^
>> which one?
>> (the next ieee revision will have different min/max operations)
> 
> FYI origin commit is:
> https://github.com/riscv/riscv-glibc/pull/44/commits/ea8c2f70ed9685f8c220e09b28098fefbc96179a
> 

i just wanted to imply that the commit message as it is
is not useful (there is no such thing as IEEE fmin/fmax)

Re: riscv: fmax/fmin sNaN fix

[DJ Delorie]

Szabolcs Nagy <szabolcs.nagy@arm.com> writes:
> i just wanted to imply that the commit message as it is
> is not useful (there is no such thing as IEEE fmin/fmax)

I'll use a more functional commit message :-)

Re: riscv: fmax/fmin sNaN fix

[Palmer Dabbelt]

On Mon, 19 Feb 2018 18:57:49 PST (-0800), dj@redhat.com wrote:
>
> RISC-V's FPU follows the IEEE spec, not the POSIX spec.  This patch
> adds handling for sNaN cases where the two specs differ.
>
> 	* sysdeps/riscv/rvd/s_fmax.c (__fmax): Handle sNaNs correctly.
> 	* sysdeps/riscv/rvd/s_fmin.c (__fmin): Likewise.
> 	* sysdeps/riscv/rvf/s_fmaxf.c (__fmaxf): Likewise.
> 	* sysdeps/riscv/rvf/s_fminf.c (__fminf): Likewise.
>
> diff --git a/sysdeps/riscv/rvd/s_fmax.c b/sysdeps/riscv/rvd/s_fmax.c
> index ef8f1344ce..22e91bfc4b 100644
> --- a/sysdeps/riscv/rvd/s_fmax.c
> +++ b/sysdeps/riscv/rvd/s_fmax.c
> @@ -17,12 +17,19 @@
>     <http://www.gnu.org/licenses/>.  */
>
>  #include <math.h>
> +#include <math_private.h>
>  #include <libm-alias-double.h>
>
>  double
>  __fmax (double x, double y)
>  {
> -  asm ("fmax.d %0, %1, %2" : "=f" (x) : "f" (x), "f" (y));
> -  return x;
> +  double res;
> +
> +  if (__glibc_unlikely ((_FCLASS (x) | _FCLASS (y)) & _FCLASS_SNAN))
> +    return x + y;
> +  else
> +    asm ("fmax.d %0, %1, %2" : "=f" (res) : "f" (x), "f" (y));
> +
> +  return res;
>  }
>  libm_alias_double (__fmax, fmax)
> diff --git a/sysdeps/riscv/rvd/s_fmin.c b/sysdeps/riscv/rvd/s_fmin.c
> index c6ff24cefb..7b35230cac 100644
> --- a/sysdeps/riscv/rvd/s_fmin.c
> +++ b/sysdeps/riscv/rvd/s_fmin.c
> @@ -17,12 +17,19 @@
>     <http://www.gnu.org/licenses/>.  */
>
>  #include <math.h>
> +#include <math_private.h>
>  #include <libm-alias-double.h>
>
>  double
>  __fmin (double x, double y)
>  {
> -  asm ("fmin.d %0, %1, %2" : "=f" (x) : "f" (x), "f" (y));
> -  return x;
> +  double res;
> +
> +  if (__glibc_unlikely ((_FCLASS (x) | _FCLASS (y)) & _FCLASS_SNAN))
> +    return x + y;
> +  else
> +    asm ("fmin.d %0, %1, %2" : "=f" (res) : "f" (x), "f" (y));
> +
> +  return res;
>  }
>  libm_alias_double (__fmin, fmin)
> diff --git a/sysdeps/riscv/rvf/s_fmaxf.c b/sysdeps/riscv/rvf/s_fmaxf.c
> index 3293f2f41c..63f7e3d664 100644
> --- a/sysdeps/riscv/rvf/s_fmaxf.c
> +++ b/sysdeps/riscv/rvf/s_fmaxf.c
> @@ -17,12 +17,19 @@
>     <http://www.gnu.org/licenses/>.  */
>
>  #include <math.h>
> +#include <math_private.h>
>  #include <libm-alias-float.h>
>
>  float
>  __fmaxf (float x, float y)
>  {
> -  asm ("fmax.s %0, %1, %2" : "=f" (x) : "f" (x), "f" (y));
> -  return x;
> +  float res;
> +
> +  if (__glibc_unlikely ((_FCLASS (x) | _FCLASS (y)) & _FCLASS_SNAN))
> +    return x + y;
> +  else
> +    asm ("fmax.s %0, %1, %2" : "=f" (res) : "f" (x), "f" (y));
> +
> +  return res;
>  }
>  libm_alias_float (__fmax, fmax)
> diff --git a/sysdeps/riscv/rvf/s_fminf.c b/sysdeps/riscv/rvf/s_fminf.c
> index e4411f04b2..82cca4e37d 100644
> --- a/sysdeps/riscv/rvf/s_fminf.c
> +++ b/sysdeps/riscv/rvf/s_fminf.c
> @@ -17,12 +17,19 @@
>     <http://www.gnu.org/licenses/>.  */
>
>  #include <math.h>
> +#include <math_private.h>
>  #include <libm-alias-float.h>
>
>  float
>  __fminf (float x, float y)
>  {
> -  asm ("fmin.s %0, %1, %2" : "=f" (x) : "f" (x), "f" (y));
> -  return x;
> +  float res;
> +
> +  if (__glibc_unlikely ((_FCLASS (x) | _FCLASS (y)) & _FCLASS_SNAN))
> +    return x + y;
> +  else
> +    asm ("fmin.s %0, %1, %2" : "=f" (res) : "f" (x), "f" (y));
> +
> +  return res;
>  }
>  libm_alias_float (__fmin, fmin)

Also looks good, modulo the commit message and ChangeLog.  Thanks!

Re: riscv: fmax/fmin sNaN fix

[Carlos O'Donell]

On 02/21/2018 09:25 AM, DJ Delorie wrote:
> Szabolcs Nagy <szabolcs.nagy@arm.com> writes:
>> i just wanted to imply that the commit message as it is
>> is not useful (there is no such thing as IEEE fmin/fmax)
> 
> I'll use a more functional commit message :-)
 
Patch review includes review of the commit messages for factual accuracy.

-- 
Cheers,
Carlos.

Re: riscv: fmax/fmin sNaN fix

[DJ Delorie]

"Carlos O'Donell" <carlos@redhat.com> writes:
> Patch review includes review of the commit messages for factual accuracy.

Please review the original changelog entry then ;-)

>	* sysdeps/riscv/rvd/s_fmax.c (__fmax): Handle sNaNs correctly.
> 	* sysdeps/riscv/rvd/s_fmin.c (__fmin): Likewise.
> 	* sysdeps/riscv/rvf/s_fmaxf.c (__fmaxf): Likewise.
> 	* sysdeps/riscv/rvf/s_fminf.c (__fminf): Likewise.

That's actually functional, and doesn't mention anything about specs,
should be sufficient.

Original author: Andrew Waterman <andrew@sifive.com>

(plus or minus the "maintainers have automatic consensus" rules, of
course ;)

Re: riscv: fmax/fmin sNaN fix

[Carlos O'Donell]

On 02/21/2018 04:24 PM, DJ Delorie wrote:
> 
> "Carlos O'Donell" <carlos@redhat.com> writes:
>> Patch review includes review of the commit messages for factual accuracy.
> 
> Please review the original changelog entry then ;-)

The ChangeLog entry is not the same as the commit message.

A detailed explanation should become the body of the commit message for your patch.

>> 	* sysdeps/riscv/rvd/s_fmax.c (__fmax): Handle sNaNs correctly.
>> 	* sysdeps/riscv/rvd/s_fmin.c (__fmin): Likewise.
>> 	* sysdeps/riscv/rvf/s_fmaxf.c (__fmaxf): Likewise.
>> 	* sysdeps/riscv/rvf/s_fminf.c (__fminf): Likewise.
> 
> That's actually functional, and doesn't mention anything about specs,
> should be sufficient.

If there is a release out for RISC-V alrady, then this needs a bug number.

So a meaningful, but minimal commit message would be:
~~~
rsicv: Fix fmax/fmin sNaN issues (Bug XXXXX)

If any input to these functions is a sNaN then the result
should be sNaN, regardless of the input.

See discussions:
https://www.sourceware.org/ml/libc-alpha/2018-02/msg00529.html
~~~

Which is fine.

You also have assumed consensus as a machine maintainer :-)

-- 
Cheers,
Carlos.

Re: riscv: fmax/fmin sNaN fix

[DJ Delorie]

"Carlos O'Donell" <carlos@redhat.com> writes:
> If there is a release out for RISC-V alrady, then this needs a bug number.

If that's the rule, then *every* change needs a bug number, because
there's always a "release out" for some platform...

(distinct from "backports to release branches need a bug number, for
tracking", if that's a rule)

Re: riscv: fmax/fmin sNaN fix

[Joseph Myers]

On Thu, 22 Feb 2018, DJ Delorie wrote:

> "Carlos O'Donell" <carlos@redhat.com> writes:
> > If there is a release out for RISC-V alrady, then this needs a bug number.
> 
> If that's the rule, then *every* change needs a bug number, because
> there's always a "release out" for some platform...

It's for changes that fix a bug that was user-visible in a release.  (My 
interpretation of being user-visible in a release excludes bugs that 
caused the build to be broken, or bugs in the manual, or bugs in a 
testcase, or test failures such as check-localplt where a failure only 
indicates an optimization issue rather than an actual misbehavior in the 
installed glibc.)

New features and internal cleanups never need bugs filed in Bugzilla; 
likewise fixes for issues introduced on master after the last release, and 
for bugs in new features added since the last release.

We want the NEWS file to be as complete as possible regarding user-visible 
changes.  For new features and significant optimizations, entries are 
added to the relevant section of the NEWS file by hand (and should be 
included in the patches adding such features).  For miscellaneous 
bugfixes, the only reference in NEWS may be in the automatically-generated 
list of fixed bugs.  For that list to be as complete as possible, a bug 
needs filing in Bugzilla before being fixed, then marking as FIXED with 
target milestone set to the next release once the fix is checked in; that 
way, it will be automatically included in the list generated by 
scripts/list-fixed-bugs.py.

> (distinct from "backports to release branches need a bug number, for
> tracking", if that's a rule)

We don't have such a rule.  Backports to release branches should be posted 
to the libc-stable list.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: riscv: fmax/fmin sNaN fix

[Joseph Myers]

On Thu, 22 Feb 2018, Joseph Myers wrote:

> It's for changes that fix a bug that was user-visible in a release.  (My 
> interpretation of being user-visible in a release excludes bugs that 
> caused the build to be broken, or bugs in the manual, or bugs in a 
> testcase, or test failures such as check-localplt where a failure only 
> indicates an optimization issue rather than an actual misbehavior in the 
> installed glibc.)

Thus, this fmax/fmin fix should have a bug filed (and then marked as FIXED 
with milestone set).  But there is no need for a bug for the 
riscv/tls-macros.h change, because tls-macros.h is only used by testcases, 
so no user-visible bug in the installed glibc is being fixed by that 
patch.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: riscv: fmax/fmin sNaN fix

[DJ Delorie]

Joseph Myers <joseph@codesourcery.com> writes:
> Thus, this fmax/fmin fix should have a bug filed (and then marked as
> FIXED with milestone set).

Probably should have one for the ldconfig bug too, then.  Can a bug be
post-created and retroactively attached to the fix?

Re: riscv: fmax/fmin sNaN fix

[Joseph Myers]

On Fri, 23 Feb 2018, DJ Delorie wrote:

> Joseph Myers <joseph@codesourcery.com> writes:
> > Thus, this fmax/fmin fix should have a bug filed (and then marked as
> > FIXED with milestone set).
> 
> Probably should have one for the ldconfig bug too, then.  Can a bug be

Wasn't that bug 22827?

> post-created and retroactively attached to the fix?

File the bug, resolve it as FIXED with milestone set and a comment 
referencing the commit with the fix and the first release with that 
commit, optionally add the [BZ #N] notation to the ChangeLog entry for the 
fix.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: riscv: fmax/fmin sNaN fix

[DJ Delorie]

Joseph Myers <joseph@codesourcery.com> writes:
> Wasn't that bug 22827?

I am *so* going to replace this faulty memory one of these days...

> File the bug, resolve it as FIXED with milestone set and a comment
> referencing the commit with the fix and the first release with that
> commit, optionally add the [BZ #N] notation to the ChangeLog entry for
> the fix.

OK.