public inbox for libc-alpha@sourceware.org
 help / color / mirror / Atom feed
* [PATCH 0/5] x86 pseudo-normal numbers
@ 2020-12-15 14:13 Siddhesh Poyarekar
  2020-12-15 14:13 ` [PATCH 1/5] x86 long double: Support pseudo numbers in fpclassifyl Siddhesh Poyarekar
                   ` (7 more replies)
  0 siblings, 8 replies; 25+ messages in thread
From: Siddhesh Poyarekar @ 2020-12-15 14:13 UTC (permalink / raw)
  To: libc-alpha; +Cc: joseph, carlos, fweimer

Hi,

Following is the patchset that harmonizes classification of
pseudo-normal numbers with gcc.  Pseudo-NaNs, Pseudo-Infinities and
unnormal numbers are considered as signaling NaN as per classification
since that is how they behave when used as operands in x86.

Pseudo-denormal numbers are not catered for in this patch.  The x86 CPU
supposedly treats these numbers as denormals, but both gcc and glibc
currently treat them as normals.

In summary, the patchset does the following:

- Update fpclassify to cater for pseudo-normal numbers
- Consolidate isnanl so that it can be used by isnanl as well as
  issignalingl.
- Update isnanl logic to return true for all pseudo-normal numbers
- Add an x86-specific issignalingl that returns true for all
  pseudo-normals.

Siddhesh Poyarekar (5):
  x86 long double: Support pseudo numbers in fpclassifyl
  x86 long double: Support pseudo numbers in isnanl
  Partially revert 681900d29683722b1cb0a8e565a0585846ec5a61
  x86 long double: Consider pseudo numbers as signaling
  x86 long double: Add tests for pseudo normal numbers

 sysdeps/i386/fpu/s_fpclassifyl.c         |   4 +
 sysdeps/i386/fpu/s_isnanl.c              |  10 +-
 sysdeps/ieee754/ldbl-96/s_issignalingl.c |   2 -
 sysdeps/x86/fpu/Makefile                 |   3 +-
 sysdeps/x86/fpu/isnanl_common.h          |  32 ++++
 sysdeps/x86/fpu/s_issignalingl.c         |  39 +++++
 sysdeps/x86/fpu/test-unnormal.c          | 196 +++++++++++++++++++++++
 sysdeps/x86/ldbl2mpn.c                   |   8 -
 8 files changed, 275 insertions(+), 19 deletions(-)
 create mode 100644 sysdeps/x86/fpu/isnanl_common.h
 create mode 100644 sysdeps/x86/fpu/s_issignalingl.c
 create mode 100644 sysdeps/x86/fpu/test-unnormal.c

-- 
2.29.2


^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH 1/5] x86 long double: Support pseudo numbers in fpclassifyl
  2020-12-15 14:13 [PATCH 0/5] x86 pseudo-normal numbers Siddhesh Poyarekar
@ 2020-12-15 14:13 ` Siddhesh Poyarekar
  2020-12-22 18:43   ` Adhemerval Zanella
  2020-12-15 14:13 ` [PATCH 2/5] x86 long double: Support pseudo numbers in isnanl Siddhesh Poyarekar
                   ` (6 subsequent siblings)
  7 siblings, 1 reply; 25+ messages in thread
From: Siddhesh Poyarekar @ 2020-12-15 14:13 UTC (permalink / raw)
  To: libc-alpha; +Cc: joseph, carlos, fweimer

---
 sysdeps/i386/fpu/s_fpclassifyl.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/sysdeps/i386/fpu/s_fpclassifyl.c b/sysdeps/i386/fpu/s_fpclassifyl.c
index 501312f51e..aeaec7279c 100644
--- a/sysdeps/i386/fpu/s_fpclassifyl.c
+++ b/sysdeps/i386/fpu/s_fpclassifyl.c
@@ -34,6 +34,10 @@ __fpclassifyl (long double x)
     retval = FP_ZERO;
   else if (ex == 0 && (hx & 0x80000000) == 0)
     retval = FP_SUBNORMAL;
+  /* Pseudo-normals, i.e. pseudo-zero, pseudo-infinity and unnormals.  They
+     behave like NaNs, so categorize them as such.  */
+  else if ((hx & 0x80000000) == 0)
+    retval = FP_NAN;
   else if (ex == 0x7fff)
     retval = ((hx & 0x7fffffff) | lx) != 0 ? FP_NAN : FP_INFINITE;
 
-- 
2.29.2


^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH 2/5] x86 long double: Support pseudo numbers in isnanl
  2020-12-15 14:13 [PATCH 0/5] x86 pseudo-normal numbers Siddhesh Poyarekar
  2020-12-15 14:13 ` [PATCH 1/5] x86 long double: Support pseudo numbers in fpclassifyl Siddhesh Poyarekar
@ 2020-12-15 14:13 ` Siddhesh Poyarekar
  2020-12-22 19:04   ` Adhemerval Zanella
  2020-12-15 14:13 ` [PATCH 3/5] Partially revert 681900d29683722b1cb0a8e565a0585846ec5a61 Siddhesh Poyarekar
                   ` (5 subsequent siblings)
  7 siblings, 1 reply; 25+ messages in thread
From: Siddhesh Poyarekar @ 2020-12-15 14:13 UTC (permalink / raw)
  To: libc-alpha; +Cc: joseph, carlos, fweimer

Sync up with gcc behaviour.  This change splits out the core isnanl
logic so that it can be reused.
---
 sysdeps/i386/fpu/s_isnanl.c     | 10 ++--------
 sysdeps/x86/fpu/isnanl_common.h | 32 ++++++++++++++++++++++++++++++++
 2 files changed, 34 insertions(+), 8 deletions(-)
 create mode 100644 sysdeps/x86/fpu/isnanl_common.h

diff --git a/sysdeps/i386/fpu/s_isnanl.c b/sysdeps/i386/fpu/s_isnanl.c
index fb97317bc9..6824f31d96 100644
--- a/sysdeps/i386/fpu/s_isnanl.c
+++ b/sysdeps/i386/fpu/s_isnanl.c
@@ -25,19 +25,13 @@ static char rcsid[] = "$NetBSD: $";
 
 #include <math.h>
 #include <math_private.h>
+#include "sysdeps/x86/fpu/isnanl_common.h"
 
 int __isnanl(long double x)
 {
 	int32_t se,hx,lx;
 	GET_LDOUBLE_WORDS(se,hx,lx,x);
-	se = (se & 0x7fff) << 1;
-	/* The additional & 0x7fffffff is required because Intel's
-	   extended format has the normally implicit 1 explicit
-	   present.  Sigh!  */
-	lx |= hx & 0x7fffffff;
-	se |= (uint32_t)(lx|(-lx))>>31;
-	se = 0xfffe - se;
-	return (int)((uint32_t)(se))>>16;
+	return x86_isnanl (se, hx, lx);
 }
 hidden_def (__isnanl)
 weak_alias (__isnanl, isnanl)
diff --git a/sysdeps/x86/fpu/isnanl_common.h b/sysdeps/x86/fpu/isnanl_common.h
new file mode 100644
index 0000000000..60a4af5b41
--- /dev/null
+++ b/sysdeps/x86/fpu/isnanl_common.h
@@ -0,0 +1,32 @@
+/* Common inline isnanl implementation for x86.
+   Copyright (C) 2020 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <https://www.gnu.org/licenses/>.  */
+
+static inline __always_inline int
+x86_isnanl (int32_t se, int32_t hx, int32_t lx)
+{
+  se = (se & 0x7fff) << 1;
+  /* Detect pseudo-normal numbers, i.e. exponent is non-zero and the top
+     bit of the significand is not set.   */
+  int pn = (uint32_t)((~hx & 0x80000000) & (se | (-se))) >> 31;
+  /* Clear the significand bit when computing mantissa.  */
+  lx |= hx & 0x7fffffff;
+  se |= (uint32_t)(lx | (-lx)) >> 31;
+  se = 0xfffe - se;
+
+  return (int)(((uint32_t)(se)) >> 16) | pn;
+}
-- 
2.29.2


^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH 3/5] Partially revert 681900d29683722b1cb0a8e565a0585846ec5a61
  2020-12-15 14:13 [PATCH 0/5] x86 pseudo-normal numbers Siddhesh Poyarekar
  2020-12-15 14:13 ` [PATCH 1/5] x86 long double: Support pseudo numbers in fpclassifyl Siddhesh Poyarekar
  2020-12-15 14:13 ` [PATCH 2/5] x86 long double: Support pseudo numbers in isnanl Siddhesh Poyarekar
@ 2020-12-15 14:13 ` Siddhesh Poyarekar
  2020-12-15 14:36   ` Florian Weimer
  2020-12-15 14:13 ` [PATCH 4/5] x86 long double: Consider pseudo numbers as signaling Siddhesh Poyarekar
                   ` (4 subsequent siblings)
  7 siblings, 1 reply; 25+ messages in thread
From: Siddhesh Poyarekar @ 2020-12-15 14:13 UTC (permalink / raw)
  To: libc-alpha; +Cc: joseph, carlos, fweimer

Do not attempt to fix the significand top bit in long double input
received in printf.  The code should never reach here because isnan
should now detect unnormals as NaN.  This is already a NOP for glibc
since it uses the gcc __builtin_isnan, which detects unnormals as NaN.
---
 sysdeps/x86/ldbl2mpn.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/sysdeps/x86/ldbl2mpn.c b/sysdeps/x86/ldbl2mpn.c
index 23afedfb67..ec8464eef7 100644
--- a/sysdeps/x86/ldbl2mpn.c
+++ b/sysdeps/x86/ldbl2mpn.c
@@ -115,14 +115,6 @@ __mpn_extract_long_double (mp_ptr res_ptr, mp_size_t size,
 	   && res_ptr[N - 1] == 0)
     /* Pseudo zero.  */
     *expt = 0;
-  else
-    /* Unlike other floating point formats, the most significant bit
-       is explicit and expected to be set for normal numbers.  Set it
-       in case it is cleared in the input.  Otherwise, callers will
-       not be able to produce the expected multi-precision integer
-       layout by shifting.  */
-    res_ptr[N - 1] |= (mp_limb_t) 1 << (LDBL_MANT_DIG - 1
-					- ((N - 1) * BITS_PER_MP_LIMB));
 
   return N;
 }
-- 
2.29.2


^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH 4/5] x86 long double: Consider pseudo numbers as signaling
  2020-12-15 14:13 [PATCH 0/5] x86 pseudo-normal numbers Siddhesh Poyarekar
                   ` (2 preceding siblings ...)
  2020-12-15 14:13 ` [PATCH 3/5] Partially revert 681900d29683722b1cb0a8e565a0585846ec5a61 Siddhesh Poyarekar
@ 2020-12-15 14:13 ` Siddhesh Poyarekar
  2020-12-22 20:13   ` Adhemerval Zanella
  2020-12-15 14:13 ` [PATCH 5/5] x86 long double: Add tests for pseudo normal numbers Siddhesh Poyarekar
                   ` (3 subsequent siblings)
  7 siblings, 1 reply; 25+ messages in thread
From: Siddhesh Poyarekar @ 2020-12-15 14:13 UTC (permalink / raw)
  To: libc-alpha; +Cc: joseph, carlos, fweimer

Implement a separate x86 version of issignalingl that returns true for
pseudo-normal numbers.

Also drop comment from the generic version of s_issignalingl since it
is no longer true or relevant.
---
 sysdeps/ieee754/ldbl-96/s_issignalingl.c |  2 --
 sysdeps/x86/fpu/s_issignalingl.c         | 39 ++++++++++++++++++++++++
 2 files changed, 39 insertions(+), 2 deletions(-)
 create mode 100644 sysdeps/x86/fpu/s_issignalingl.c

diff --git a/sysdeps/ieee754/ldbl-96/s_issignalingl.c b/sysdeps/ieee754/ldbl-96/s_issignalingl.c
index ec542ad468..2aa0ffae0e 100644
--- a/sysdeps/ieee754/ldbl-96/s_issignalingl.c
+++ b/sysdeps/ieee754/ldbl-96/s_issignalingl.c
@@ -34,8 +34,6 @@ __issignalingl (long double x)
   hxi ^= 0x40000000;
   /* If lxi != 0, then set any suitable bit of the significand in hxi.  */
   hxi |= (lxi | -lxi) >> 31;
-  /* We do not recognize a pseudo NaN as sNaN; they're invalid on 80387 and
-     later.  */
   /* We have to compare for greater (instead of greater or equal), because x's
      significand being all-zero designates infinity not NaN.  */
   return ((exi & 0x7fff) == 0x7fff) && (hxi > 0xc0000000);
diff --git a/sysdeps/x86/fpu/s_issignalingl.c b/sysdeps/x86/fpu/s_issignalingl.c
new file mode 100644
index 0000000000..df273b881d
--- /dev/null
+++ b/sysdeps/x86/fpu/s_issignalingl.c
@@ -0,0 +1,39 @@
+/* Test for signaling NaN - x86 version.
+   Copyright (C) 2020 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <https://www.gnu.org/licenses/>.  */
+
+#include <math.h>
+#include <math_private.h>
+#include <nan-high-order-bit.h>
+#include "sysdeps/x86/fpu/isnanl_common.h"
+
+int
+__issignalingl (long double x)
+{
+  uint32_t exi, hxi, lxi, nan, pn;
+  GET_LDOUBLE_WORDS (exi, hxi, lxi, x);
+#if HIGH_ORDER_BIT_IS_SET_FOR_SNAN
+# error not implemented
+#else
+  nan = x86_isnanl (exi, hxi, lxi);
+  pn = (~hxi & 0x80000000) >> 31;
+  hxi = (~hxi & 0x40000000) >> 30;
+
+  return nan & (pn | hxi);
+#endif
+}
+libm_hidden_def (__issignalingl)
-- 
2.29.2


^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH 5/5] x86 long double: Add tests for pseudo normal numbers
  2020-12-15 14:13 [PATCH 0/5] x86 pseudo-normal numbers Siddhesh Poyarekar
                   ` (3 preceding siblings ...)
  2020-12-15 14:13 ` [PATCH 4/5] x86 long double: Consider pseudo numbers as signaling Siddhesh Poyarekar
@ 2020-12-15 14:13 ` Siddhesh Poyarekar
  2020-12-22 21:48   ` Adhemerval Zanella
  2020-12-15 18:26 ` [PATCH 0/5] x86 pseudo-normal numbers Joseph Myers
                   ` (2 subsequent siblings)
  7 siblings, 1 reply; 25+ messages in thread
From: Siddhesh Poyarekar @ 2020-12-15 14:13 UTC (permalink / raw)
  To: libc-alpha; +Cc: joseph, carlos, fweimer

Add some tests for fpclassify, isnanl, isinfl and issignaling.
---
 sysdeps/x86/fpu/Makefile        |   3 +-
 sysdeps/x86/fpu/test-unnormal.c | 196 ++++++++++++++++++++++++++++++++
 2 files changed, 198 insertions(+), 1 deletion(-)
 create mode 100644 sysdeps/x86/fpu/test-unnormal.c

diff --git a/sysdeps/x86/fpu/Makefile b/sysdeps/x86/fpu/Makefile
index 600e42c3db..e77de56d14 100644
--- a/sysdeps/x86/fpu/Makefile
+++ b/sysdeps/x86/fpu/Makefile
@@ -4,11 +4,12 @@ CPPFLAGS += -I../soft-fp
 
 libm-support += powl_helper
 tests += test-fenv-sse test-fenv-clear-sse test-fenv-x87 test-fenv-sse-2 \
-	 test-flt-eval-method-387 test-flt-eval-method-sse
+	 test-flt-eval-method-387 test-flt-eval-method-sse test-unnormal
 CFLAGS-test-fenv-sse.c += -msse2 -mfpmath=sse
 CFLAGS-test-fenv-clear-sse.c += -msse2 -mfpmath=sse
 CFLAGS-test-fenv-sse-2.c += -msse2 -mfpmath=sse
 CFLAGS-test-flt-eval-method-387.c += -fexcess-precision=standard -mfpmath=387
 CFLAGS-test-flt-eval-method-sse.c += -fexcess-precision=standard -msse2 \
 				     -mfpmath=sse
+CFLAGS-test-unnormal.c += -fsignaling-nans -std=c2x
 endif
diff --git a/sysdeps/x86/fpu/test-unnormal.c b/sysdeps/x86/fpu/test-unnormal.c
new file mode 100644
index 0000000000..fc65d9290f
--- /dev/null
+++ b/sysdeps/x86/fpu/test-unnormal.c
@@ -0,0 +1,196 @@
+/* Test long double classification with x86 pseudo normal numbers.
+   Copyright (C) 2020 Free Software Foundation, Inc.
+
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <https://www.gnu.org/licenses/>.  */
+
+#include <math.h>
+#include <stdio.h>
+#include <string.h>
+
+struct tests
+{
+  const char *val;
+  int class;
+} inputs[] = {
+      /* Normal.  */
+      {"\x00\x04\x00\x00\x00\x00\x00\x00\x00\x04", FP_NAN},
+      {"\x00\x04\x00\x00\x00\x00\x00\xf0\x00\x04", FP_NORMAL},
+      /* Pseudo-infinite.  */
+      {"\x00\x00\x00\x00\x00\x00\x00\x00\xff\x7f", FP_NAN},
+      {"\x00\x00\x00\x00\x00\x00\x00\x80\xff\x7f", FP_INFINITE},
+      /* Pseudo-zero.  */
+      {"\x00\x00\x00\x00\x00\x00\x00\x00\x00\x01", FP_NAN},
+      {"\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00", FP_ZERO},
+};
+
+const char *classes[5];
+#define stringify(N) #N
+
+static void
+initialize (void)
+{
+  classes[FP_NAN] = stringify(FP_NAN);
+  classes[FP_INFINITE] = stringify(FP_INFINITY);
+  classes[FP_ZERO] = stringify(FP_ZERO);
+  classes[FP_SUBNORMAL] = stringify(FP_SUBNORMAL);
+  classes[FP_NORMAL] = stringify(FP_NORMAL);
+}
+
+static void
+unnormal_str (const char *val, char *ret)
+{
+  for (int i = 9; i >= 0; i--)
+    {
+      if (i == 7 || i == 3)
+	*ret++ = ' ';
+      snprintf(ret, 3, "%02x", (unsigned char) val[i]);
+      ret += 2;
+    }
+}
+
+static int
+test_fpclassify (void)
+{
+  int ret = 0;
+
+  printf ("* fpclassify tests:\n");
+  for (int i = 0; i < sizeof (inputs)/sizeof (struct tests); i++)
+    {
+      long double value;
+      char buf[22];
+
+      memcpy (&value, inputs[i].val, 10);
+      unnormal_str(inputs[i].val, buf);
+      int class = fpclassify(value);
+
+      if (class != inputs[i].class)
+	{
+	  printf ("0x%s: got %s, expected %s\n", buf,
+		  classes[fpclassify(value)],
+		  classes[inputs[i].class]);
+	  ret |= 1;
+	}
+      else
+	printf ("0x%s: OK\n", buf);
+    }
+  return ret;
+}
+
+static int
+test_isinf (void)
+{
+  int ret = 0;
+
+  printf ("* isinfl tests:\n");
+  for (int i = 0; i < sizeof (inputs)/sizeof (struct tests); i++)
+    {
+      long double value;
+      char buf[22];
+
+      memcpy (&value, inputs[i].val, 10);
+      unnormal_str(inputs[i].val, buf);
+      int inf = isinf (value);
+
+      if ((inputs[i].class == FP_INFINITE && inf)
+	  || (inputs[i].class != FP_INFINITE && !inf))
+	printf ("0x%s: OK\n", buf);
+      else
+	{
+	  printf ("0x%s: got %s, expected %s\n", buf,
+		  inf ? "INFINITE" : "NOT INFINITE",
+		  classes[inputs[i].class]);
+	  ret |= 1;
+	}
+    }
+
+  return ret;
+}
+
+static int
+test_isnan (void)
+{
+  int ret = 0;
+
+  printf ("* isnanl tests:\n");
+  for (int i = 0; i < sizeof (inputs)/sizeof (struct tests); i++)
+    {
+      long double value;
+      char buf[22];
+
+      memcpy (&value, inputs[i].val, 10);
+      unnormal_str(inputs[i].val, buf);
+      int nan = isnan (value);
+
+      if ((inputs[i].class == FP_NAN && nan)
+	  || (inputs[i].class != FP_NAN && !nan))
+	printf ("0x%s: OK\n", buf);
+      else
+	{
+	  printf ("0x%s: got %s, expected %s\n", buf,
+		  nan ? "NAN" : "NOT NAN",
+		  classes[inputs[i].class]);
+	  ret |= 1;
+	}
+    }
+  return ret;
+}
+
+static int
+test_issignaling (void)
+{
+  int ret = 0;
+
+  printf ("* issignaling tests:\n");
+  for (int i = 0; i < sizeof (inputs)/sizeof (struct tests); i++)
+    {
+      long double value;
+      char buf[22];
+
+      memcpy (&value, inputs[i].val, 10);
+      unnormal_str(inputs[i].val, buf);
+      int signaling = issignaling (value);
+
+      if ((inputs[i].class == FP_NAN && signaling)
+	  || (inputs[i].class != FP_NAN && !signaling))
+	printf ("0x%s: OK\n", buf);
+      else
+	{
+	  printf ("0x%s: got %s, expected %s\n", buf,
+		  signaling ? "SIGNALING" : "NOT SIGNALING",
+		  classes[inputs[i].class]);
+	  ret |= 1;
+	}
+    }
+  return ret;
+}
+
+int
+do_test (void)
+{
+  int ret = 0;
+
+  initialize ();
+
+  ret |= test_fpclassify ();
+  ret |= test_isinf ();
+  ret |= test_isnan ();
+  ret |= test_issignaling ();
+
+  return ret;
+}
+
+#include <support/test-driver.c>
-- 
2.29.2


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 3/5] Partially revert 681900d29683722b1cb0a8e565a0585846ec5a61
  2020-12-15 14:13 ` [PATCH 3/5] Partially revert 681900d29683722b1cb0a8e565a0585846ec5a61 Siddhesh Poyarekar
@ 2020-12-15 14:36   ` Florian Weimer
  2020-12-15 15:01     ` Siddhesh Poyarekar
  0 siblings, 1 reply; 25+ messages in thread
From: Florian Weimer @ 2020-12-15 14:36 UTC (permalink / raw)
  To: Siddhesh Poyarekar; +Cc: libc-alpha, joseph, carlos

* Siddhesh Poyarekar:

> Do not attempt to fix the significand top bit in long double input
> received in printf.  The code should never reach here because isnan
> should now detect unnormals as NaN.  This is already a NOP for glibc
> since it uses the gcc __builtin_isnan, which detects unnormals as NaN.
> ---
>  sysdeps/x86/ldbl2mpn.c | 8 --------
>  1 file changed, 8 deletions(-)
>
> diff --git a/sysdeps/x86/ldbl2mpn.c b/sysdeps/x86/ldbl2mpn.c
> index 23afedfb67..ec8464eef7 100644
> --- a/sysdeps/x86/ldbl2mpn.c
> +++ b/sysdeps/x86/ldbl2mpn.c
> @@ -115,14 +115,6 @@ __mpn_extract_long_double (mp_ptr res_ptr, mp_size_t size,
>  	   && res_ptr[N - 1] == 0)
>      /* Pseudo zero.  */
>      *expt = 0;
> -  else
> -    /* Unlike other floating point formats, the most significant bit
> -       is explicit and expected to be set for normal numbers.  Set it
> -       in case it is cleared in the input.  Otherwise, callers will
> -       not be able to produce the expected multi-precision integer
> -       layout by shifting.  */
> -    res_ptr[N - 1] |= (mp_limb_t) 1 << (LDBL_MANT_DIG - 1
> -					- ((N - 1) * BITS_PER_MP_LIMB));
>  
>    return N;
>  }

I think you should also tweak the test in
sysdeps/x86/tst-ldbl-nonnormal-printf.c to expect "nan" only.

Thanks,
Florian
-- 
Red Hat GmbH, https://de.redhat.com/ , Registered seat: Grasbrunn,
Commercial register: Amtsgericht Muenchen, HRB 153243,
Managing Directors: Charles Cachera, Brian Klemm, Laurie Krebs, Michael O'Neill


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 3/5] Partially revert 681900d29683722b1cb0a8e565a0585846ec5a61
  2020-12-15 14:36   ` Florian Weimer
@ 2020-12-15 15:01     ` Siddhesh Poyarekar
  0 siblings, 0 replies; 25+ messages in thread
From: Siddhesh Poyarekar @ 2020-12-15 15:01 UTC (permalink / raw)
  To: Florian Weimer; +Cc: libc-alpha, joseph

On 12/15/20 8:06 PM, Florian Weimer via Libc-alpha wrote:

> I think you should also tweak the test in
> sysdeps/x86/tst-ldbl-nonnormal-printf.c to expect "nan" only.

Thanks for catching that.  I'll fix it up.

Siddhesh

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 0/5] x86 pseudo-normal numbers
  2020-12-15 14:13 [PATCH 0/5] x86 pseudo-normal numbers Siddhesh Poyarekar
                   ` (4 preceding siblings ...)
  2020-12-15 14:13 ` [PATCH 5/5] x86 long double: Add tests for pseudo normal numbers Siddhesh Poyarekar
@ 2020-12-15 18:26 ` Joseph Myers
  2020-12-15 18:29   ` Siddhesh Poyarekar
  2020-12-18  4:03 ` [PING][PATCH " Siddhesh Poyarekar
  2020-12-22  7:46 ` [PING*2][PATCH " Siddhesh Poyarekar
  7 siblings, 1 reply; 25+ messages in thread
From: Joseph Myers @ 2020-12-15 18:26 UTC (permalink / raw)
  To: Siddhesh Poyarekar; +Cc: libc-alpha, fweimer

On Tue, 15 Dec 2020, Siddhesh Poyarekar via Libc-alpha wrote:

> Pseudo-denormal numbers are not catered for in this patch.  The x86 CPU
> supposedly treats these numbers as denormals, but both gcc and glibc
> currently treat them as normals.

The CPU treats them as denormals in that it signals the (x86-specific) 
"denormal operand" exception for them, but otherwise treats them as if the 
biased exponent were 1.  Since the "denormal operand" exception is not 
part of ISO C or FE_ALL_EXCEPT and glibc does not attempt to produce any 
particular semantics for when it is raised, making glibc handle them the 
same as the processor does means handling them the same as the 
corresponding normal representations.

-- 
Joseph S. Myers
joseph@codesourcery.com

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 0/5] x86 pseudo-normal numbers
  2020-12-15 18:26 ` [PATCH 0/5] x86 pseudo-normal numbers Joseph Myers
@ 2020-12-15 18:29   ` Siddhesh Poyarekar
  0 siblings, 0 replies; 25+ messages in thread
From: Siddhesh Poyarekar @ 2020-12-15 18:29 UTC (permalink / raw)
  To: Joseph Myers; +Cc: fweimer, libc-alpha

On 12/15/20 11:56 PM, Joseph Myers wrote:
> On Tue, 15 Dec 2020, Siddhesh Poyarekar via Libc-alpha wrote:
> 
>> Pseudo-denormal numbers are not catered for in this patch.  The x86 CPU
>> supposedly treats these numbers as denormals, but both gcc and glibc
>> currently treat them as normals.
> 
> The CPU treats them as denormals in that it signals the (x86-specific)
> "denormal operand" exception for them, but otherwise treats them as if the
> biased exponent were 1.  Since the "denormal operand" exception is not
> part of ISO C or FE_ALL_EXCEPT and glibc does not attempt to produce any
> particular semantics for when it is raised, making glibc handle them the
> same as the processor does means handling them the same as the
> corresponding normal representations.

That's perfect, I'll not bother with them anymore then.

Thanks,
Siddhesh

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PING][PATCH 0/5] x86 pseudo-normal numbers
  2020-12-15 14:13 [PATCH 0/5] x86 pseudo-normal numbers Siddhesh Poyarekar
                   ` (5 preceding siblings ...)
  2020-12-15 18:26 ` [PATCH 0/5] x86 pseudo-normal numbers Joseph Myers
@ 2020-12-18  4:03 ` Siddhesh Poyarekar
  2020-12-22  7:46 ` [PING*2][PATCH " Siddhesh Poyarekar
  7 siblings, 0 replies; 25+ messages in thread
From: Siddhesh Poyarekar @ 2020-12-18  4:03 UTC (permalink / raw)
  To: Siddhesh Poyarekar, libc-alpha; +Cc: fweimer, joseph

Ping!  I have made the changes to 3/5 and am waiting for reviews for the 
rest of the patches before posting a v2, in case more changes are 
needed.  It would be really nice to get this in before the freeze deadline.

Siddhesh

On 12/15/20 7:43 PM, Siddhesh Poyarekar via Libc-alpha wrote:
> Hi,
> 
> Following is the patchset that harmonizes classification of
> pseudo-normal numbers with gcc.  Pseudo-NaNs, Pseudo-Infinities and
> unnormal numbers are considered as signaling NaN as per classification
> since that is how they behave when used as operands in x86.
> 
> Pseudo-denormal numbers are not catered for in this patch.  The x86 CPU
> supposedly treats these numbers as denormals, but both gcc and glibc
> currently treat them as normals.
> 
> In summary, the patchset does the following:
> 
> - Update fpclassify to cater for pseudo-normal numbers
> - Consolidate isnanl so that it can be used by isnanl as well as
>    issignalingl.
> - Update isnanl logic to return true for all pseudo-normal numbers
> - Add an x86-specific issignalingl that returns true for all
>    pseudo-normals.
> 
> Siddhesh Poyarekar (5):
>    x86 long double: Support pseudo numbers in fpclassifyl
>    x86 long double: Support pseudo numbers in isnanl
>    Partially revert 681900d29683722b1cb0a8e565a0585846ec5a61
>    x86 long double: Consider pseudo numbers as signaling
>    x86 long double: Add tests for pseudo normal numbers
> 
>   sysdeps/i386/fpu/s_fpclassifyl.c         |   4 +
>   sysdeps/i386/fpu/s_isnanl.c              |  10 +-
>   sysdeps/ieee754/ldbl-96/s_issignalingl.c |   2 -
>   sysdeps/x86/fpu/Makefile                 |   3 +-
>   sysdeps/x86/fpu/isnanl_common.h          |  32 ++++
>   sysdeps/x86/fpu/s_issignalingl.c         |  39 +++++
>   sysdeps/x86/fpu/test-unnormal.c          | 196 +++++++++++++++++++++++
>   sysdeps/x86/ldbl2mpn.c                   |   8 -
>   8 files changed, 275 insertions(+), 19 deletions(-)
>   create mode 100644 sysdeps/x86/fpu/isnanl_common.h
>   create mode 100644 sysdeps/x86/fpu/s_issignalingl.c
>   create mode 100644 sysdeps/x86/fpu/test-unnormal.c
> 


^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PING*2][PATCH 0/5] x86 pseudo-normal numbers
  2020-12-15 14:13 [PATCH 0/5] x86 pseudo-normal numbers Siddhesh Poyarekar
                   ` (6 preceding siblings ...)
  2020-12-18  4:03 ` [PING][PATCH " Siddhesh Poyarekar
@ 2020-12-22  7:46 ` Siddhesh Poyarekar
  2020-12-22 11:11   ` Adhemerval Zanella
  7 siblings, 1 reply; 25+ messages in thread
From: Siddhesh Poyarekar @ 2020-12-22  7:46 UTC (permalink / raw)
  To: libc-alpha; +Cc: fweimer, joseph

Ping!


On 12/15/20 7:43 PM, Siddhesh Poyarekar via Libc-alpha wrote:
> Hi,
> 
> Following is the patchset that harmonizes classification of
> pseudo-normal numbers with gcc.  Pseudo-NaNs, Pseudo-Infinities and
> unnormal numbers are considered as signaling NaN as per classification
> since that is how they behave when used as operands in x86.
> 
> Pseudo-denormal numbers are not catered for in this patch.  The x86 CPU
> supposedly treats these numbers as denormals, but both gcc and glibc
> currently treat them as normals.
> 
> In summary, the patchset does the following:
> 
> - Update fpclassify to cater for pseudo-normal numbers
> - Consolidate isnanl so that it can be used by isnanl as well as
>    issignalingl.
> - Update isnanl logic to return true for all pseudo-normal numbers
> - Add an x86-specific issignalingl that returns true for all
>    pseudo-normals.
> 
> Siddhesh Poyarekar (5):
>    x86 long double: Support pseudo numbers in fpclassifyl
>    x86 long double: Support pseudo numbers in isnanl
>    Partially revert 681900d29683722b1cb0a8e565a0585846ec5a61
>    x86 long double: Consider pseudo numbers as signaling
>    x86 long double: Add tests for pseudo normal numbers
> 
>   sysdeps/i386/fpu/s_fpclassifyl.c         |   4 +
>   sysdeps/i386/fpu/s_isnanl.c              |  10 +-
>   sysdeps/ieee754/ldbl-96/s_issignalingl.c |   2 -
>   sysdeps/x86/fpu/Makefile                 |   3 +-
>   sysdeps/x86/fpu/isnanl_common.h          |  32 ++++
>   sysdeps/x86/fpu/s_issignalingl.c         |  39 +++++
>   sysdeps/x86/fpu/test-unnormal.c          | 196 +++++++++++++++++++++++
>   sysdeps/x86/ldbl2mpn.c                   |   8 -
>   8 files changed, 275 insertions(+), 19 deletions(-)
>   create mode 100644 sysdeps/x86/fpu/isnanl_common.h
>   create mode 100644 sysdeps/x86/fpu/s_issignalingl.c
>   create mode 100644 sysdeps/x86/fpu/test-unnormal.c
> 


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PING*2][PATCH 0/5] x86 pseudo-normal numbers
  2020-12-22  7:46 ` [PING*2][PATCH " Siddhesh Poyarekar
@ 2020-12-22 11:11   ` Adhemerval Zanella
  0 siblings, 0 replies; 25+ messages in thread
From: Adhemerval Zanella @ 2020-12-22 11:11 UTC (permalink / raw)
  To: libc-alpha

I will review the set this week.

On 22/12/2020 04:46, Siddhesh Poyarekar via Libc-alpha wrote:
> Ping!
> 
> 
> On 12/15/20 7:43 PM, Siddhesh Poyarekar via Libc-alpha wrote:
>> Hi,
>>
>> Following is the patchset that harmonizes classification of
>> pseudo-normal numbers with gcc.  Pseudo-NaNs, Pseudo-Infinities and
>> unnormal numbers are considered as signaling NaN as per classification
>> since that is how they behave when used as operands in x86.
>>
>> Pseudo-denormal numbers are not catered for in this patch.  The x86 CPU
>> supposedly treats these numbers as denormals, but both gcc and glibc
>> currently treat them as normals.
>>
>> In summary, the patchset does the following:
>>
>> - Update fpclassify to cater for pseudo-normal numbers
>> - Consolidate isnanl so that it can be used by isnanl as well as
>>    issignalingl.
>> - Update isnanl logic to return true for all pseudo-normal numbers
>> - Add an x86-specific issignalingl that returns true for all
>>    pseudo-normals.
>>
>> Siddhesh Poyarekar (5):
>>    x86 long double: Support pseudo numbers in fpclassifyl
>>    x86 long double: Support pseudo numbers in isnanl
>>    Partially revert 681900d29683722b1cb0a8e565a0585846ec5a61
>>    x86 long double: Consider pseudo numbers as signaling
>>    x86 long double: Add tests for pseudo normal numbers
>>
>>   sysdeps/i386/fpu/s_fpclassifyl.c         |   4 +
>>   sysdeps/i386/fpu/s_isnanl.c              |  10 +-
>>   sysdeps/ieee754/ldbl-96/s_issignalingl.c |   2 -
>>   sysdeps/x86/fpu/Makefile                 |   3 +-
>>   sysdeps/x86/fpu/isnanl_common.h          |  32 ++++
>>   sysdeps/x86/fpu/s_issignalingl.c         |  39 +++++
>>   sysdeps/x86/fpu/test-unnormal.c          | 196 +++++++++++++++++++++++
>>   sysdeps/x86/ldbl2mpn.c                   |   8 -
>>   8 files changed, 275 insertions(+), 19 deletions(-)
>>   create mode 100644 sysdeps/x86/fpu/isnanl_common.h
>>   create mode 100644 sysdeps/x86/fpu/s_issignalingl.c
>>   create mode 100644 sysdeps/x86/fpu/test-unnormal.c
>>
> 

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 1/5] x86 long double: Support pseudo numbers in fpclassifyl
  2020-12-15 14:13 ` [PATCH 1/5] x86 long double: Support pseudo numbers in fpclassifyl Siddhesh Poyarekar
@ 2020-12-22 18:43   ` Adhemerval Zanella
  2020-12-23  1:43     ` Siddhesh Poyarekar
  0 siblings, 1 reply; 25+ messages in thread
From: Adhemerval Zanella @ 2020-12-22 18:43 UTC (permalink / raw)
  To: libc-alpha, Siddhesh Poyarekar; +Cc: Florian Weimer, Joseph Myers



On 15/12/2020 11:13, Siddhesh Poyarekar via Libc-alpha wrote:
> ---
>  sysdeps/i386/fpu/s_fpclassifyl.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/sysdeps/i386/fpu/s_fpclassifyl.c b/sysdeps/i386/fpu/s_fpclassifyl.c
> index 501312f51e..aeaec7279c 100644
> --- a/sysdeps/i386/fpu/s_fpclassifyl.c
> +++ b/sysdeps/i386/fpu/s_fpclassifyl.c
> @@ -34,6 +34,10 @@ __fpclassifyl (long double x)
>      retval = FP_ZERO;
>    else if (ex == 0 && (hx & 0x80000000) == 0)
>      retval = FP_SUBNORMAL;
> +  /* Pseudo-normals, i.e. pseudo-zero, pseudo-infinity and unnormals.  They

Intel manual adds a hyphen for un-normals.

> +     behave like NaNs, so categorize them as such.  */
> +  else if ((hx & 0x80000000) == 0)
> +    retval = FP_NAN;

Ok.

>    else if (ex == 0x7fff)
>      retval = ((hx & 0x7fffffff) | lx) != 0 ? FP_NAN : FP_INFINITE;
>  
> 

Maybe move the implementation to sysdep/x86/fpu since it is used for
both x86_64 and i386?  It avoid the implicit cross abi include the
and make the implementation selection more clear.

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 2/5] x86 long double: Support pseudo numbers in isnanl
  2020-12-15 14:13 ` [PATCH 2/5] x86 long double: Support pseudo numbers in isnanl Siddhesh Poyarekar
@ 2020-12-22 19:04   ` Adhemerval Zanella
  2020-12-23  1:49     ` Siddhesh Poyarekar
  0 siblings, 1 reply; 25+ messages in thread
From: Adhemerval Zanella @ 2020-12-22 19:04 UTC (permalink / raw)
  To: Siddhesh Poyarekar, libc-alpha; +Cc: fweimer, joseph



On 15/12/2020 11:13, Siddhesh Poyarekar via Libc-alpha wrote:
> Sync up with gcc behaviour.  This change splits out the core isnanl
> logic so that it can be reused.
> ---
>  sysdeps/i386/fpu/s_isnanl.c     | 10 ++--------
>  sysdeps/x86/fpu/isnanl_common.h | 32 ++++++++++++++++++++++++++++++++
>  2 files changed, 34 insertions(+), 8 deletions(-)
>  create mode 100644 sysdeps/x86/fpu/isnanl_common.h
> 
> diff --git a/sysdeps/i386/fpu/s_isnanl.c b/sysdeps/i386/fpu/s_isnanl.c
> index fb97317bc9..6824f31d96 100644
> --- a/sysdeps/i386/fpu/s_isnanl.c
> +++ b/sysdeps/i386/fpu/s_isnanl.c
> @@ -25,19 +25,13 @@ static char rcsid[] = "$NetBSD: $";
>  
>  #include <math.h>
>  #include <math_private.h>
> +#include "sysdeps/x86/fpu/isnanl_common.h"
>  
>  int __isnanl(long double x)
>  {
>  	int32_t se,hx,lx;
>  	GET_LDOUBLE_WORDS(se,hx,lx,x);
> -	se = (se & 0x7fff) << 1;
> -	/* The additional & 0x7fffffff is required because Intel's
> -	   extended format has the normally implicit 1 explicit
> -	   present.  Sigh!  */
> -	lx |= hx & 0x7fffffff;
> -	se |= (uint32_t)(lx|(-lx))>>31;
> -	se = 0xfffe - se;
> -	return (int)((uint32_t)(se))>>16;
> +	return x86_isnanl (se, hx, lx);
>  }
>  hidden_def (__isnanl)
>  weak_alias (__isnanl, isnanl)

As for fpclassify, I think a better strategy would to move this code to
sysdeps/x86/fpu.

> diff --git a/sysdeps/x86/fpu/isnanl_common.h b/sysdeps/x86/fpu/isnanl_common.h
> new file mode 100644
> index 0000000000..60a4af5b41
> --- /dev/null
> +++ b/sysdeps/x86/fpu/isnanl_common.h
> @@ -0,0 +1,32 @@
> +/* Common inline isnanl implementation for x86.
> +   Copyright (C) 2020 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <https://www.gnu.org/licenses/>.  */
> +
> +static inline __always_inline int
> +x86_isnanl (int32_t se, int32_t hx, int32_t lx)
> +{
> +  se = (se & 0x7fff) << 1;
> +  /* Detect pseudo-normal numbers, i.e. exponent is non-zero and the top
> +     bit of the significand is not set.   */

This sentence sounds strange, would 'if' instead of 'of' be better?

> +  int pn = (uint32_t)((~hx & 0x80000000) & (se | (-se))) >> 31;

Ok.

> +  /* Clear the significand bit when computing mantissa.  */
> +  lx |= hx & 0x7fffffff;
> +  se |= (uint32_t)(lx | (-lx)) >> 31;
> +  se = 0xfffe - se;
> +
> +  return (int)(((uint32_t)(se)) >> 16) | pn;
> +}
> 

Not sure if this is really a gain here, is this routine really worth
to move to inline instead of just calling the __isnanl (since it seems
to be used solely on issignalingl)?

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 4/5] x86 long double: Consider pseudo numbers as signaling
  2020-12-15 14:13 ` [PATCH 4/5] x86 long double: Consider pseudo numbers as signaling Siddhesh Poyarekar
@ 2020-12-22 20:13   ` Adhemerval Zanella
  2020-12-23  1:50     ` Siddhesh Poyarekar
  0 siblings, 1 reply; 25+ messages in thread
From: Adhemerval Zanella @ 2020-12-22 20:13 UTC (permalink / raw)
  To: libc-alpha



On 15/12/2020 11:13, Siddhesh Poyarekar via Libc-alpha wrote:
> Implement a separate x86 version of issignalingl that returns true for
> pseudo-normal numbers.
> 
> Also drop comment from the generic version of s_issignalingl since it
> is no longer true or relevant.
> ---
>  sysdeps/ieee754/ldbl-96/s_issignalingl.c |  2 --
>  sysdeps/x86/fpu/s_issignalingl.c         | 39 ++++++++++++++++++++++++
>  2 files changed, 39 insertions(+), 2 deletions(-)
>  create mode 100644 sysdeps/x86/fpu/s_issignalingl.c
> 
> diff --git a/sysdeps/ieee754/ldbl-96/s_issignalingl.c b/sysdeps/ieee754/ldbl-96/s_issignalingl.c
> index ec542ad468..2aa0ffae0e 100644
> --- a/sysdeps/ieee754/ldbl-96/s_issignalingl.c
> +++ b/sysdeps/ieee754/ldbl-96/s_issignalingl.c
> @@ -34,8 +34,6 @@ __issignalingl (long double x)
>    hxi ^= 0x40000000;
>    /* If lxi != 0, then set any suitable bit of the significand in hxi.  */
>    hxi |= (lxi | -lxi) >> 31;
> -  /* We do not recognize a pseudo NaN as sNaN; they're invalid on 80387 and
> -     later.  */
>    /* We have to compare for greater (instead of greater or equal), because x's
>       significand being all-zero designates infinity not NaN.  */
>    return ((exi & 0x7fff) == 0x7fff) && (hxi > 0xc0000000);
> diff --git a/sysdeps/x86/fpu/s_issignalingl.c b/sysdeps/x86/fpu/s_issignalingl.c

We have multiple internal defines that try to use the common implementation
instead of pushing for arch-specific ones (for instance nan-high-order-bit.h, 
fix-int-fp-convert-zero.h, fix-fp-int-convert-overflow.h, etc).  Maybe it
would be simpler to just add a new header (nan-handle-pseudo-number.h or
something related) and adapt the generic implementation.

> new file mode 100644
> index 0000000000..df273b881d
> --- /dev/null
> +++ b/sysdeps/x86/fpu/s_issignalingl.c
> @@ -0,0 +1,39 @@
> +/* Test for signaling NaN - x86 version.
> +   Copyright (C) 2020 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <https://www.gnu.org/licenses/>.  */
> +
> +#include <math.h>
> +#include <math_private.h>
> +#include <nan-high-order-bit.h>
> +#include "sysdeps/x86/fpu/isnanl_common.h"
> +
> +int
> +__issignalingl (long double x)
> +{
> +  uint32_t exi, hxi, lxi, nan, pn;
> +  GET_LDOUBLE_WORDS (exi, hxi, lxi, x);
> +#if HIGH_ORDER_BIT_IS_SET_FOR_SNAN
> +# error not implemented
> +#else
> +  nan = x86_isnanl (exi, hxi, lxi);
> +  pn = (~hxi & 0x80000000) >> 31;
> +  hxi = (~hxi & 0x40000000) >> 30;
> +
> +  return nan & (pn | hxi);
> +#endif
> +}
> +libm_hidden_def (__issignalingl)
> 

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 5/5] x86 long double: Add tests for pseudo normal numbers
  2020-12-15 14:13 ` [PATCH 5/5] x86 long double: Add tests for pseudo normal numbers Siddhesh Poyarekar
@ 2020-12-22 21:48   ` Adhemerval Zanella
  2020-12-23  1:58     ` Siddhesh Poyarekar
  0 siblings, 1 reply; 25+ messages in thread
From: Adhemerval Zanella @ 2020-12-22 21:48 UTC (permalink / raw)
  To: Siddhesh Poyarekar, libc-alpha; +Cc: fweimer, joseph



On 15/12/2020 11:13, Siddhesh Poyarekar via Libc-alpha wrote:
> Add some tests for fpclassify, isnanl, isinfl and issignaling.
> ---
>  sysdeps/x86/fpu/Makefile        |   3 +-
>  sysdeps/x86/fpu/test-unnormal.c | 196 ++++++++++++++++++++++++++++++++
>  2 files changed, 198 insertions(+), 1 deletion(-)
>  create mode 100644 sysdeps/x86/fpu/test-unnormal.c
> 
> diff --git a/sysdeps/x86/fpu/Makefile b/sysdeps/x86/fpu/Makefile
> index 600e42c3db..e77de56d14 100644
> --- a/sysdeps/x86/fpu/Makefile
> +++ b/sysdeps/x86/fpu/Makefile
> @@ -4,11 +4,12 @@ CPPFLAGS += -I../soft-fp
>  
>  libm-support += powl_helper
>  tests += test-fenv-sse test-fenv-clear-sse test-fenv-x87 test-fenv-sse-2 \
> -	 test-flt-eval-method-387 test-flt-eval-method-sse
> +	 test-flt-eval-method-387 test-flt-eval-method-sse test-unnormal
>  CFLAGS-test-fenv-sse.c += -msse2 -mfpmath=sse
>  CFLAGS-test-fenv-clear-sse.c += -msse2 -mfpmath=sse
>  CFLAGS-test-fenv-sse-2.c += -msse2 -mfpmath=sse
>  CFLAGS-test-flt-eval-method-387.c += -fexcess-precision=standard -mfpmath=387
>  CFLAGS-test-flt-eval-method-sse.c += -fexcess-precision=standard -msse2 \
>  				     -mfpmath=sse
> +CFLAGS-test-unnormal.c += -fsignaling-nans -std=c2x
>  endif

A possibility is to hookup this tests on 
math/libm-test-{fpclassify,isnan,isinf,issignaling}.inc using the new define
I suggested on the 4/5 part [1] so you can also check if no exceptions are being
generated and errno is not set.

It increases the tests coverage and avoid a arch-specific tests.

[1] https://sourceware.org/pipermail/libc-alpha/2020-December/121004.html

> diff --git a/sysdeps/x86/fpu/test-unnormal.c b/sysdeps/x86/fpu/test-unnormal.c
> new file mode 100644
> index 0000000000..fc65d9290f
> --- /dev/null
> +++ b/sysdeps/x86/fpu/test-unnormal.c
> @@ -0,0 +1,196 @@
> +/* Test long double classification with x86 pseudo normal numbers.
> +   Copyright (C) 2020 Free Software Foundation, Inc.
> +
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <https://www.gnu.org/licenses/>.  */
> +
> +#include <math.h>
> +#include <stdio.h>
> +#include <string.h>
> +
> +struct tests
> +{
> +  const char *val;
> +  int class;
> +} inputs[] = {
> +      /* Normal.  */
> +      {"\x00\x04\x00\x00\x00\x00\x00\x00\x00\x04", FP_NAN},
> +      {"\x00\x04\x00\x00\x00\x00\x00\xf0\x00\x04", FP_NORMAL},
> +      /* Pseudo-infinite.  */
> +      {"\x00\x00\x00\x00\x00\x00\x00\x00\xff\x7f", FP_NAN},
> +      {"\x00\x00\x00\x00\x00\x00\x00\x80\xff\x7f", FP_INFINITE},
> +      /* Pseudo-zero.  */
> +      {"\x00\x00\x00\x00\x00\x00\x00\x00\x00\x01", FP_NAN},
> +      {"\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00", FP_ZERO},
> +};
> +

I find this quite confusing to parse the value represented. I think
it would be way more readable to include <math_ldbl.h> and define the
values using the ieee_long_double_shape_type 'parts' member.

If the idea is also to check snprintf, I think it would be better to
the tests to a different test.

Also make the inputs a 'static' variable.

> +const char *classes[5];
> +#define stringify(N) #N
> +
> +static void
> +initialize (void)
> +{
> +  classes[FP_NAN] = stringify(FP_NAN);
> +  classes[FP_INFINITE] = stringify(FP_INFINITY);
> +  classes[FP_ZERO] = stringify(FP_ZERO);
> +  classes[FP_SUBNORMAL] = stringify(FP_SUBNORMAL);
> +  classes[FP_NORMAL] = stringify(FP_NORMAL);
> +}
> +
> +static void
> +unnormal_str (const char *val, char *ret)
> +{
> +  for (int i = 9; i >= 0; i--)
> +    {
> +      if (i == 7 || i == 3)
> +	*ret++ = ' ';
> +      snprintf(ret, 3, "%02x", (unsigned char) val[i]);
> +      ret += 2;
> +    }
> +}
> +
> +static int
> +test_fpclassify (void)
> +{
> +  int ret = 0;
> +
> +  printf ("* fpclassify tests:\n");

Maybe add the verbose output only when tests is invoke with --debug
(same for other cases).

> +  for (int i = 0; i < sizeof (inputs)/sizeof (struct tests); i++)
> +    {
> +      long double value;
> +      char buf[22];
> +
> +      memcpy (&value, inputs[i].val, 10);
> +      unnormal_str(inputs[i].val, buf);
> +      int class = fpclassify(value);
> +
> +      if (class != inputs[i].class)

Use TEST_COMPARE.

> +	{
> +	  printf ("0x%s: got %s, expected %s\n", buf,
> +		  classes[fpclassify(value)],
> +		  classes[inputs[i].class]);
> +	  ret |= 1;
> +	}
> +      else
> +	printf ("0x%s: OK\n", buf);
> +    }
> +  return ret;
> +}
> +
> +static int
> +test_isinf (void)
> +{
> +  int ret = 0;
> +
> +  printf ("* isinfl tests:\n");
> +  for (int i = 0; i < sizeof (inputs)/sizeof (struct tests); i++)
> +    {
> +      long double value;
> +      char buf[22];
> +
> +      memcpy (&value, inputs[i].val, 10);
> +      unnormal_str(inputs[i].val, buf);
> +      int inf = isinf (value);
> +
> +      if ((inputs[i].class == FP_INFINITE && inf)
> +	  || (inputs[i].class != FP_INFINITE && !inf))
> +	printf ("0x%s: OK\n", buf);
> +      else
> +	{
> +	  printf ("0x%s: got %s, expected %s\n", buf,
> +		  inf ? "INFINITE" : "NOT INFINITE",
> +		  classes[inputs[i].class]);
> +	  ret |= 1;
> +	}
> +    }
> +
> +  return ret;
> +}
> +
> +static int
> +test_isnan (void)
> +{
> +  int ret = 0;
> +
> +  printf ("* isnanl tests:\n");
> +  for (int i = 0; i < sizeof (inputs)/sizeof (struct tests); i++)
> +    {
> +      long double value;
> +      char buf[22];
> +
> +      memcpy (&value, inputs[i].val, 10);
> +      unnormal_str(inputs[i].val, buf);
> +      int nan = isnan (value);
> +
> +      if ((inputs[i].class == FP_NAN && nan)
> +	  || (inputs[i].class != FP_NAN && !nan))
> +	printf ("0x%s: OK\n", buf);
> +      else
> +	{
> +	  printf ("0x%s: got %s, expected %s\n", buf,
> +		  nan ? "NAN" : "NOT NAN",
> +		  classes[inputs[i].class]);
> +	  ret |= 1;
> +	}
> +    }
> +  return ret;
> +}
> +
> +static int
> +test_issignaling (void)
> +{
> +  int ret = 0;
> +
> +  printf ("* issignaling tests:\n");
> +  for (int i = 0; i < sizeof (inputs)/sizeof (struct tests); i++)
> +    {
> +      long double value;
> +      char buf[22];
> +
> +      memcpy (&value, inputs[i].val, 10);
> +      unnormal_str(inputs[i].val, buf);
> +      int signaling = issignaling (value);
> +
> +      if ((inputs[i].class == FP_NAN && signaling)
> +	  || (inputs[i].class != FP_NAN && !signaling))
> +	printf ("0x%s: OK\n", buf);
> +      else
> +	{
> +	  printf ("0x%s: got %s, expected %s\n", buf,
> +		  signaling ? "SIGNALING" : "NOT SIGNALING",
> +		  classes[inputs[i].class]);
> +	  ret |= 1;
> +	}
> +    }
> +  return ret;
> +}
> +
> +int
> +do_test (void)
> +{
> +  int ret = 0;
> +
> +  initialize ();
> +
> +  ret |= test_fpclassify ();
> +  ret |= test_isinf ();
> +  ret |= test_isnan ();
> +  ret |= test_issignaling ();
> +
> +  return ret;
> +}
> +
> +#include <support/test-driver.c>
> 

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 1/5] x86 long double: Support pseudo numbers in fpclassifyl
  2020-12-22 18:43   ` Adhemerval Zanella
@ 2020-12-23  1:43     ` Siddhesh Poyarekar
  0 siblings, 0 replies; 25+ messages in thread
From: Siddhesh Poyarekar @ 2020-12-23  1:43 UTC (permalink / raw)
  To: Adhemerval Zanella, libc-alpha; +Cc: Florian Weimer, Joseph Myers

On 12/23/20 12:13 AM, Adhemerval Zanella via Libc-alpha wrote:
> 
> 
> On 15/12/2020 11:13, Siddhesh Poyarekar via Libc-alpha wrote:
>> ---
>>   sysdeps/i386/fpu/s_fpclassifyl.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/sysdeps/i386/fpu/s_fpclassifyl.c b/sysdeps/i386/fpu/s_fpclassifyl.c
>> index 501312f51e..aeaec7279c 100644
>> --- a/sysdeps/i386/fpu/s_fpclassifyl.c
>> +++ b/sysdeps/i386/fpu/s_fpclassifyl.c
>> @@ -34,6 +34,10 @@ __fpclassifyl (long double x)
>>       retval = FP_ZERO;
>>     else if (ex == 0 && (hx & 0x80000000) == 0)
>>       retval = FP_SUBNORMAL;
>> +  /* Pseudo-normals, i.e. pseudo-zero, pseudo-infinity and unnormals.  They
> 
> Intel manual adds a hyphen for un-normals.
> 

Fixed.

>> +     behave like NaNs, so categorize them as such.  */
>> +  else if ((hx & 0x80000000) == 0)
>> +    retval = FP_NAN;
> 
> Ok.
> 
>>     else if (ex == 0x7fff)
>>       retval = ((hx & 0x7fffffff) | lx) != 0 ? FP_NAN : FP_INFINITE;
>>   
>>
> 
> Maybe move the implementation to sysdep/x86/fpu since it is used for
> both x86_64 and i386?  It avoid the implicit cross abi include the
> and make the implementation selection more clear.
> 

OK.

Thanks,
Siddhesh

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 2/5] x86 long double: Support pseudo numbers in isnanl
  2020-12-22 19:04   ` Adhemerval Zanella
@ 2020-12-23  1:49     ` Siddhesh Poyarekar
  2020-12-23  8:34       ` Siddhesh Poyarekar
  0 siblings, 1 reply; 25+ messages in thread
From: Siddhesh Poyarekar @ 2020-12-23  1:49 UTC (permalink / raw)
  To: Adhemerval Zanella, libc-alpha; +Cc: fweimer, joseph

On 12/23/20 12:34 AM, Adhemerval Zanella via Libc-alpha wrote:
> 
> 
> On 15/12/2020 11:13, Siddhesh Poyarekar via Libc-alpha wrote:
>> Sync up with gcc behaviour.  This change splits out the core isnanl
>> logic so that it can be reused.
>> ---
>>   sysdeps/i386/fpu/s_isnanl.c     | 10 ++--------
>>   sysdeps/x86/fpu/isnanl_common.h | 32 ++++++++++++++++++++++++++++++++
>>   2 files changed, 34 insertions(+), 8 deletions(-)
>>   create mode 100644 sysdeps/x86/fpu/isnanl_common.h
>>
>> diff --git a/sysdeps/i386/fpu/s_isnanl.c b/sysdeps/i386/fpu/s_isnanl.c
>> index fb97317bc9..6824f31d96 100644
>> --- a/sysdeps/i386/fpu/s_isnanl.c
>> +++ b/sysdeps/i386/fpu/s_isnanl.c
>> @@ -25,19 +25,13 @@ static char rcsid[] = "$NetBSD: $";
>>   
>>   #include <math.h>
>>   #include <math_private.h>
>> +#include "sysdeps/x86/fpu/isnanl_common.h"
>>   
>>   int __isnanl(long double x)
>>   {
>>   	int32_t se,hx,lx;
>>   	GET_LDOUBLE_WORDS(se,hx,lx,x);
>> -	se = (se & 0x7fff) << 1;
>> -	/* The additional & 0x7fffffff is required because Intel's
>> -	   extended format has the normally implicit 1 explicit
>> -	   present.  Sigh!  */
>> -	lx |= hx & 0x7fffffff;
>> -	se |= (uint32_t)(lx|(-lx))>>31;
>> -	se = 0xfffe - se;
>> -	return (int)((uint32_t)(se))>>16;
>> +	return x86_isnanl (se, hx, lx);
>>   }
>>   hidden_def (__isnanl)
>>   weak_alias (__isnanl, isnanl)
> 
> As for fpclassify, I think a better strategy would to move this code to
> sysdeps/x86/fpu.

OK.

> 
>> diff --git a/sysdeps/x86/fpu/isnanl_common.h b/sysdeps/x86/fpu/isnanl_common.h
>> new file mode 100644
>> index 0000000000..60a4af5b41
>> --- /dev/null
>> +++ b/sysdeps/x86/fpu/isnanl_common.h
>> @@ -0,0 +1,32 @@
>> +/* Common inline isnanl implementation for x86.
>> +   Copyright (C) 2020 Free Software Foundation, Inc.
>> +   This file is part of the GNU C Library.
>> +
>> +   The GNU C Library is free software; you can redistribute it and/or
>> +   modify it under the terms of the GNU Lesser General Public
>> +   License as published by the Free Software Foundation; either
>> +   version 2.1 of the License, or (at your option) any later version.
>> +
>> +   The GNU C Library is distributed in the hope that it will be useful,
>> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>> +   Lesser General Public License for more details.
>> +
>> +   You should have received a copy of the GNU Lesser General Public
>> +   License along with the GNU C Library; if not, see
>> +   <https://www.gnu.org/licenses/>.  */
>> +
>> +static inline __always_inline int
>> +x86_isnanl (int32_t se, int32_t hx, int32_t lx)
>> +{
>> +  se = (se & 0x7fff) << 1;
>> +  /* Detect pseudo-normal numbers, i.e. exponent is non-zero and the top
>> +     bit of the significand is not set.   */
> 
> This sentence sounds strange, would 'if' instead of 'of' be better?
> 

Yes that's a typo :)

>> +  int pn = (uint32_t)((~hx & 0x80000000) & (se | (-se))) >> 31;
> 
> Ok.
> 
>> +  /* Clear the significand bit when computing mantissa.  */
>> +  lx |= hx & 0x7fffffff;
>> +  se |= (uint32_t)(lx | (-lx)) >> 31;
>> +  se = 0xfffe - se;
>> +
>> +  return (int)(((uint32_t)(se)) >> 16) | pn;
>> +}
>>
> 
> Not sure if this is really a gain here, is this routine really worth
> to move to inline instead of just calling the __isnanl (since it seems
> to be used solely on issignalingl)?
> 

The functions are micro-optimised to even avoid branches, so I reckon 
the extra indirection would be a no no.  Also, the inlining ought to 
optimize issignaling further as there are redundant ops in there.

Siddhesh

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 4/5] x86 long double: Consider pseudo numbers as signaling
  2020-12-22 20:13   ` Adhemerval Zanella
@ 2020-12-23  1:50     ` Siddhesh Poyarekar
  0 siblings, 0 replies; 25+ messages in thread
From: Siddhesh Poyarekar @ 2020-12-23  1:50 UTC (permalink / raw)
  To: Adhemerval Zanella, libc-alpha

On 12/23/20 1:43 AM, Adhemerval Zanella via Libc-alpha wrote:
> 
> 
> On 15/12/2020 11:13, Siddhesh Poyarekar via Libc-alpha wrote:
>> Implement a separate x86 version of issignalingl that returns true for
>> pseudo-normal numbers.
>>
>> Also drop comment from the generic version of s_issignalingl since it
>> is no longer true or relevant.
>> ---
>>   sysdeps/ieee754/ldbl-96/s_issignalingl.c |  2 --
>>   sysdeps/x86/fpu/s_issignalingl.c         | 39 ++++++++++++++++++++++++
>>   2 files changed, 39 insertions(+), 2 deletions(-)
>>   create mode 100644 sysdeps/x86/fpu/s_issignalingl.c
>>
>> diff --git a/sysdeps/ieee754/ldbl-96/s_issignalingl.c b/sysdeps/ieee754/ldbl-96/s_issignalingl.c
>> index ec542ad468..2aa0ffae0e 100644
>> --- a/sysdeps/ieee754/ldbl-96/s_issignalingl.c
>> +++ b/sysdeps/ieee754/ldbl-96/s_issignalingl.c
>> @@ -34,8 +34,6 @@ __issignalingl (long double x)
>>     hxi ^= 0x40000000;
>>     /* If lxi != 0, then set any suitable bit of the significand in hxi.  */
>>     hxi |= (lxi | -lxi) >> 31;
>> -  /* We do not recognize a pseudo NaN as sNaN; they're invalid on 80387 and
>> -     later.  */
>>     /* We have to compare for greater (instead of greater or equal), because x's
>>        significand being all-zero designates infinity not NaN.  */
>>     return ((exi & 0x7fff) == 0x7fff) && (hxi > 0xc0000000);
>> diff --git a/sysdeps/x86/fpu/s_issignalingl.c b/sysdeps/x86/fpu/s_issignalingl.c
> 
> We have multiple internal defines that try to use the common implementation
> instead of pushing for arch-specific ones (for instance nan-high-order-bit.h,
> fix-int-fp-convert-zero.h, fix-fp-int-convert-overflow.h, etc).  Maybe it
> would be simpler to just add a new header (nan-handle-pseudo-number.h or
> something related) and adapt the generic implementation.

OK I'll try to do that.

Thanks,
Siddhesh

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 5/5] x86 long double: Add tests for pseudo normal numbers
  2020-12-22 21:48   ` Adhemerval Zanella
@ 2020-12-23  1:58     ` Siddhesh Poyarekar
  2020-12-23 10:20       ` Siddhesh Poyarekar
  0 siblings, 1 reply; 25+ messages in thread
From: Siddhesh Poyarekar @ 2020-12-23  1:58 UTC (permalink / raw)
  To: Adhemerval Zanella, libc-alpha; +Cc: fweimer, joseph

On 12/23/20 3:18 AM, Adhemerval Zanella via Libc-alpha wrote:
> 
> 
> On 15/12/2020 11:13, Siddhesh Poyarekar via Libc-alpha wrote:
>> Add some tests for fpclassify, isnanl, isinfl and issignaling.
>> ---
>>   sysdeps/x86/fpu/Makefile        |   3 +-
>>   sysdeps/x86/fpu/test-unnormal.c | 196 ++++++++++++++++++++++++++++++++
>>   2 files changed, 198 insertions(+), 1 deletion(-)
>>   create mode 100644 sysdeps/x86/fpu/test-unnormal.c
>>
>> diff --git a/sysdeps/x86/fpu/Makefile b/sysdeps/x86/fpu/Makefile
>> index 600e42c3db..e77de56d14 100644
>> --- a/sysdeps/x86/fpu/Makefile
>> +++ b/sysdeps/x86/fpu/Makefile
>> @@ -4,11 +4,12 @@ CPPFLAGS += -I../soft-fp
>>   
>>   libm-support += powl_helper
>>   tests += test-fenv-sse test-fenv-clear-sse test-fenv-x87 test-fenv-sse-2 \
>> -	 test-flt-eval-method-387 test-flt-eval-method-sse
>> +	 test-flt-eval-method-387 test-flt-eval-method-sse test-unnormal
>>   CFLAGS-test-fenv-sse.c += -msse2 -mfpmath=sse
>>   CFLAGS-test-fenv-clear-sse.c += -msse2 -mfpmath=sse
>>   CFLAGS-test-fenv-sse-2.c += -msse2 -mfpmath=sse
>>   CFLAGS-test-flt-eval-method-387.c += -fexcess-precision=standard -mfpmath=387
>>   CFLAGS-test-flt-eval-method-sse.c += -fexcess-precision=standard -msse2 \
>>   				     -mfpmath=sse
>> +CFLAGS-test-unnormal.c += -fsignaling-nans -std=c2x
>>   endif
> 
> A possibility is to hookup this tests on
> math/libm-test-{fpclassify,isnan,isinf,issignaling}.inc using the new define
> I suggested on the 4/5 part [1] so you can also check if no exceptions are being
> generated and errno is not set.
> 
> It increases the tests coverage and avoid a arch-specific tests.
> 
> [1] https://sourceware.org/pipermail/libc-alpha/2020-December/121004.html

OK, it will need changes to the driver.

>> diff --git a/sysdeps/x86/fpu/test-unnormal.c b/sysdeps/x86/fpu/test-unnormal.c
>> new file mode 100644
>> index 0000000000..fc65d9290f
>> --- /dev/null
>> +++ b/sysdeps/x86/fpu/test-unnormal.c
>> @@ -0,0 +1,196 @@
>> +/* Test long double classification with x86 pseudo normal numbers.
>> +   Copyright (C) 2020 Free Software Foundation, Inc.
>> +
>> +   This file is part of the GNU C Library.
>> +
>> +   The GNU C Library is free software; you can redistribute it and/or
>> +   modify it under the terms of the GNU Lesser General Public
>> +   License as published by the Free Software Foundation; either
>> +   version 2.1 of the License, or (at your option) any later version.
>> +
>> +   The GNU C Library is distributed in the hope that it will be useful,
>> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>> +   Lesser General Public License for more details.
>> +
>> +   You should have received a copy of the GNU Lesser General Public
>> +   License along with the GNU C Library; if not, see
>> +   <https://www.gnu.org/licenses/>.  */
>> +
>> +#include <math.h>
>> +#include <stdio.h>
>> +#include <string.h>
>> +
>> +struct tests
>> +{
>> +  const char *val;
>> +  int class;
>> +} inputs[] = {
>> +      /* Normal.  */
>> +      {"\x00\x04\x00\x00\x00\x00\x00\x00\x00\x04", FP_NAN},
>> +      {"\x00\x04\x00\x00\x00\x00\x00\xf0\x00\x04", FP_NORMAL},
>> +      /* Pseudo-infinite.  */
>> +      {"\x00\x00\x00\x00\x00\x00\x00\x00\xff\x7f", FP_NAN},
>> +      {"\x00\x00\x00\x00\x00\x00\x00\x80\xff\x7f", FP_INFINITE},
>> +      /* Pseudo-zero.  */
>> +      {"\x00\x00\x00\x00\x00\x00\x00\x00\x00\x01", FP_NAN},
>> +      {"\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00", FP_ZERO},
>> +};
>> +
> 
> I find this quite confusing to parse the value represented. I think
> it would be way more readable to include <math_ldbl.h> and define the
> values using the ieee_long_double_shape_type 'parts' member.

That's a good idea.

> If the idea is also to check snprintf, I think it would be better to
> the tests to a different test.

The *printf already has its own test.

> Also make the inputs a 'static' variable.

OK.

>> +const char *classes[5];
>> +#define stringify(N) #N
>> +
>> +static void
>> +initialize (void)
>> +{
>> +  classes[FP_NAN] = stringify(FP_NAN);
>> +  classes[FP_INFINITE] = stringify(FP_INFINITY);
>> +  classes[FP_ZERO] = stringify(FP_ZERO);
>> +  classes[FP_SUBNORMAL] = stringify(FP_SUBNORMAL);
>> +  classes[FP_NORMAL] = stringify(FP_NORMAL);
>> +}
>> +
>> +static void
>> +unnormal_str (const char *val, char *ret)
>> +{
>> +  for (int i = 9; i >= 0; i--)
>> +    {
>> +      if (i == 7 || i == 3)
>> +	*ret++ = ' ';
>> +      snprintf(ret, 3, "%02x", (unsigned char) val[i]);
>> +      ret += 2;
>> +    }
>> +}
>> +
>> +static int
>> +test_fpclassify (void)
>> +{
>> +  int ret = 0;
>> +
>> +  printf ("* fpclassify tests:\n");
> 
> Maybe add the verbose output only when tests is invoke with --debug
> (same for other cases).

OK.

>> +  for (int i = 0; i < sizeof (inputs)/sizeof (struct tests); i++)
>> +    {
>> +      long double value;
>> +      char buf[22];
>> +
>> +      memcpy (&value, inputs[i].val, 10);
>> +      unnormal_str(inputs[i].val, buf);
>> +      int class = fpclassify(value);
>> +
>> +      if (class != inputs[i].class)
> 
> Use TEST_COMPARE.

OK.

Thanks,
Siddhesh

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 2/5] x86 long double: Support pseudo numbers in isnanl
  2020-12-23  1:49     ` Siddhesh Poyarekar
@ 2020-12-23  8:34       ` Siddhesh Poyarekar
  0 siblings, 0 replies; 25+ messages in thread
From: Siddhesh Poyarekar @ 2020-12-23  8:34 UTC (permalink / raw)
  To: Siddhesh Poyarekar, Adhemerval Zanella, libc-alpha; +Cc: fweimer, joseph

On 12/23/20 7:19 AM, Siddhesh Poyarekar via Libc-alpha wrote:
>>> +  /* Detect pseudo-normal numbers, i.e. exponent is non-zero and the 
>>> top
>>> +     bit of the significand is not set.   */
>>
>> This sentence sounds strange, would 'if' instead of 'of' be better?
>>
> 
> Yes that's a typo :)
> 

Oh wait, it's not.  "the top bit of the significant is not set", i.e. 
the MSB of the significand.

Siddhesh

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 5/5] x86 long double: Add tests for pseudo normal numbers
  2020-12-23  1:58     ` Siddhesh Poyarekar
@ 2020-12-23 10:20       ` Siddhesh Poyarekar
  2020-12-23 17:44         ` Adhemerval Zanella
  0 siblings, 1 reply; 25+ messages in thread
From: Siddhesh Poyarekar @ 2020-12-23 10:20 UTC (permalink / raw)
  To: Siddhesh Poyarekar, Adhemerval Zanella, libc-alpha; +Cc: fweimer, joseph

On 12/23/20 7:28 AM, Siddhesh Poyarekar via Libc-alpha wrote:
> On 12/23/20 3:18 AM, Adhemerval Zanella via Libc-alpha wrote:
>>
>>
>> On 15/12/2020 11:13, Siddhesh Poyarekar via Libc-alpha wrote:
>>> Add some tests for fpclassify, isnanl, isinfl and issignaling.
>>> ---
>>>   sysdeps/x86/fpu/Makefile        |   3 +-
>>>   sysdeps/x86/fpu/test-unnormal.c | 196 ++++++++++++++++++++++++++++++++
>>>   2 files changed, 198 insertions(+), 1 deletion(-)
>>>   create mode 100644 sysdeps/x86/fpu/test-unnormal.c
>>>
>>> diff --git a/sysdeps/x86/fpu/Makefile b/sysdeps/x86/fpu/Makefile
>>> index 600e42c3db..e77de56d14 100644
>>> --- a/sysdeps/x86/fpu/Makefile
>>> +++ b/sysdeps/x86/fpu/Makefile
>>> @@ -4,11 +4,12 @@ CPPFLAGS += -I../soft-fp
>>>   libm-support += powl_helper
>>>   tests += test-fenv-sse test-fenv-clear-sse test-fenv-x87 
>>> test-fenv-sse-2 \
>>> -     test-flt-eval-method-387 test-flt-eval-method-sse
>>> +     test-flt-eval-method-387 test-flt-eval-method-sse test-unnormal
>>>   CFLAGS-test-fenv-sse.c += -msse2 -mfpmath=sse
>>>   CFLAGS-test-fenv-clear-sse.c += -msse2 -mfpmath=sse
>>>   CFLAGS-test-fenv-sse-2.c += -msse2 -mfpmath=sse
>>>   CFLAGS-test-flt-eval-method-387.c += -fexcess-precision=standard 
>>> -mfpmath=387
>>>   CFLAGS-test-flt-eval-method-sse.c += -fexcess-precision=standard 
>>> -msse2 \
>>>                        -mfpmath=sse
>>> +CFLAGS-test-unnormal.c += -fsignaling-nans -std=c2x
>>>   endif
>>
>> A possibility is to hookup this tests on
>> math/libm-test-{fpclassify,isnan,isinf,issignaling}.inc using the new 
>> define
>> I suggested on the 4/5 part [1] so you can also check if no exceptions 
>> are being
>> generated and errno is not set.
>>
>> It increases the tests coverage and avoid a arch-specific tests.
>>
>> [1] https://sourceware.org/pipermail/libc-alpha/2020-December/121004.html
> 
> OK, it will need changes to the driver.
> 

I took a look at this and it looks like it will need a significant 
amount of changes to the driver and test generation scripts, since these 
numbers are not generated by the CPU.  Would it be OK if I defer this to 
later if we ever feel the need to add tests beyond test-unnormal?

Siddhesh

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 5/5] x86 long double: Add tests for pseudo normal numbers
  2020-12-23 10:20       ` Siddhesh Poyarekar
@ 2020-12-23 17:44         ` Adhemerval Zanella
  2020-12-24  0:48           ` Siddhesh Poyarekar
  0 siblings, 1 reply; 25+ messages in thread
From: Adhemerval Zanella @ 2020-12-23 17:44 UTC (permalink / raw)
  To: Siddhesh Poyarekar, Siddhesh Poyarekar, libc-alpha; +Cc: fweimer, joseph



On 23/12/2020 07:20, Siddhesh Poyarekar wrote:
> On 12/23/20 7:28 AM, Siddhesh Poyarekar via Libc-alpha wrote:
>> On 12/23/20 3:18 AM, Adhemerval Zanella via Libc-alpha wrote:
>>>
>>>
>>> On 15/12/2020 11:13, Siddhesh Poyarekar via Libc-alpha wrote:
>>>> Add some tests for fpclassify, isnanl, isinfl and issignaling.
>>>> ---
>>>>   sysdeps/x86/fpu/Makefile        |   3 +-
>>>>   sysdeps/x86/fpu/test-unnormal.c | 196 ++++++++++++++++++++++++++++++++
>>>>   2 files changed, 198 insertions(+), 1 deletion(-)
>>>>   create mode 100644 sysdeps/x86/fpu/test-unnormal.c
>>>>
>>>> diff --git a/sysdeps/x86/fpu/Makefile b/sysdeps/x86/fpu/Makefile
>>>> index 600e42c3db..e77de56d14 100644
>>>> --- a/sysdeps/x86/fpu/Makefile
>>>> +++ b/sysdeps/x86/fpu/Makefile
>>>> @@ -4,11 +4,12 @@ CPPFLAGS += -I../soft-fp
>>>>   libm-support += powl_helper
>>>>   tests += test-fenv-sse test-fenv-clear-sse test-fenv-x87 test-fenv-sse-2 \
>>>> -     test-flt-eval-method-387 test-flt-eval-method-sse
>>>> +     test-flt-eval-method-387 test-flt-eval-method-sse test-unnormal
>>>>   CFLAGS-test-fenv-sse.c += -msse2 -mfpmath=sse
>>>>   CFLAGS-test-fenv-clear-sse.c += -msse2 -mfpmath=sse
>>>>   CFLAGS-test-fenv-sse-2.c += -msse2 -mfpmath=sse
>>>>   CFLAGS-test-flt-eval-method-387.c += -fexcess-precision=standard -mfpmath=387
>>>>   CFLAGS-test-flt-eval-method-sse.c += -fexcess-precision=standard -msse2 \
>>>>                        -mfpmath=sse
>>>> +CFLAGS-test-unnormal.c += -fsignaling-nans -std=c2x
>>>>   endif
>>>
>>> A possibility is to hookup this tests on
>>> math/libm-test-{fpclassify,isnan,isinf,issignaling}.inc using the new define
>>> I suggested on the 4/5 part [1] so you can also check if no exceptions are being
>>> generated and errno is not set.
>>>
>>> It increases the tests coverage and avoid a arch-specific tests.
>>>
>>> [1] https://sourceware.org/pipermail/libc-alpha/2020-December/121004.html
>>
>> OK, it will need changes to the driver.
>>
> 
> I took a look at this and it looks like it will need a significant amount of changes to the driver and test generation scripts, since these numbers are not generated by the CPU.  Would it be OK if I defer this to later if we ever feel the need to add tests beyond test-unnormal?
> 
> Siddhesh

I don't have a strong opinion, although below you can check it does not
really require too much tinkering to add a new tests for pseudo-normal zero. 
The idea is to add the generic constants pseudo_xxx, define a new list using 
a different struct, and issue ALL_RM_TESTS with RUN_TEST_LOOP_f_i_tg_u.

It does add a *lot* more coverage (errno, rounding modes, exceptions).

---

diff --git a/math/libm-test-driver.c b/math/libm-test-driver.c
index 11b541b2e7..ddfd16cf91 100644
--- a/math/libm-test-driver.c
+++ b/math/libm-test-driver.c
@@ -19,6 +19,7 @@
 #include "libm-test-support.h"
 
 #include <math-tests-arch.h>
+#include <nan-pseudo-number.h>
 
 /* Flags set by the including file.  */
 const int flag_test_errno = TEST_ERRNO;
@@ -122,6 +123,11 @@ const char qtype_str[] = TYPE_STR;
 /* For nexttoward tests.  */
 #define snan_value_ld	__builtin_nansl ("")
 
+#if HANDLE_PSEUDO_NUMBERS
+# include <math_ldbl.h>
+#define pseudo_inf { .parts = { 0x00000000, 0x00000000, 0x7fff }}
+#endif
+
 /* Structures for each kind of test.  */
 /* Used for both RUN_TEST_LOOP_f_f and RUN_TEST_LOOP_fp_f.  */
 struct test_f_f_data
@@ -316,6 +322,18 @@ struct test_f_i_data
     int exceptions;
   } rd, rn, rz, ru;
 };
+#if HANDLE_PSEUDO_NUMBERS
+struct test_f_i_data_u
+{
+  const char *arg_str;
+  ieee_long_double_shape_type arg;
+  struct
+  {
+    int expected;
+    int exceptions;
+  } rd, rn, rz, ru;
+};
+#endif
 /* Used for RUN_TEST_LOOP_ff_b, RUN_TEST_LOOP_fpfp_b and
    RUN_TEST_LOOP_ff_i_tg.  */
 struct test_ff_i_data
@@ -832,6 +850,13 @@ struct test_Ff_b1_data
 		       (ARRAY)[i].RM_##ROUNDING_MODE.expected,		\
 		       (ARRAY)[i].RM_##ROUNDING_MODE.exceptions);	\
   ROUND_RESTORE_ ## ROUNDING_MODE
+#define RUN_TEST_LOOP_f_i_tg_u(FUNC_NAME, ARRAY, ROUNDING_MODE)		\
+  IF_ROUND_INIT_ ## ROUNDING_MODE					\
+    for (size_t i = 0; i < sizeof (ARRAY) / sizeof (ARRAY)[0]; i++)	\
+      RUN_TEST_f_i_tg ((ARRAY)[i].arg_str, FUNC_NAME, (ARRAY)[i].arg.value,\
+		       (ARRAY)[i].RM_##ROUNDING_MODE.expected,		\
+		       (ARRAY)[i].RM_##ROUNDING_MODE.exceptions);	\
+  ROUND_RESTORE_ ## ROUNDING_MODE
 #define RUN_TEST_ff_b(ARG_STR, FUNC_NAME, ARG1, ARG2, EXPECTED,		\
 		      EXCEPTIONS)					\
   do									\
diff --git a/math/libm-test-fpclassify.inc b/math/libm-test-fpclassify.inc
index 96b557ecb4..0e2e00a09d 100644
--- a/math/libm-test-fpclassify.inc
+++ b/math/libm-test-fpclassify.inc
@@ -37,10 +37,20 @@ static const struct test_f_i_data fpclassify_test_data[] =
     TEST_f_i (fpclassify, -min_subnorm_value, FP_SUBNORMAL, NO_INEXACT_EXCEPTION|ERRNO_UNCHANGED),
   };
 
+#if HANDLE_PSEUDO_NUMBERS
+static const struct test_f_i_data_u fpclassify_test_data_u[] =
+  {
+    TEST_f_i (fpclassify, pseudo_inf, FP_NAN, NO_INEXACT_EXCEPTION|ERRNO_UNCHANGED),
+  };
+#endif
+
 static void
 fpclassify_test (void)
 {
   ALL_RM_TEST (fpclassify, 1, fpclassify_test_data, RUN_TEST_LOOP_f_i_tg, END);
+#if HANDLE_PSEUDO_NUMBERS
+  ALL_RM_TEST (fpclassify, 1, fpclassify_test_data_u, RUN_TEST_LOOP_f_i_tg_u, END);
+#endif
 }
 
 static void

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 5/5] x86 long double: Add tests for pseudo normal numbers
  2020-12-23 17:44         ` Adhemerval Zanella
@ 2020-12-24  0:48           ` Siddhesh Poyarekar
  0 siblings, 0 replies; 25+ messages in thread
From: Siddhesh Poyarekar @ 2020-12-24  0:48 UTC (permalink / raw)
  To: Adhemerval Zanella, Siddhesh Poyarekar, libc-alpha; +Cc: fweimer, joseph

On 12/23/20 11:14 PM, Adhemerval Zanella wrote:
> I don't have a strong opinion, although below you can check it does not
> really require too much tinkering to add a new tests for pseudo-normal zero.
> The idea is to add the generic constants pseudo_xxx, define a new list using
> a different struct, and issue ALL_RM_TESTS with RUN_TEST_LOOP_f_i_tg_u.

Thanks for the example, I got caught up in the TEST_* macros and their 
conversion in python and took an unnecessary detour.  I'll give this 
another shot.

Thanks,
Siddhesh

^ permalink raw reply	[flat|nested] 25+ messages in thread

end of thread, other threads:[~2020-12-24  0:48 UTC | newest]

Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-15 14:13 [PATCH 0/5] x86 pseudo-normal numbers Siddhesh Poyarekar
2020-12-15 14:13 ` [PATCH 1/5] x86 long double: Support pseudo numbers in fpclassifyl Siddhesh Poyarekar
2020-12-22 18:43   ` Adhemerval Zanella
2020-12-23  1:43     ` Siddhesh Poyarekar
2020-12-15 14:13 ` [PATCH 2/5] x86 long double: Support pseudo numbers in isnanl Siddhesh Poyarekar
2020-12-22 19:04   ` Adhemerval Zanella
2020-12-23  1:49     ` Siddhesh Poyarekar
2020-12-23  8:34       ` Siddhesh Poyarekar
2020-12-15 14:13 ` [PATCH 3/5] Partially revert 681900d29683722b1cb0a8e565a0585846ec5a61 Siddhesh Poyarekar
2020-12-15 14:36   ` Florian Weimer
2020-12-15 15:01     ` Siddhesh Poyarekar
2020-12-15 14:13 ` [PATCH 4/5] x86 long double: Consider pseudo numbers as signaling Siddhesh Poyarekar
2020-12-22 20:13   ` Adhemerval Zanella
2020-12-23  1:50     ` Siddhesh Poyarekar
2020-12-15 14:13 ` [PATCH 5/5] x86 long double: Add tests for pseudo normal numbers Siddhesh Poyarekar
2020-12-22 21:48   ` Adhemerval Zanella
2020-12-23  1:58     ` Siddhesh Poyarekar
2020-12-23 10:20       ` Siddhesh Poyarekar
2020-12-23 17:44         ` Adhemerval Zanella
2020-12-24  0:48           ` Siddhesh Poyarekar
2020-12-15 18:26 ` [PATCH 0/5] x86 pseudo-normal numbers Joseph Myers
2020-12-15 18:29   ` Siddhesh Poyarekar
2020-12-18  4:03 ` [PING][PATCH " Siddhesh Poyarekar
2020-12-22  7:46 ` [PING*2][PATCH " Siddhesh Poyarekar
2020-12-22 11:11   ` Adhemerval Zanella

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).