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

  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.


	Jakub
  

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
> 
>
  

On Fri, Mar 10, 2023 at 08:53:37AM +0000, Richard Biener wrote:
> 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 ...

I'll defer such changes to Aldy.

As for verification, I think verify_range will not fail on it, it mainly
checks whether it is normalized (e.g. if minimum is frange_val_min and
maximum is frange_val_max and NaNs are possible with both signs (if NaNs
are supported) then it is VR_VARYING etc.).  It doesn't check if the actual
VR_RANGE bounds are smaller or larger than the VR_VARYING bounds, there is
just equality check.
Of course, behavior of wider than varying ranges is still unexpected in
many ways, say the union_ of such a range and VR_VARYING will ICE etc.

Now, I guess another possibility for the reverse ops over these wider ranges
would be avoid calling fold_range in the reverse ops, but call rv_fold
directly or have fold_range variant which would instead of the op1, op2
argument have 2 triplets, op1, op1lb, op1ub, op2, op2lb, op2ub, and it
would use those const REAL_VALUE_TYPE &op??b in preference to
op?.{lower,upper}_bound () or perhaps normal fold_range be implemented
in terms of this extended fold_range.  Then we wouldn't need to bother with
these non-standard franges...

> But OK for the moment.

Thanks, committed.

	Jakub
  

On 3/10/23 11:29, Jakub Jelinek wrote:
> On Fri, Mar 10, 2023 at 08:53:37AM +0000, Richard Biener wrote:
>> 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 ...
> 
> I'll defer such changes to Aldy.
> 
> As for verification, I think verify_range will not fail on it, it mainly
> checks whether it is normalized (e.g. if minimum is frange_val_min and
> maximum is frange_val_max and NaNs are possible with both signs (if NaNs
> are supported) then it is VR_VARYING etc.).  It doesn't check if the actual
> VR_RANGE bounds are smaller or larger than the VR_VARYING bounds, there is
> just equality check.
> Of course, behavior of wider than varying ranges is still unexpected in
> many ways, say the union_ of such a range and VR_VARYING will ICE etc.
> 
> Now, I guess another possibility for the reverse ops over these wider ranges
> would be avoid calling fold_range in the reverse ops, but call rv_fold
> directly or have fold_range variant which would instead of the op1, op2
> argument have 2 triplets, op1, op1lb, op1ub, op2, op2lb, op2ub, and it
> would use those const REAL_VALUE_TYPE &op??b in preference to
> op?.{lower,upper}_bound () or perhaps normal fold_range be implemented
> in terms of this extended fold_range.  Then we wouldn't need to bother with
> these non-standard franges...
> 
>> But OK for the moment.
> 
> Thanks, committed.

I'm not a big fan of constructing ranges that break all our internal 
consistency checks.  I'd also prefer to add another constructor (or add 
a flag to the current constructor) instead of making range-op-float 
routines friends.  We also have bits in the vrange (or frange) that we 
could use for other semantics, especially since frange_storage can be 
streamlined when stored in GC/etc.

I'm on PTO this week.  Could we revisit this next week?  And if worse 
comes to worse, leave it as is, and fix it properly next release?

Thanks for your work on this.
Aldy
  

On Mon, 13 Mar 2023, Aldy Hernandez wrote:

> 
> 
> On 3/10/23 11:29, Jakub Jelinek wrote:
> > On Fri, Mar 10, 2023 at 08:53:37AM +0000, Richard Biener wrote:
> >> 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 ...
> > 
> > I'll defer such changes to Aldy.
> > 
> > As for verification, I think verify_range will not fail on it, it mainly
> > checks whether it is normalized (e.g. if minimum is frange_val_min and
> > maximum is frange_val_max and NaNs are possible with both signs (if NaNs
> > are supported) then it is VR_VARYING etc.).  It doesn't check if the actual
> > VR_RANGE bounds are smaller or larger than the VR_VARYING bounds, there is
> > just equality check.
> > Of course, behavior of wider than varying ranges is still unexpected in
> > many ways, say the union_ of such a range and VR_VARYING will ICE etc.
> > 
> > Now, I guess another possibility for the reverse ops over these wider ranges
> > would be avoid calling fold_range in the reverse ops, but call rv_fold
> > directly or have fold_range variant which would instead of the op1, op2
> > argument have 2 triplets, op1, op1lb, op1ub, op2, op2lb, op2ub, and it
> > would use those const REAL_VALUE_TYPE &op??b in preference to
> > op?.{lower,upper}_bound () or perhaps normal fold_range be implemented
> > in terms of this extended fold_range.  Then we wouldn't need to bother with
> > these non-standard franges...
> > 
> >> But OK for the moment.
> > 
> > Thanks, committed.
> 
> I'm not a big fan of constructing ranges that break all our internal
> consistency checks.  I'd also prefer to add another constructor (or add a flag
> to the current constructor) instead of making range-op-float routines friends.
> We also have bits in the vrange (or frange) that we could use for other
> semantics, especially since frange_storage can be streamlined when stored in
> GC/etc.
> 
> I'm on PTO this week.  Could we revisit this next week?  And if worse comes to
> worse, leave it as is, and fix it properly next release?

Yes, sure - I just noticed that we're forced to use high-level API for
something that's quite low-level and should be internal (a range 
"breaking" internal consistency checks).

Richard.
  

On 3/13/23 08:50, Richard Biener wrote:
> On Mon, 13 Mar 2023, Aldy Hernandez wrote:
> 
>>
>>
>> On 3/10/23 11:29, Jakub Jelinek wrote:
>>> On Fri, Mar 10, 2023 at 08:53:37AM +0000, Richard Biener wrote:
>>>> 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 ...
>>>
>>> I'll defer such changes to Aldy.
>>>
>>> As for verification, I think verify_range will not fail on it, it mainly
>>> checks whether it is normalized (e.g. if minimum is frange_val_min and
>>> maximum is frange_val_max and NaNs are possible with both signs (if NaNs
>>> are supported) then it is VR_VARYING etc.).  It doesn't check if the actual
>>> VR_RANGE bounds are smaller or larger than the VR_VARYING bounds, there is
>>> just equality check.
>>> Of course, behavior of wider than varying ranges is still unexpected in
>>> many ways, say the union_ of such a range and VR_VARYING will ICE etc.
>>>
>>> Now, I guess another possibility for the reverse ops over these wider ranges
>>> would be avoid calling fold_range in the reverse ops, but call rv_fold
>>> directly or have fold_range variant which would instead of the op1, op2
>>> argument have 2 triplets, op1, op1lb, op1ub, op2, op2lb, op2ub, and it
>>> would use those const REAL_VALUE_TYPE &op??b in preference to
>>> op?.{lower,upper}_bound () or perhaps normal fold_range be implemented
>>> in terms of this extended fold_range.  Then we wouldn't need to bother with
>>> these non-standard franges...
>>>
>>>> But OK for the moment.
>>>
>>> Thanks, committed.
>>
>> I'm not a big fan of constructing ranges that break all our internal
>> consistency checks.  I'd also prefer to add another constructor (or add a flag
>> to the current constructor) instead of making range-op-float routines friends.
>> We also have bits in the vrange (or frange) that we could use for other
>> semantics, especially since frange_storage can be streamlined when stored in
>> GC/etc.
>>
>> I'm on PTO this week.  Could we revisit this next week?  And if worse comes to
>> worse, leave it as is, and fix it properly next release?
> 
> Yes, sure - I just noticed that we're forced to use high-level API for
> something that's quite low-level and should be internal (a range
> "breaking" internal consistency checks).

Yeah, let's fix the API.  No sense hacking around things if what we need 
is to tweak the design.

I don't like hacking around things.  It always comes back to bite me ;-).

Aldy
  

On Mon, Mar 13, 2023 at 08:59:15AM +0100, Aldy Hernandez wrote:
> > Yes, sure - I just noticed that we're forced to use high-level API for
> > something that's quite low-level and should be internal (a range
> > "breaking" internal consistency checks).
> 
> Yeah, let's fix the API.  No sense hacking around things if what we need is
> to tweak the design.
> 
> I don't like hacking around things.  It always comes back to bite me ;-).

Sure.  The current state is that I think the actual bugs are fixed except
for the !MODE_HAS_INFINITIES case which people rarely use, so fixing up the
API can wait even to next release.

For !MODE_HAS_INFINITIES, I wonder if the best fix wouldn't be to change
set and a few other spots, so that if the boundaries are 
real_min_representable/real_max_representable, we widen them to -inf and inf
and change frange_val_min/max to also be dconstninf/dconstinf for
!MODE_HAS_INFINITIES, because the min/max for that case (probably) really work as
infinities.  Whenever we actually round that value to mode, it will become
real_min_representable/real_max_representable again.
But that can also wait a week.

	Jakub
  

On 3/13/23 09:06, Jakub Jelinek wrote:
> On Mon, Mar 13, 2023 at 08:59:15AM +0100, Aldy Hernandez wrote:
>>> Yes, sure - I just noticed that we're forced to use high-level API for
>>> something that's quite low-level and should be internal (a range
>>> "breaking" internal consistency checks).
>>
>> Yeah, let's fix the API.  No sense hacking around things if what we need is
>> to tweak the design.
>>
>> I don't like hacking around things.  It always comes back to bite me ;-).
> 
> Sure.  The current state is that I think the actual bugs are fixed except
> for the !MODE_HAS_INFINITIES case which people rarely use, so fixing up the
> API can wait even to next release.
> 
> For !MODE_HAS_INFINITIES, I wonder if the best fix wouldn't be to change
> set and a few other spots, so that if the boundaries are
> real_min_representable/real_max_representable, we widen them to -inf and inf
> and change frange_val_min/max to also be dconstninf/dconstinf for
> !MODE_HAS_INFINITIES, because the min/max for that case (probably) really work as
> infinities.  Whenever we actually round that value to mode, it will become
> real_min_representable/real_max_representable again.
> But that can also wait a week.

That sounds very reasonable.  It would remove special casing and would 
make the code easier to read.  For that matter, that was what I had in 
the original implementation.

Aldy
  

On Mon, Mar 13, 2023 at 09:41:47AM +0100, Aldy Hernandez wrote:
> On 3/13/23 09:06, Jakub Jelinek wrote:
> > On Mon, Mar 13, 2023 at 08:59:15AM +0100, Aldy Hernandez wrote:
> > > > Yes, sure - I just noticed that we're forced to use high-level API for
> > > > something that's quite low-level and should be internal (a range
> > > > "breaking" internal consistency checks).
> > > 
> > > Yeah, let's fix the API.  No sense hacking around things if what we need is
> > > to tweak the design.
> > > 
> > > I don't like hacking around things.  It always comes back to bite me ;-).
> > 
> > Sure.  The current state is that I think the actual bugs are fixed except
> > for the !MODE_HAS_INFINITIES case which people rarely use, so fixing up the
> > API can wait even to next release.
> > 
> > For !MODE_HAS_INFINITIES, I wonder if the best fix wouldn't be to change
> > set and a few other spots, so that if the boundaries are
> > real_min_representable/real_max_representable, we widen them to -inf and inf
> > and change frange_val_min/max to also be dconstninf/dconstinf for
> > !MODE_HAS_INFINITIES, because the min/max for that case (probably) really work as
> > infinities.  Whenever we actually round that value to mode, it will become
> > real_min_representable/real_max_representable again.
> > But that can also wait a week.
> 
> That sounds very reasonable.  It would remove special casing and would make
> the code easier to read.  For that matter, that was what I had in the
> original implementation.

I think we don't want to remove the special casing for -ffinite-math-only
on types which do support infinities.
Thinking further on it, perhaps for !MODE_HAS_INFINITIES a better fix would
be to do something like the patch below.
Consider say having a range of VAX float type:
#define M0 -FLT_MAX
#define M1 nextafterf (F0, FLT_MAX)
#define M2 nextafterf (M1, FLT_MAX)
[M2, M2] - [M0, M1]
Or perhaps if one or both of the operands are in such a case a min and max,
perform real_arithmetic recurse on the argument replaced with
dconstninf/dconstinf and then depending on inf pick the mininum or maximum
of the two results (and carefully think about what to do if both operands
are min/max).

2023-03-20  Jakub Jelinek  <jakub@redhat.com>

	* range-op-float.cc (frange_arithmetic): For !MODE_HAS_INFINITIES
	types, pretend operands with minimum or maximum values are actually
	infinities.

--- gcc/range-op-float.cc.jj	2023-03-10 12:40:19.673108938 +0100
+++ gcc/range-op-float.cc	2023-03-20 16:58:36.604981486 +0100
@@ -313,8 +313,26 @@ frange_arithmetic (enum tree_code code,
   REAL_VALUE_TYPE value;
   enum machine_mode mode = TYPE_MODE (type);
   bool mode_composite = MODE_COMPOSITE_P (mode);
+  const REAL_VALUE_TYPE *pop1 = &op1;
+  const REAL_VALUE_TYPE *pop2 = &op2;
 
-  bool inexact = real_arithmetic (&value, code, &op1, &op2);
+  if (!MODE_HAS_INFINITIES (mode))
+    {
+      // If mode doesn't have infinities, the minimum and maximum
+      // values are saturating.  Pretend for real_arithmetic such
+      // values are actual infinities.  real_convert will then
+      // canonicalize the result not to be an infinity.
+      if (frange_val_is_min (op1, type))
+	pop1 = &dconstninf;
+      else if (frange_val_is_max (op1, type))
+	pop1 = &dconstinf;
+      if (frange_val_is_min (op2, type))
+	pop2 = &dconstninf;
+      else if (frange_val_is_max (op2, type))
+	pop2 = &dconstinf;
+    }
+
+  bool inexact = real_arithmetic (&value, code, pop1, pop2);
   real_convert (&result, mode, &value);
 
   // Be extra careful if there may be discrepancies between the


	Jakub
  

On Mon, Mar 20, 2023 at 5:14 PM Jakub Jelinek <jakub@redhat.com> wrote:
>
> On Mon, Mar 13, 2023 at 09:41:47AM +0100, Aldy Hernandez wrote:
> > On 3/13/23 09:06, Jakub Jelinek wrote:
> > > On Mon, Mar 13, 2023 at 08:59:15AM +0100, Aldy Hernandez wrote:
> > > > > Yes, sure - I just noticed that we're forced to use high-level API for
> > > > > something that's quite low-level and should be internal (a range
> > > > > "breaking" internal consistency checks).
> > > >
> > > > Yeah, let's fix the API.  No sense hacking around things if what we need is
> > > > to tweak the design.
> > > >
> > > > I don't like hacking around things.  It always comes back to bite me ;-).
> > >
> > > Sure.  The current state is that I think the actual bugs are fixed except
> > > for the !MODE_HAS_INFINITIES case which people rarely use, so fixing up the
> > > API can wait even to next release.
> > >
> > > For !MODE_HAS_INFINITIES, I wonder if the best fix wouldn't be to change
> > > set and a few other spots, so that if the boundaries are
> > > real_min_representable/real_max_representable, we widen them to -inf and inf
> > > and change frange_val_min/max to also be dconstninf/dconstinf for
> > > !MODE_HAS_INFINITIES, because the min/max for that case (probably) really work as
> > > infinities.  Whenever we actually round that value to mode, it will become
> > > real_min_representable/real_max_representable again.
> > > But that can also wait a week.
> >
> > That sounds very reasonable.  It would remove special casing and would make
> > the code easier to read.  For that matter, that was what I had in the
> > original implementation.
>
> I think we don't want to remove the special casing for -ffinite-math-only
> on types which do support infinities.
> Thinking further on it, perhaps for !MODE_HAS_INFINITIES a better fix would
> be to do something like the patch below.
> Consider say having a range of VAX float type:
> #define M0 -FLT_MAX
> #define M1 nextafterf (F0, FLT_MAX)
> #define M2 nextafterf (M1, FLT_MAX)
> [M2, M2] - [M0, M1]
> Or perhaps if one or both of the operands are in such a case a min and max,
> perform real_arithmetic recurse on the argument replaced with
> dconstninf/dconstinf and then depending on inf pick the mininum or maximum
> of the two results (and carefully think about what to do if both operands
> are min/max).

LGTM.
Aldy

>
> 2023-03-20  Jakub Jelinek  <jakub@redhat.com>
>
>         * range-op-float.cc (frange_arithmetic): For !MODE_HAS_INFINITIES
>         types, pretend operands with minimum or maximum values are actually
>         infinities.
>
> --- gcc/range-op-float.cc.jj    2023-03-10 12:40:19.673108938 +0100
> +++ gcc/range-op-float.cc       2023-03-20 16:58:36.604981486 +0100
> @@ -313,8 +313,26 @@ frange_arithmetic (enum tree_code code,
>    REAL_VALUE_TYPE value;
>    enum machine_mode mode = TYPE_MODE (type);
>    bool mode_composite = MODE_COMPOSITE_P (mode);
> +  const REAL_VALUE_TYPE *pop1 = &op1;
> +  const REAL_VALUE_TYPE *pop2 = &op2;
>
> -  bool inexact = real_arithmetic (&value, code, &op1, &op2);
> +  if (!MODE_HAS_INFINITIES (mode))
> +    {
> +      // If mode doesn't have infinities, the minimum and maximum
> +      // values are saturating.  Pretend for real_arithmetic such
> +      // values are actual infinities.  real_convert will then
> +      // canonicalize the result not to be an infinity.
> +      if (frange_val_is_min (op1, type))
> +       pop1 = &dconstninf;
> +      else if (frange_val_is_max (op1, type))
> +       pop1 = &dconstinf;
> +      if (frange_val_is_min (op2, type))
> +       pop2 = &dconstninf;
> +      else if (frange_val_is_max (op2, type))
> +       pop2 = &dconstinf;
> +    }
> +
> +  bool inexact = real_arithmetic (&value, code, pop1, pop2);
>    real_convert (&result, mode, &value);
>
>    // Be extra careful if there may be discrepancies between the
>
>
>         Jakub
>
  

On 3/10/23 09:53, Richard Biener wrote:
> On Fri, 10 Mar 2023, Jakub Jelinek wrote:

Coming back to this...

>   /* 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);
>   if (lhs.kind () != VR_VARYING)
>     {
>       ret.clear_nan ();
>       ret.union_ (lhs);
>     }
>   flag_finite_math_only = save_flag_finite_math_only;

It looks like what you want to do is be able to create a range with a 
known NAN state, but without the setter reducing the range to 
min/max_representable.

How about we enhance the API to provide:

1. Constructor with a known NAN state.
2. Setter with a flag to keep it from canonicalizing into 
min/max_representable.

The flag in 2 could in the future be saved in the frange object to keep 
union and friends from further canonicalization.

So the above could be written as:

	// Construct [lb, ub] with a known NAN state.
	frange tmp (lb, ub, lhs.get_nan_state ());

	// Set RET without dropping/reducing the range to MIN/MAX.
	ret.set (tmp, FRANGE_FLAG_NO_REPRESENTABLE_REDUCTION);

An alternative is to allow the setter to set everything:

	ret.set (type, lb, ub,
		lhs.get_nan_state (),
		FRANGE_FLAG_NO_REPRESENTABLE_REDUCTION);

Would this work?  I'd be happy to whip up something this week, or if 
preferred, leave it to the next release.

Aldy
  

On Tue, Mar 21, 2023 at 02:28:31PM +0100, Aldy Hernandez wrote:
> >   /* 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);
> >   if (lhs.kind () != VR_VARYING)
> >     {
> >       ret.clear_nan ();
> >       ret.union_ (lhs);
> >     }
> >   flag_finite_math_only = save_flag_finite_math_only;
> 
> It looks like what you want to do is be able to create a range with a known
> NAN state, but without the setter reducing the range to
> min/max_representable.
> 
> How about we enhance the API to provide:
> 
> 1. Constructor with a known NAN state.
> 2. Setter with a flag to keep it from canonicalizing into
> min/max_representable.
> 
> The flag in 2 could in the future be saved in the frange object to keep
> union and friends from further canonicalization.
> 
> So the above could be written as:
> 
> 	// Construct [lb, ub] with a known NAN state.
> 	frange tmp (lb, ub, lhs.get_nan_state ());
> 
> 	// Set RET without dropping/reducing the range to MIN/MAX.
> 	ret.set (tmp, FRANGE_FLAG_NO_REPRESENTABLE_REDUCTION);
> 
> An alternative is to allow the setter to set everything:
> 
> 	ret.set (type, lb, ub,
> 		lhs.get_nan_state (),
> 		FRANGE_FLAG_NO_REPRESENTABLE_REDUCTION);
> 
> Would this work?  I'd be happy to whip up something this week, or if
> preferred, leave it to the next release.

The latter would be better, I really don't need a temporary range in that
spot, the union_ is only to copy the NaN state.
Though, I think right now set actually doesn't do reduction to representable
at all, all it does is equality compare bounds against the applicable
boundaries and if both are equal and NaN state is appropriate, change it
into VR_VARYING.
So maybe for now all we need is the 4 argument set.

	Jakub
  

On 3/21/23 14:39, Jakub Jelinek wrote:
> On Tue, Mar 21, 2023 at 02:28:31PM +0100, Aldy Hernandez wrote:
>>>    /* 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);
>>>    if (lhs.kind () != VR_VARYING)
>>>      {
>>>        ret.clear_nan ();
>>>        ret.union_ (lhs);
>>>      }
>>>    flag_finite_math_only = save_flag_finite_math_only;
>>
>> It looks like what you want to do is be able to create a range with a known
>> NAN state, but without the setter reducing the range to
>> min/max_representable.
>>
>> How about we enhance the API to provide:
>>
>> 1. Constructor with a known NAN state.
>> 2. Setter with a flag to keep it from canonicalizing into
>> min/max_representable.
>>
>> The flag in 2 could in the future be saved in the frange object to keep
>> union and friends from further canonicalization.
>>
>> So the above could be written as:
>>
>> 	// Construct [lb, ub] with a known NAN state.
>> 	frange tmp (lb, ub, lhs.get_nan_state ());
>>
>> 	// Set RET without dropping/reducing the range to MIN/MAX.
>> 	ret.set (tmp, FRANGE_FLAG_NO_REPRESENTABLE_REDUCTION);
>>
>> An alternative is to allow the setter to set everything:
>>
>> 	ret.set (type, lb, ub,
>> 		lhs.get_nan_state (),
>> 		FRANGE_FLAG_NO_REPRESENTABLE_REDUCTION);
>>
>> Would this work?  I'd be happy to whip up something this week, or if
>> preferred, leave it to the next release.
> 
> The latter would be better, I really don't need a temporary range in that
> spot, the union_ is only to copy the NaN state.
> Though, I think right now set actually doesn't do reduction to representable
> at all, all it does is equality compare bounds against the applicable
> boundaries and if both are equal and NaN state is appropriate, change it
> into VR_VARYING.
> So maybe for now all we need is the 4 argument set.

So, this?

frange::set (tree type,
	const REAL_VALUE_TYPE &min,
	const REAL_VALUE_TYPE &max,
	const nan_state &,
	value_range_kind kind = VR_RANGE)

If so, I'll start poking at it.

Aldy
  

On Tue, Mar 21, 2023 at 02:49:49PM +0100, Aldy Hernandez wrote:
> So, this?
> 
> frange::set (tree type,
> 	const REAL_VALUE_TYPE &min,
> 	const REAL_VALUE_TYPE &max,
> 	const nan_state &,
> 	value_range_kind kind = VR_RANGE)
> 
> If so, I'll start poking at it.

Yes.

	Jakub
  

On 3/21/23 14:56, Jakub Jelinek wrote:
> On Tue, Mar 21, 2023 at 02:49:49PM +0100, Aldy Hernandez wrote:
>> So, this?
>>
>> frange::set (tree type,
>> 	const REAL_VALUE_TYPE &min,
>> 	const REAL_VALUE_TYPE &max,
>> 	const nan_state &,
>> 	value_range_kind kind = VR_RANGE)
>>
>> If so, I'll start poking at it.
> 
> Yes.
> 
> 	Jakub
> 

Tested on x86-64 Linux.

OK?
Aldy
  

On Wed, Mar 22, 2023 at 07:32:44AM +0100, Aldy Hernandez wrote:
> On 3/21/23 14:56, Jakub Jelinek wrote:
> > On Tue, Mar 21, 2023 at 02:49:49PM +0100, Aldy Hernandez wrote:
> > > So, this?
> > > 
> > > frange::set (tree type,
> > > 	const REAL_VALUE_TYPE &min,
> > > 	const REAL_VALUE_TYPE &max,
> > > 	const nan_state &,
> > > 	value_range_kind kind = VR_RANGE)
> > > 
> > > If so, I'll start poking at it.
> > 
> > Yes.
> > 
> > 	Jakub
> > 
> 
> Tested on x86-64 Linux.
> 
> OK?

Yes, thanks.

	Jakub