Re: [PATCH] frange: flush denormals to zero for -funsafe-math-optimizations.

[Richard Biener <richard.guenther@gmail.com>]

On Mon, Sep 19, 2022 at 3:04 PM Aldy Hernandez <aldyh@redhat.com> wrote:
>
> On Mon, Sep 19, 2022 at 9:37 AM Richard Biener
> <richard.guenther@gmail.com> wrote:
> >
> > On Sat, Sep 17, 2022 at 10:25 AM Aldy Hernandez via Gcc-patches
> > <gcc-patches@gcc.gnu.org> wrote:
> > >
> > > Jakub has mentioned that for -funsafe-math-optimizations we may flush
> > > denormals to zero, in which case we need to be careful to extend the
> > > ranges to the appropriate zero.  This patch does exactly that.  For a
> > > range of [x, -DENORMAL] we flush to [x, -0.0] and for [+DENORMAL, x]
> > > we flush to [+0.0, x].
> > >
> > > It is unclear whether we should do this for Alpha, since I believe
> > > flushing to zero is the default, and the port requires -mieee for IEEE
> > > sanity.  If so, perhaps we should add a target hook so backends are
> > > free to request flushing to zero.
> > >
> > > Thoughts?
> >
> > I'm not sure what the intention of this is - it effectively results in
> > more conservative ranges for -funsafe-math-optimizations.  That is,
> > if -funsafe-math-optimizations says that denormals don't exist and
> > are all zero then doesn't that mean we can instead use the smalles
> > non-denormal number less than (or greater than) zero here?
> >
> > That said, the flushing you do is also "valid" for
> > -fno-unsafe-math-optimizations
> > in case we don't want to deal with denormals in range boundaries.
> >
> > It might also be a correctness requirement in case we don't know how
> > targets handle denormals (IIRC even OS defaults might matter here) so
> > we conservatively have to treat them as being flushed to zero.
>
> Actually, rth suggested we always flush to zero because we don't know
> what the target would do.  Again, I'm happy to do whatever you agree
> on.  I have no opinion.  My main goal here is correctness.

Yes, I think we should do this.

> >
> > So ... if we want to be on the "safe" side then please always do this.
> >
> > At the same point you could do
> >
> >  if (!HONOR_SIGNED_ZEROS ())
> >    if (real_iszero (&m_max))
> >      {
> >         if (real_iszero (&m_min))
> >            m.min.sign = 1;
> >         m_max.sign = 1;
> >      }
>
> But wouldn't that set [-0.0, -0.0] when encountering [+0, +0] ??

Yeah, that's my laziness not adding a special case for m_min == m_max.

>
> >    else if (real_iszero (&m_min))
> >      m_min.sign = 0;
>
> Jakub actually suggested something completely different...just set +0
> always for !HONOR_SIGNED_ZEROS.

Hmm, but the [-1, -0.] with known sign becomes [-1, +0.] with unknown sign?

Richard.

> Aldy
>
> >
> > so that we canonicalize a zero bound so that the sign is known for a range.
> >
> > Richard.
> >
> > > gcc/ChangeLog:
> > >
> > >         * value-range.cc (frange::flush_denormals_to_zero): New.
> > >         (frange::set): Call flush_denormals_to_zero.
> > >         * value-range.h (class frange): Add flush_denormals_to_zero.
> > > ---
> > >  gcc/value-range.cc | 24 ++++++++++++++++++++++++
> > >  gcc/value-range.h  |  1 +
> > >  2 files changed, 25 insertions(+)
> > >
> > > diff --git a/gcc/value-range.cc b/gcc/value-range.cc
> > > index 67d5d7fa90f..f285734f0e0 100644
> > > --- a/gcc/value-range.cc
> > > +++ b/gcc/value-range.cc
> > > @@ -267,6 +267,26 @@ tree_compare (tree_code code, tree op1, tree op2)
> > >    return !integer_zerop (fold_build2 (code, integer_type_node, op1, op2));
> > >  }
> > >
> > > +// Flush denormal endpoints to the appropriate 0.0.
> > > +
> > > +void
> > > +frange::flush_denormals_to_zero ()
> > > +{
> > > +  if (undefined_p () || known_isnan ())
> > > +    return;
> > > +
> > > +  // Flush [x, -DENORMAL] to [x, -0.0].
> > > +  if (real_isdenormal (&m_max) && real_isneg (&m_max))
> > > +    {
> > > +      m_max = dconst0;
> > > +      if (HONOR_SIGNED_ZEROS (m_type))
> > > +       m_max.sign = 1;
> > > +    }
> > > +  // Flush [+DENORMAL, x] to [+0.0, x].
> > > +  if (real_isdenormal (&m_min) && !real_isneg (&m_min))
> > > +    m_min = dconst0;
> > > +}
> > > +
> > >  // Setter for franges.
> > >
> > >  void
> > > @@ -317,6 +337,10 @@ frange::set (tree min, tree max, value_range_kind kind)
> > >    gcc_checking_assert (tree_compare (LE_EXPR, min, max));
> > >
> > >    normalize_kind ();
> > > +
> > > +  if (flag_unsafe_math_optimizations)
> > > +    flush_denormals_to_zero ();
> > > +
> > >    if (flag_checking)
> > >      verify_range ();
> > >  }
> > > diff --git a/gcc/value-range.h b/gcc/value-range.h
> > > index 3a401f3e4e2..795b1f00fdc 100644
> > > --- a/gcc/value-range.h
> > > +++ b/gcc/value-range.h
> > > @@ -327,6 +327,7 @@ private:
> > >    bool union_nans (const frange &);
> > >    bool intersect_nans (const frange &);
> > >    bool combine_zeros (const frange &, bool union_p);
> > > +  void flush_denormals_to_zero ();
> > >
> > >    tree m_type;
> > >    REAL_VALUE_TYPE m_min;
> > > --
> > > 2.37.1
> > >
> >
>