Re: [PATCH] range-op-float: Fix up -ffinite-math-only range extension and don't extend into infinities [PR109008]

[Richard Biener <rguenther@suse.de>]

On Fri, 10 Mar 2023, Jakub Jelinek wrote:

> Hi!
> 
> The following patch does two things (both related to range extension
> around the boundaries).
> 
> The first part (in the 2 real_isfinite blocks) is to make the ranges
> narrower when the old boundaries are minimum and/or maximum representable
> finite number.  In that case frange_nextafter gives -Inf or +Inf,
> but then the resulting computed reverse range is very far from the actually
> needed range, usually extends up to infinity or could even result in NaNs.
> While infinities are really the next representable numbers in the
> corresponding mode, REAL_VALUE_TYPE is actually a type with wider range
> for exponent and 160 bit precision, so the patch instead uses
> nextafter number in a hypothetical floating point format with the same
> mantissa precision but wider range of exponents.  This significantly
> improves the actual ranges of the reverse operations, while still making
> them conservatively correct.
> 
> The second part is a fix for miscompilation of the new testcase below.
> For -ffinite-math-only, without this patch we extend the minimum and/or
> maximum representable finite number to -Inf or +Inf, with the patch to
> some number outside of the normal exponent range of the mode, but then
> we use set which canonicalizes it and turns the boundaries back to
> the minimum and/or maximum representable finite numbers, but because
> in say [__DBL_MAX__, __DBL_MAX__] = op1 + [__DBL_MAX__, __DBL_MAX__]
> op1 can be larger than 0, up to the largest number which rounds to even
> down back to __DBL_MAX__ and there are still no infinities involved,
> it needs to work even with -ffinite-math-only.  So, we really need to
> widen the lhs range a little bit even in that case.  The patch does
> that through temporarily clearing -ffinite-math-only, such that the
> value with infinities or the outside of bounds values passes the
> setting and verification (the VR_VARYING case is needed because
> we get ICEs otherwise, but when lhs is VR_VARYING in -ffast-math,
> i.e. minimum to maximum representable finite and both signs of NaN,
> then set does all we need, we don't need to or in a NaN range).
> We don't really later use the range in a way that would become a problem
> that it is wider than varying, we actually just perform maths on the
> two boundaries.
> 
> As I said in the PR, this doesn't fix the !MODE_HAS_INFINITIES case,
> I believe we actually need to treat the boundary values as infinities
> in that case because they (probably) work like that, but it is unclear
> if it is just the reverse operation lhs widening that is a problem there,
> or whether it is a general problem.  I have zero experience with
> floating points without infinities (PDP11, some ARM half type?,
> what else?).
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> 
> 2023-03-10  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR tree-optimization/109008
> 	* range-op-float.cc (float_widen_lhs_range): If lb is
> 	minimum representable finite number or ub is maximum
> 	representable finite number, instead of widening it to
> 	-inf or inf widen it to negative or positive 0x0.8p+(EMAX+1).
> 	Temporarily clear flag_finite_math_only when canonicalizing
> 	the widened range.
> 
> 	* gcc.dg/pr109008.c: New test.
> 
> --- gcc/range-op-float.cc.jj	2023-03-09 09:54:53.880453046 +0100
> +++ gcc/range-op-float.cc	2023-03-09 20:52:07.456284507 +0100
> @@ -2217,12 +2217,42 @@ float_widen_lhs_range (tree type, const
>    REAL_VALUE_TYPE lb = lhs.lower_bound ();
>    REAL_VALUE_TYPE ub = lhs.upper_bound ();
>    if (real_isfinite (&lb))
> -    frange_nextafter (TYPE_MODE (type), lb, dconstninf);
> +    {
> +      frange_nextafter (TYPE_MODE (type), lb, dconstninf);
> +      if (real_isinf (&lb))
> +	{
> +	  /* For -DBL_MAX, instead of -Inf use
> +	     nexttoward (-DBL_MAX, -LDBL_MAX) in a hypothetical
> +	     wider type with the same mantissa precision but larger
> +	     exponent range; it is outside of range of double values,
> +	     but makes it clear it is just one ulp larger rather than
> +	     infinite amount larger.  */
> +	  lb = dconstm1;
> +	  SET_REAL_EXP (&lb, FLOAT_MODE_FORMAT (TYPE_MODE (type))->emax + 1);
> +	}
> +    }
>    if (real_isfinite (&ub))
> -    frange_nextafter (TYPE_MODE (type), ub, dconstinf);
> +    {
> +      frange_nextafter (TYPE_MODE (type), ub, dconstinf);
> +      if (real_isinf (&ub))
> +	{
> +	  /* For DBL_MAX similarly.  */
> +	  ub = dconst1;
> +	  SET_REAL_EXP (&ub, FLOAT_MODE_FORMAT (TYPE_MODE (type))->emax + 1);
> +	}
> +    }
> +  /* Temporarily disable -ffinite-math-only, so that frange::set doesn't
> +     reduce the range back to real_min_representable (type) as lower bound
> +     or real_max_representable (type) as upper bound.  */
> +  bool save_flag_finite_math_only = flag_finite_math_only;
> +  flag_finite_math_only = false;
>    ret.set (type, lb, ub);
> -  ret.clear_nan ();
> -  ret.union_ (lhs);
> +  if (lhs.kind () != VR_VARYING)
> +    {
> +      ret.clear_nan ();
> +      ret.union_ (lhs);
> +    }
> +  flag_finite_math_only = save_flag_finite_math_only;

Meh - I wonder if we can avoid all this by making float_widen_lhs_range
friend of frange and simply access m_min/m_max directly and use the
copy-CTOR to copy bounds and nan state ... after all verify_range
will likely fail after you restore flag_finite_math_only ...

But OK for the moment.

Thanks,
Richard.

>    return ret;
>  }
>  
> --- gcc/testsuite/gcc.dg/pr109008.c.jj	2023-03-09 12:25:11.507955698 +0100
> +++ gcc/testsuite/gcc.dg/pr109008.c	2023-03-09 12:33:35.795598344 +0100
> @@ -0,0 +1,26 @@
> +/* PR tree-optimization/109008 */
> +/* { dg-do run } */
> +/* { dg-options "-O2 -ffinite-math-only -fexcess-precision=standard" } */
> +
> +__attribute__((noipa)) double
> +foo (double eps)
> +{
> +  double d = __DBL_MAX__ + eps;
> +  if (d == __DBL_MAX__)
> +    if (eps > 16.0)
> +      return eps;
> +  return 0.0;
> +}
> +
> +int
> +main ()
> +{
> +#if __DBL_MANT_DIG__ == 53 && __DBL_MAX_EXP__ == 1024 && __DBL_MIN_EXP__ == -1021 \
> +    && __FLT_EVAL_METHOD__ == 0
> +  if (foo (0x0.8p+970) == 0.0)
> +    __builtin_abort ();
> +  if (foo (32.0) == 0.0)
> +    __builtin_abort ();
> +#endif
> +  return 0;
> +}
> 
> 	Jakub
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg,
Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman;
HRB 36809 (AG Nuernberg)