[COMMITTED] Drop -0.0 in frange::set() for !HONOR_SIGNED_ZEROS.

Similar to what we do for NANs when !HONOR_NANS and Inf when
flag_finite_math_only, we can remove -0.0 from the range at creation
time.

We were kinda sorta doing this because there is a bug in
real_isdenormal that is causing flush_denormals_to_zero to saturate
[x, -0.0] to [x, +0.0] when !HONOR_SIGNED_ZEROS.  Fixing this bug
(upcoming), causes us to leave -0.0 in places where we aren't
expecting it (the intersection code).

gcc/ChangeLog:

	* value-range.cc (frange::set): Drop -0.0 for !HONOR_SIGNED_ZEROS.
---
 gcc/value-range.cc | 8 ++++++++
 1 file changed, 8 insertions(+)

On Fri, Oct 14, 2022 at 04:26:50PM +0200, Aldy Hernandez via Gcc-patches wrote:
> Similar to what we do for NANs when !HONOR_NANS and Inf when
> flag_finite_math_only, we can remove -0.0 from the range at creation
> time.
> 
> We were kinda sorta doing this because there is a bug in
> real_isdenormal that is causing flush_denormals_to_zero to saturate
> [x, -0.0] to [x, +0.0] when !HONOR_SIGNED_ZEROS.  Fixing this bug
> (upcoming), causes us to leave -0.0 in places where we aren't
> expecting it (the intersection code).
> 
> gcc/ChangeLog:
> 
> 	* value-range.cc (frange::set): Drop -0.0 for !HONOR_SIGNED_ZEROS.

This looks wrong to me.
!HONOR_NANS is different from !HONOR_SIGNED_ZEROS.
The former says that either NaNs aren't supported or if they appear,
it will be UB.
The latter says that either -0.0 doesn't exist, or user doesn't care
if -0.0 or 0.0 is used.

So, what you do is ok for !MODE_HAS_SIGNED_ZEROS (TYPE_MODE (m_type)),
but otherwise we want to canonicalize [x, -0.0] to [x, 0.0] and
[0.0, y] to [-0.0, y].

> --- a/gcc/value-range.cc
> +++ b/gcc/value-range.cc
> @@ -324,6 +324,14 @@ frange::set (tree type,
>        m_neg_nan = false;
>      }
>  
> +  if (!HONOR_SIGNED_ZEROS (m_type))
> +    {
> +      if (real_iszero (&m_min, 1))
> +	m_min.sign = 0;
> +      if (real_iszero (&m_max, 1))
> +	m_max.sign = 0;
> +    }
> +
>    // For -ffinite-math-only we can drop ranges outside the
>    // representable numbers to min/max for the type.
>    if (flag_finite_math_only)
> -- 
> 2.37.3

	Jakub

On Fri, Oct 14, 2022 at 4:33 PM Jakub Jelinek <jakub@redhat.com> wrote:
>
> On Fri, Oct 14, 2022 at 04:26:50PM +0200, Aldy Hernandez via Gcc-patches wrote:
> > Similar to what we do for NANs when !HONOR_NANS and Inf when
> > flag_finite_math_only, we can remove -0.0 from the range at creation
> > time.
> >
> > We were kinda sorta doing this because there is a bug in
> > real_isdenormal that is causing flush_denormals_to_zero to saturate
> > [x, -0.0] to [x, +0.0] when !HONOR_SIGNED_ZEROS.  Fixing this bug
> > (upcoming), causes us to leave -0.0 in places where we aren't
> > expecting it (the intersection code).
> >
> > gcc/ChangeLog:
> >
> >       * value-range.cc (frange::set): Drop -0.0 for !HONOR_SIGNED_ZEROS.
>
> This looks wrong to me.
> !HONOR_NANS is different from !HONOR_SIGNED_ZEROS.
> The former says that either NaNs aren't supported or if they appear,
> it will be UB.
> The latter says that either -0.0 doesn't exist, or user doesn't care
> if -0.0 or 0.0 is used.
>
> So, what you do is ok for !MODE_HAS_SIGNED_ZEROS (TYPE_MODE (m_type)),
> but otherwise we want to canonicalize [x, -0.0] to [x, 0.0] and
> [0.0, y] to [-0.0, y].

If the user doesn't care, I would expect  they'd be ok with it being
+0.0, but I must say, this is way beyond my paygrade.

How does this patch in testing look?

Thanks.
Aldy

On Fri, Oct 14, 2022 at 04:53:13PM +0200, Aldy Hernandez wrote:
> > This looks wrong to me.
> > !HONOR_NANS is different from !HONOR_SIGNED_ZEROS.
> > The former says that either NaNs aren't supported or if they appear,
> > it will be UB.
> > The latter says that either -0.0 doesn't exist, or user doesn't care
> > if -0.0 or 0.0 is used.
> >
> > So, what you do is ok for !MODE_HAS_SIGNED_ZEROS (TYPE_MODE (m_type)),
> > but otherwise we want to canonicalize [x, -0.0] to [x, 0.0] and
> > [0.0, y] to [-0.0, y].
> 
> If the user doesn't care, I would expect  they'd be ok with it being
> +0.0, but I must say, this is way beyond my paygrade.

Unlike the NaN case where they can (easily) arrange for NaNs not to appear
(say, avoid numerically undefined operations), for +/-0 if the hardware
supports it they don't have much choice, sometimes computation will yield
one, sometimes the other.  The option is "I don't use anything that depends
on the zero sign, which is e.g. copysign from zero, signbit or poking at the
bit patterns".

> How does this patch in testing look?

LGTM (perhaps some comment would be nice though).

> From 045d57b979722d15ce7fce733616bbf4ab0e0459 Mon Sep 17 00:00:00 2001
> From: Aldy Hernandez <aldyh@redhat.com>
> Date: Fri, 14 Oct 2022 16:49:33 +0200
> Subject: [PATCH] Implement distinction between HONOR_SIGNED_ZEROS and
>  MODE_HAS_SIGNED_ZEROS.
> 
> gcc/ChangeLog:
> 
> 	* value-range.cc (frange::set): Implement distinction between
> 	HONOR_SIGNED_ZEROS and MODE_HAS_SIGNED_ZEROS.
> ---
>  gcc/value-range.cc | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/gcc/value-range.cc b/gcc/value-range.cc
> index 6b4f3dddcd5..e5b4c1565d4 100644
> --- a/gcc/value-range.cc
> +++ b/gcc/value-range.cc
> @@ -324,13 +324,20 @@ frange::set (tree type,
>        m_neg_nan = false;
>      }
>  
> -  if (!HONOR_SIGNED_ZEROS (m_type))
> +  if (!MODE_HAS_SIGNED_ZEROS (TYPE_MODE (m_type)))
>      {
>        if (real_iszero (&m_min, 1))
>  	m_min.sign = 0;
>        if (real_iszero (&m_max, 1))
>  	m_max.sign = 0;
>      }
> +  else if (!HONOR_SIGNED_ZEROS (m_type))
> +    {
> +      if (real_iszero (&m_max, 1))
> +	m_max.sign = 0;
> +      if (real_iszero (&m_min, 0))
> +	m_min.sign = 1;
> +    }
>  
>    // For -ffinite-math-only we can drop ranges outside the
>    // representable numbers to min/max for the type.
> -- 
> 2.37.3
> 


	Jakub

On Fri, Oct 14, 2022 at 5:00 PM Jakub Jelinek <jakub@redhat.com> wrote:
>
> On Fri, Oct 14, 2022 at 04:53:13PM +0200, Aldy Hernandez wrote:
> > > This looks wrong to me.
> > > !HONOR_NANS is different from !HONOR_SIGNED_ZEROS.
> > > The former says that either NaNs aren't supported or if they appear,
> > > it will be UB.
> > > The latter says that either -0.0 doesn't exist, or user doesn't care
> > > if -0.0 or 0.0 is used.
> > >
> > > So, what you do is ok for !MODE_HAS_SIGNED_ZEROS (TYPE_MODE (m_type)),
> > > but otherwise we want to canonicalize [x, -0.0] to [x, 0.0] and
> > > [0.0, y] to [-0.0, y].
> >
> > If the user doesn't care, I would expect  they'd be ok with it being
> > +0.0, but I must say, this is way beyond my paygrade.
>
> Unlike the NaN case where they can (easily) arrange for NaNs not to appear
> (say, avoid numerically undefined operations), for +/-0 if the hardware
> supports it they don't have much choice, sometimes computation will yield
> one, sometimes the other.  The option is "I don't use anything that depends
> on the zero sign, which is e.g. copysign from zero, signbit or poking at the
> bit patterns".
>
> > How does this patch in testing look?
>
> LGTM (perhaps some comment would be nice though).

Ughh, can I trouble you for one, cause frankly I'm a bit lost on this one?

Aldy