Re: [PATCH] PR70117, ppc long double isinf

[Richard Biener]

On April 8, 2016 5:03:04 AM GMT+02:00, Alan Modra  wrote:
>On Thu, Apr 07, 2016 at 10:43:31AM -0400, David Edelsohn wrote:
>> Yes, I have been following this entertaining thread.
>
>How to waste lots of time over one bit.  Floating point is like that.
>:-)
>
>I see the bug was opened against 5.3, so OK to commit there after a
>few days and maybe 4.9 too, Richard?

Yes please.

Richard.

Re: [PATCH] PR70117, ppc long double isinf

[Alan Modra]

On Thu, Apr 07, 2016 at 10:43:31AM -0400, David Edelsohn wrote:
> Yes, I have been following this entertaining thread.

How to waste lots of time over one bit.  Floating point is like that.
:-)

I see the bug was opened against 5.3, so OK to commit there after a
few days and maybe 4.9 too, Richard?

-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH] PR70117, ppc long double isinf

[David Edelsohn]

On Thu, Apr 7, 2016 at 10:17 AM, Alan Modra  wrote:
> On Thu, Apr 07, 2016 at 11:32:58AM +0200, Richard Biener wrote:
>> That's good to know.  I think the patch is OK but please seek approval from 
>> a ppc maintainer as well
>
> There's only one of those.  David?  Thread starts here
> https://gcc.gnu.org/ml/gcc-patches/2016-04/msg00213.html

Yes, I have been following this entertaining thread.

This is okay.

By the way, xlc -qldbl128 should enable 128 bit.

Thanks, David

Re: [PATCH] PR70117, ppc long double isinf

[Alan Modra]

On Thu, Apr 07, 2016 at 11:32:58AM +0200, Richard Biener wrote:
> That's good to know.  I think the patch is OK but please seek approval from a 
> ppc maintainer as well

There's only one of those.  David?  Thread starts here
https://gcc.gnu.org/ml/gcc-patches/2016-04/msg00213.html

-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH] PR70117, ppc long double isinf

[Richard Biener]

On April 7, 2016 10:03:54 AM GMT+02:00, Alan Modra  wrote:
>On Wed, Apr 06, 2016 at 06:49:19PM +0930, Alan Modra wrote:
>> On Wed, Apr 06, 2016 at 10:46:48AM +0200, Richard Biener wrote:
>> > Can you add a testcase or two for the isnormal () case?
>> 
>> Sure.  I'll adapt the testcase I was using to verify the output,
>
>Revised testcase - target fixed, compiled at -O2 with volatile vars so
>we're testing optimized builtins with non-constant data.
>
>diff --git a/gcc/testsuite/gcc.target/powerpc/pr70117.c
>b/gcc/testsuite/gcc.target/powerpc/pr70117.c
>new file mode 100644
>index 000..f1fdedb
>--- /dev/null
>+++ b/gcc/testsuite/gcc.target/powerpc/pr70117.c
>@@ -0,0 +1,92 @@
>+/* { dg-do run { target { powerpc*-*-linux* powerpc*-*-darwin*
>powerpc*-*-aix* rs6000-*-* } } } */
>+/* { dg-options "-std=c99 -mlong-double-128 -O2" } */
>+
>+#include 
>+
>+union gl_long_double_union
>+{
>+  struct { double hi; double lo; } dd;
>+  long double ld;
>+};
>+
>+/* This is gnulib's LDBL_MAX which, being 107 bits in precision, is
>+   slightly larger than gcc's 106 bit precision LDBL_MAX.  */
>+volatile union gl_long_double_union gl_LDBL_MAX =
>+  { { DBL_MAX, DBL_MAX / (double)134217728UL / (double)134217728UL }
>};
>+
>+volatile double min_denorm = 0x1p-1074;
>+volatile double ld_low = 0x1p-969;
>+volatile double dinf = 1.0/0.0;
>+volatile double dnan = 0.0/0.0;
>+
>+int
>+main (void)
>+{
>+  long double ld;
>+
>+  ld = gl_LDBL_MAX.ld;
>+  if (__builtin_isinfl (ld))
>+__builtin_abort ();
>+  ld = -gl_LDBL_MAX.ld;
>+  if (__builtin_isinfl (ld))
>+__builtin_abort ();
>+
>+  ld = gl_LDBL_MAX.ld;
>+  if (!__builtin_isfinite (ld))
>+__builtin_abort ();
>+  ld = -gl_LDBL_MAX.ld;
>+  if (!__builtin_isfinite (ld))
>+__builtin_abort ();
>+
>+  ld = ld_low;
>+  if (!__builtin_isnormal (ld))
>+__builtin_abort ();
>+  ld = -ld_low;
>+  if (!__builtin_isnormal (ld))
>+__builtin_abort ();
>+
>+  ld = -min_denorm;
>+  ld += ld_low;
>+  if (__builtin_isnormal (ld))
>+__builtin_abort ();
>+  ld = min_denorm;
>+  ld -= ld_low;
>+  if (__builtin_isnormal (ld))
>+__builtin_abort ();
>+
>+  ld = 0.0;
>+  if (__builtin_isnormal (ld))
>+__builtin_abort ();
>+  ld = -0.0;
>+  if (__builtin_isnormal (ld))
>+__builtin_abort ();
>+
>+  ld = LDBL_MAX;
>+  if (!__builtin_isnormal (ld))
>+__builtin_abort ();
>+  ld = -LDBL_MAX;
>+  if (!__builtin_isnormal (ld))
>+__builtin_abort ();
>+
>+  ld = gl_LDBL_MAX.ld;
>+  if (!__builtin_isnormal (ld))
>+__builtin_abort ();
>+  ld = -gl_LDBL_MAX.ld;
>+  if (!__builtin_isnormal (ld))
>+__builtin_abort ();
>+
>+  ld = dinf;
>+  if (__builtin_isnormal (ld))
>+__builtin_abort ();
>+  ld = -dinf;
>+  if (__builtin_isnormal (ld))
>+__builtin_abort ();
>+
>+  ld = dnan;
>+  if (__builtin_isnormal (ld))
>+__builtin_abort ();
>+  ld = -dnan;
>+  if (__builtin_isnormal (ld))
>+__builtin_abort ();
>+  return 0;
>+}
>
>> >  What does XLC do here?
>> 
>> Not sure, sorry.  I don't have xlc handy.  Will try later.
>
>It seems that to compile 128-bit long double with xlc, I need xlc128,
>and I don't have that..  For a double, xlc implements isnormal() on
>power8 by moving the fpr argument to a gpr followed by a bunch of bit
>twiddling to test the exponent.  xlc's sequence isn't as good as it
>could be, 15 insns.  The ideal ought to be the following, I think,
>which gcc compiles to 8 insns on power8 (and could be 7 insns if a
>useless sign extension was eliminated).
>
>int
>bit_isnormal (double x)
>{
>  union { double d; uint64_t l; } val;
>  val.d = x;
>  uint64_t exp = (val.l >> 52) & 0x7ff;
>  return exp - 1 < 0x7fe;
>}
>
>The above is around twice as fast as fold_builtin_interclass_mathfn
>implementation of isnormal() for double, on power8.  I expect a bit
>twiddling implementation for IBM extended would show similar or better
>improvement.
>
>However I'm not inclined to pursue this, especially for gcc-6.  The
>patch I posted for isnormal() IBM extended is already faster (about
>65% average timing on power8) than what existed previously.

That's good to know.  I think the patch is OK but please seek approval from a 
ppc maintainer as well

Thanks,
Richard.

Re: [PATCH] PR70117, ppc long double isinf

[Alan Modra]

On Wed, Apr 06, 2016 at 06:49:19PM +0930, Alan Modra wrote:
> On Wed, Apr 06, 2016 at 10:46:48AM +0200, Richard Biener wrote:
> > Can you add a testcase or two for the isnormal () case?
> 
> Sure.  I'll adapt the testcase I was using to verify the output,

Revised testcase - target fixed, compiled at -O2 with volatile vars so
we're testing optimized builtins with non-constant data.

diff --git a/gcc/testsuite/gcc.target/powerpc/pr70117.c 
b/gcc/testsuite/gcc.target/powerpc/pr70117.c
new file mode 100644
index 000..f1fdedb
--- /dev/null
+++ b/gcc/testsuite/gcc.target/powerpc/pr70117.c
@@ -0,0 +1,92 @@
+/* { dg-do run { target { powerpc*-*-linux* powerpc*-*-darwin* powerpc*-*-aix* 
rs6000-*-* } } } */
+/* { dg-options "-std=c99 -mlong-double-128 -O2" } */
+
+#include 
+
+union gl_long_double_union
+{
+  struct { double hi; double lo; } dd;
+  long double ld;
+};
+
+/* This is gnulib's LDBL_MAX which, being 107 bits in precision, is
+   slightly larger than gcc's 106 bit precision LDBL_MAX.  */
+volatile union gl_long_double_union gl_LDBL_MAX =
+  { { DBL_MAX, DBL_MAX / (double)134217728UL / (double)134217728UL } };
+
+volatile double min_denorm = 0x1p-1074;
+volatile double ld_low = 0x1p-969;
+volatile double dinf = 1.0/0.0;
+volatile double dnan = 0.0/0.0;
+
+int
+main (void)
+{
+  long double ld;
+
+  ld = gl_LDBL_MAX.ld;
+  if (__builtin_isinfl (ld))
+__builtin_abort ();
+  ld = -gl_LDBL_MAX.ld;
+  if (__builtin_isinfl (ld))
+__builtin_abort ();
+
+  ld = gl_LDBL_MAX.ld;
+  if (!__builtin_isfinite (ld))
+__builtin_abort ();
+  ld = -gl_LDBL_MAX.ld;
+  if (!__builtin_isfinite (ld))
+__builtin_abort ();
+
+  ld = ld_low;
+  if (!__builtin_isnormal (ld))
+__builtin_abort ();
+  ld = -ld_low;
+  if (!__builtin_isnormal (ld))
+__builtin_abort ();
+
+  ld = -min_denorm;
+  ld += ld_low;
+  if (__builtin_isnormal (ld))
+__builtin_abort ();
+  ld = min_denorm;
+  ld -= ld_low;
+  if (__builtin_isnormal (ld))
+__builtin_abort ();
+
+  ld = 0.0;
+  if (__builtin_isnormal (ld))
+__builtin_abort ();
+  ld = -0.0;
+  if (__builtin_isnormal (ld))
+__builtin_abort ();
+
+  ld = LDBL_MAX;
+  if (!__builtin_isnormal (ld))
+__builtin_abort ();
+  ld = -LDBL_MAX;
+  if (!__builtin_isnormal (ld))
+__builtin_abort ();
+
+  ld = gl_LDBL_MAX.ld;
+  if (!__builtin_isnormal (ld))
+__builtin_abort ();
+  ld = -gl_LDBL_MAX.ld;
+  if (!__builtin_isnormal (ld))
+__builtin_abort ();
+
+  ld = dinf;
+  if (__builtin_isnormal (ld))
+__builtin_abort ();
+  ld = -dinf;
+  if (__builtin_isnormal (ld))
+__builtin_abort ();
+
+  ld = dnan;
+  if (__builtin_isnormal (ld))
+__builtin_abort ();
+  ld = -dnan;
+  if (__builtin_isnormal (ld))
+__builtin_abort ();
+  return 0;
+}

> >  What does XLC do here?
> 
> Not sure, sorry.  I don't have xlc handy.  Will try later.

It seems that to compile 128-bit long double with xlc, I need xlc128,
and I don't have that..  For a double, xlc implements isnormal() on
power8 by moving the fpr argument to a gpr followed by a bunch of bit
twiddling to test the exponent.  xlc's sequence isn't as good as it
could be, 15 insns.  The ideal ought to be the following, I think,
which gcc compiles to 8 insns on power8 (and could be 7 insns if a
useless sign extension was eliminated).

int
bit_isnormal (double x)
{
  union { double d; uint64_t l; } val;
  val.d = x;
  uint64_t exp = (val.l >> 52) & 0x7ff;
  return exp - 1 < 0x7fe;
}

The above is around twice as fast as fold_builtin_interclass_mathfn
implementation of isnormal() for double, on power8.  I expect a bit
twiddling implementation for IBM extended would show similar or better
improvement.

However I'm not inclined to pursue this, especially for gcc-6.  The
patch I posted for isnormal() IBM extended is already faster (about
65% average timing on power8) than what existed previously.

-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH] PR70117, ppc long double isinf

[Alan Modra]

On Wed, Apr 06, 2016 at 12:27:36PM +0200, Andreas Schwab wrote:
> Alan Modra  writes:
> 
> > diff --git a/gcc/testsuite/gcc.target/powerpc/pr70117.c 
> > b/gcc/testsuite/gcc.target/powerpc/pr70117.c
> > new file mode 100644
> > index 000..99e6f19
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.target/powerpc/pr70117.c
> > @@ -0,0 +1,22 @@
> > +/* { dg-do run { target { { powerpc*-*-darwin* powerpc*-*-aix* rs6000-*-* 
> > } || { powerpc*-*-linux* && lp64 } } } } */
> 
> Any reason why it is restricted to lp64?

No, that was me copying from rs6000-ldouble-1.c without thinking.
We've had double-double on powerpc-linux 32-bit for quite a while.

-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH] PR70117, ppc long double isinf

[Andreas Schwab]

Alan Modra  writes:

> diff --git a/gcc/testsuite/gcc.target/powerpc/pr70117.c 
> b/gcc/testsuite/gcc.target/powerpc/pr70117.c
> new file mode 100644
> index 000..99e6f19
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/pr70117.c
> @@ -0,0 +1,22 @@
> +/* { dg-do run { target { { powerpc*-*-darwin* powerpc*-*-aix* rs6000-*-* } 
> || { powerpc*-*-linux* && lp64 } } } } */

Any reason why it is restricted to lp64?

Andreas.

-- 
Andreas Schwab, SUSE Labs, sch...@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH] PR70117, ppc long double isinf

[Alan Modra]

On Wed, Apr 06, 2016 at 10:46:48AM +0200, Richard Biener wrote:
> On Wed, Apr 6, 2016 at 10:31 AM, Alan Modra  wrote:
> > On Tue, Apr 05, 2016 at 11:29:30AM +0200, Richard Biener wrote:
> >> In general the patch looks like a good approach to me but can we
> >> hide that
> >>
> >> > +  const struct real_format *fmt = FLOAT_MODE_FORMAT (mode);
> >> > +  bool is_ibm_extended = fmt->pnan < fmt->p;
> >>
> >> in a function somewhere in real.[ch]?
> >
> > On looking in real.h, I see there is already a macro to do it.
> >
> > Here's the revised version that properly tests the long double
> > subnormal limit.  Bootstrapped and regression tested
> > powerpc64le-linux.
> 
> Can you add a testcase or two for the isnormal () case?

Sure.  I'll adapt the testcase I was using to verify the output,
attached in case you're interested.

> I wonder whether the isnormal tests are too excessive to put in
> inline code and thus libgcc code wouldn't be better to handle this...

Out-of-line would be better for -Os at least.

> At least the glibc implementation looks a lot simpler to me ...
> (if ./sysdeps/ieee754/ldbl-128ibm/s_fpclassifyl.c is the correct one).

It looks more or less the same to me, except done by bit twiddling on
integers.  :)

> Thus an alternative is to inline sth similar via the folding or via
> an optab and not folding (I'd prefer the latter).
> 
> That said, did you inspect the generated code for a isnormal (x)
> call for non-constant x?

Yes, I spent quite a bit of time fiddling trying to get optimal code.
I'm not claiming I succeeded..

>  What does XLC do here?

Not sure, sorry.  I don't have xlc handy.  Will try later.

-- 
Alan Modra
Australia Development Lab, IBM
int __attribute__ ((noclone, noinline))
isnormal (double x)
{
  return __builtin_isnormal (x);
}

int __attribute__ ((noclone, noinline))
isnormal_ld (long double x)
{
  return __builtin_isnormal (x);
}

double min_norm = 0x1p-1022;
double min_denorm = 0x1p-1074;
double ld_low = 0x1p-969;

int
main (void)
{
  static union { long double ld; unsigned long l[2]; } x;

  __builtin_printf ("%a %d\n", min_norm, isnormal (min_norm));
  __builtin_printf ("%a %d\n", min_norm * 0.5, isnormal (min_norm * 0.5));

  x.ld = ld_low;
  __builtin_printf ("%La (%016lx %016lx) %d\n", x.ld, x.l[0], x.l[1],
		isnormal_ld (x.ld));
  x.ld = -ld_low;
  __builtin_printf ("%La (%016lx %016lx) %d\n", x.ld, x.l[0], x.l[1],
		isnormal_ld (x.ld));
  x.ld = -min_norm * 0.5;
  x.ld += ld_low;
  __builtin_printf ("%La (%016lx %016lx) %d\n", x.ld, x.l[0], x.l[1],
		isnormal_ld (x.ld));
  x.ld = min_norm * 0.5;
  x.ld -= ld_low;
  __builtin_printf ("%La (%016lx %016lx) %d\n", x.ld, x.l[0], x.l[1],
		isnormal_ld (x.ld));
  x.ld = -min_norm;
  x.ld += ld_low;
  __builtin_printf ("%La (%016lx %016lx) %d\n", x.ld, x.l[0], x.l[1],
		isnormal_ld (x.ld));
  x.ld = min_norm;
  x.ld -= ld_low;
  __builtin_printf ("%La (%016lx %016lx) %d\n", x.ld, x.l[0], x.l[1],
		isnormal_ld (x.ld));
  x.ld = -min_denorm;
  x.ld += ld_low;
  __builtin_printf ("%La (%016lx %016lx) %d\n", x.ld, x.l[0], x.l[1],
		isnormal_ld (x.ld));
  x.ld = min_denorm;
  x.ld -= ld_low;
  __builtin_printf ("%La (%016lx %016lx) %d\n", x.ld, x.l[0], x.l[1],
		isnormal_ld (x.ld));
  x.ld = min_denorm;
  x.ld += ld_low;
  __builtin_printf ("%La (%016lx %016lx) %d\n", x.ld, x.l[0], x.l[1],
		isnormal_ld (x.ld));
  x.ld = -min_denorm;
  x.ld -= ld_low;
  __builtin_printf ("%La (%016lx %016lx) %d\n", x.ld, x.l[0], x.l[1],
		isnormal_ld (x.ld));
  return 0;
}

Re: [PATCH] PR70117, ppc long double isinf

[Richard Biener]

On Wed, Apr 6, 2016 at 10:31 AM, Alan Modra  wrote:
> On Tue, Apr 05, 2016 at 11:29:30AM +0200, Richard Biener wrote:
>> In general the patch looks like a good approach to me but can we
>> hide that
>>
>> > +  const struct real_format *fmt = FLOAT_MODE_FORMAT (mode);
>> > +  bool is_ibm_extended = fmt->pnan < fmt->p;
>>
>> in a function somewhere in real.[ch]?
>
> On looking in real.h, I see there is already a macro to do it.
>
> Here's the revised version that properly tests the long double
> subnormal limit.  Bootstrapped and regression tested
> powerpc64le-linux.

Can you add a testcase or two for the isnormal () case?

I wonder whether the isnormal tests are too excessive to put in
inline code and thus libgcc code wouldn't be better to handle this...

At least the glibc implementation looks a lot simpler to me ...
(if ./sysdeps/ieee754/ldbl-128ibm/s_fpclassifyl.c is the correct one).

Thus an alternative is to inline sth similar via the folding or via
an optab and not folding (I'd prefer the latter).

That said, did you inspect the generated code for a isnormal (x)
call for non-constant x?  What does XLC do here?

Richard.

> gcc/
> PR target/70117
> * builtins.c (fold_builtin_classify): For IBM extended precision,
> look at just the high-order double to test for NaN.
> (fold_builtin_interclass_mathfn): Similarly for Inf.  For isnormal
> test just the high double for Inf but both doubles for subnormal
> limit.
> gcc/testsuite/
> * gcc.target/powerpc/pr70117.c: New.
>
> diff --git a/gcc/builtins.c b/gcc/builtins.c
> index 9368ed0..9162838 100644
> --- a/gcc/builtins.c
> +++ b/gcc/builtins.c
> @@ -7529,6 +7529,8 @@ fold_builtin_interclass_mathfn (location_t loc, tree 
> fndecl, tree arg)
>
>mode = TYPE_MODE (TREE_TYPE (arg));
>
> +  bool is_ibm_extended = MODE_COMPOSITE_P (mode);
> +
>/* If there is no optab, try generic code.  */
>switch (DECL_FUNCTION_CODE (fndecl))
>  {
> @@ -7538,10 +7540,18 @@ fold_builtin_interclass_mathfn (location_t loc, tree 
> fndecl, tree arg)
>{
> /* isinf(x) -> isgreater(fabs(x),DBL_MAX).  */
> tree const isgr_fn = builtin_decl_explicit (BUILT_IN_ISGREATER);
> -   tree const type = TREE_TYPE (arg);
> +   tree type = TREE_TYPE (arg);
> REAL_VALUE_TYPE r;
> char buf[128];
>
> +   if (is_ibm_extended)
> + {
> +   /* NaN and Inf are encoded in the high-order double value
> +  only.  The low-order value is not significant.  */
> +   type = double_type_node;
> +   mode = DFmode;
> +   arg = fold_build1_loc (loc, NOP_EXPR, type, arg);
> + }
> get_max_float (REAL_MODE_FORMAT (mode), buf, sizeof (buf));
> real_from_string (, buf);
> result = build_call_expr (isgr_fn, 2,
> @@ -7554,10 +7564,18 @@ fold_builtin_interclass_mathfn (location_t loc, tree 
> fndecl, tree arg)
>{
> /* isfinite(x) -> islessequal(fabs(x),DBL_MAX).  */
> tree const isle_fn = builtin_decl_explicit (BUILT_IN_ISLESSEQUAL);
> -   tree const type = TREE_TYPE (arg);
> +   tree type = TREE_TYPE (arg);
> REAL_VALUE_TYPE r;
> char buf[128];
>
> +   if (is_ibm_extended)
> + {
> +   /* NaN and Inf are encoded in the high-order double value
> +  only.  The low-order value is not significant.  */
> +   type = double_type_node;
> +   mode = DFmode;
> +   arg = fold_build1_loc (loc, NOP_EXPR, type, arg);
> + }
> get_max_float (REAL_MODE_FORMAT (mode), buf, sizeof (buf));
> real_from_string (, buf);
> result = build_call_expr (isle_fn, 2,
> @@ -7577,21 +7595,72 @@ fold_builtin_interclass_mathfn (location_t loc, tree 
> fndecl, tree arg)
> /* isnormal(x) -> isgreaterequal(fabs(x),DBL_MIN) &
>islessequal(fabs(x),DBL_MAX).  */
> tree const isle_fn = builtin_decl_explicit (BUILT_IN_ISLESSEQUAL);
> -   tree const isge_fn = builtin_decl_explicit (BUILT_IN_ISGREATEREQUAL);
> -   tree const type = TREE_TYPE (arg);
> +   tree type = TREE_TYPE (arg);
> +   tree orig_arg, max_exp, min_exp;
> +   machine_mode orig_mode = mode;
> REAL_VALUE_TYPE rmax, rmin;
> char buf[128];
>
> +   orig_arg = arg = builtin_save_expr (arg);
> +   if (is_ibm_extended)
> + {
> +   /* Use double to test the normal range of IBM extended
> +  precision.  Emin for IBM extended precision is
> +  different to emin for IEEE double, being 53 higher
> +  since the low double exponent is at least 53 lower
> +  than the high double exponent.  */
> +   type = double_type_node;
> +   mode = DFmode;
> +   arg = fold_build1_loc (loc, NOP_EXPR, type, arg);
> + }
> +   arg = fold_build1_loc (loc, ABS_EXPR, type, arg);
> +
> get_max_float