Re: [PATCH] range-op-float: Further comparison fixes

[Aldy Hernandez <aldyh@redhat.com>]

On 3/31/23 12:45, Jakub Jelinek wrote:
> On Fri, Mar 31, 2023 at 09:57:54AM +0200, Jakub Jelinek via Gcc-patches wrote:
>> and so if maybe_isnan, they always return [0, 1].  Now, thinking about it,
>> this is unnecessary pessimization, for the case where the ... block
>> returns range_false (type) we actually could do it also if maybe_isnan (op1,
>> op2), because if one or both operands are NAN, the comparison will be false,
>> and if neither is NAN, the comparison will be also false.  Will fix
>> incrementally today.
> 
> Here it is.
> 1) the foperator_{,not_}equal::fold_range cases
>     - we have correctly first a case handling known_isnan on either operand,
>       return there range_false (type) - EQ, resp. range_true (type) - NE
>     - next we handle the singleton cases, maybe_isnan + singleton isn't
>       singleton, so these are ok
>     - there is a missing case (not handled in this patch) where both operands
>       are known to be zeros, but not singleton zeros
>     - there is some !maybe_isnan (op1, op2) handling which tries to prove
>       when the operands are certainly not equal and results in
>       range_false (type) - EQ, resp. range_true (type) - NE
>     - otherwise range_true_and_false (type)
>     Now, I think (and this patch implements it) that the !maybe_isnan (op1, op2)
>     check is unnecessary.  If we prove that when ignoring maybe NANs, the
>     ranges don't intersect and so the comparison is always false for EQ or
>     always true for NE, if NANs can appear, it doesn't change anything on
>     that either, for EQ if NAN appears, the result is false like what we
>     proved for the finite ranges, for NE if NAN appears, the result is true
>     like what we proved for the finite ranges
> 2) foperator_{lt,le,gt,ge}::fold_range cases
>     - these have correctly known_isnan on either operand first and return
>       there range_false (type)
>     - then !maybe_isnan (op1, op2) condition guarded checks
>     - finally range_true_and_false (type) - so do that for
>       maybe_isnan (op1, op2)
>     Here in the !maybe_isnan (op1, op2) guarded code we have some condition
>     which results in range_true (type), another condition which results in
>     range_false (type) and otherwise range_true_and_false (type).
>     Now, the condition which results in range_false (type) can be IMHO done
>     also for the maybe_isnan (op1, op2) cases, because it is [0, 0]
>     if both operands are finite or [0, 0] if either operand is NAN.
> 3) finally, LTGT_EXPR wasn't handled at all.  LTGT_EXPR is the inverse
>     comparision to UNEQ_EXPR, I believe both raise exceptions only if
>     either operand is sNaN, UNEQ_EXPR is true if both operands are
>     either equal or either of the operands is NAN, while LTGT_EXPR is
>     true if the operands compare either smaller or larger and is false if
>     either of the operands is NAN.
> 
> Ok for trunk if this passes bootstrap/regtest?

LGTM
Aldy

> 
> 2023-03-31  Jakub Jelinek  <jakub@redhat.com>
> 
> 	* range-op-float.cc (foperator_equal::fold_range): Perform the
> 	non-singleton handling regardless of maybe_isnan (op1, op2).
> 	(foperator_not_equal::fold_range): Likewise.
> 	(foperator_lt::fold_range, foperator_le::fold_range,
> 	foperator_gt::fold_range, foperator_ge::fold_range): Perform the
> 	real_* comparison check which results in range_false (type)
> 	even if maybe_isnan (op1, op2).  Simplify.
> 	(foperator_ltgt): New class.
> 	(fop_ltgt): New variable.
> 	(floating_op_table::floating_op_table): Handle LTGT_EXPR using
> 	fop_ltgt.
> 
> --- gcc/range-op-float.cc.jj	2023-03-31 09:30:11.245296618 +0200
> +++ gcc/range-op-float.cc	2023-03-31 11:23:04.817876083 +0200
> @@ -616,7 +616,7 @@ foperator_equal::fold_range (irange &r,
>         else
>   	r = range_false (type);
>       }
> -  else if (!maybe_isnan (op1, op2))
> +  else
>       {
>         // If ranges do not intersect, we know the range is not equal,
>         // otherwise we don't know anything for sure.
> @@ -638,8 +638,6 @@ foperator_equal::fold_range (irange &r,
>         else
>   	r = range_true_and_false (type);
>       }
> -  else
> -    r = range_true_and_false (type);
>     return true;
>   }
>   
> @@ -734,7 +732,7 @@ foperator_not_equal::fold_range (irange
>         else
>   	r = range_true (type);
>       }
> -  else if (!maybe_isnan (op1, op2))
> +  else
>       {
>         // If ranges do not intersect, we know the range is not equal,
>         // otherwise we don't know anything for sure.
> @@ -756,8 +754,6 @@ foperator_not_equal::fold_range (irange
>         else
>   	r = range_true_and_false (type);
>       }
> -  else
> -    r = range_true_and_false (type);
>     return true;
>   }
>   
> @@ -839,17 +835,13 @@ foperator_lt::fold_range (irange &r, tre
>     if (frelop_early_resolve (r, type, op1, op2, rel, VREL_LT))
>       return true;
>   
> -  if (op1.known_isnan () || op2.known_isnan ())
> +  if (op1.known_isnan ()
> +      || op2.known_isnan ()
> +      || !real_less (&op1.lower_bound (), &op2.upper_bound ()))
>       r = range_false (type);
> -  else if (!maybe_isnan (op1, op2))
> -    {
> -      if (real_less (&op1.upper_bound (), &op2.lower_bound ()))
> -	r = range_true (type);
> -      else if (!real_less (&op1.lower_bound (), &op2.upper_bound ()))
> -	r = range_false (type);
> -      else
> -	r = range_true_and_false (type);
> -    }
> +  else if (!maybe_isnan (op1, op2)
> +	   && real_less (&op1.upper_bound (), &op2.lower_bound ()))
> +    r = range_true (type);
>     else
>       r = range_true_and_false (type);
>     return true;
> @@ -959,17 +951,13 @@ foperator_le::fold_range (irange &r, tre
>     if (frelop_early_resolve (r, type, op1, op2, rel, VREL_LE))
>       return true;
>   
> -  if (op1.known_isnan () || op2.known_isnan ())
> +  if (op1.known_isnan ()
> +      || op2.known_isnan ()
> +      || !real_compare (LE_EXPR, &op1.lower_bound (), &op2.upper_bound ()))
>       r = range_false (type);
> -  else if (!maybe_isnan (op1, op2))
> -    {
> -      if (real_compare (LE_EXPR, &op1.upper_bound (), &op2.lower_bound ()))
> -	r = range_true (type);
> -      else if (!real_compare (LE_EXPR, &op1.lower_bound (), &op2.upper_bound ()))
> -	r = range_false (type);
> -      else
> -	r = range_true_and_false (type);
> -    }
> +  else if (!maybe_isnan (op1, op2)
> +	   && real_compare (LE_EXPR, &op1.upper_bound (), &op2.lower_bound ()))
> +    r = range_true (type);
>     else
>       r = range_true_and_false (type);
>     return true;
> @@ -1073,17 +1061,13 @@ foperator_gt::fold_range (irange &r, tre
>     if (frelop_early_resolve (r, type, op1, op2, rel, VREL_GT))
>       return true;
>   
> -  if (op1.known_isnan () || op2.known_isnan ())
> +  if (op1.known_isnan ()
> +      || op2.known_isnan ()
> +      || !real_compare (GT_EXPR, &op1.upper_bound (), &op2.lower_bound ()))
>       r = range_false (type);
> -  else if (!maybe_isnan (op1, op2))
> -    {
> -      if (real_compare (GT_EXPR, &op1.lower_bound (), &op2.upper_bound ()))
> -	r = range_true (type);
> -      else if (!real_compare (GT_EXPR, &op1.upper_bound (), &op2.lower_bound ()))
> -	r = range_false (type);
> -      else
> -	r = range_true_and_false (type);
> -    }
> +  else if (!maybe_isnan (op1, op2)
> +	   && real_compare (GT_EXPR, &op1.lower_bound (), &op2.upper_bound ()))
> +    r = range_true (type);
>     else
>       r = range_true_and_false (type);
>     return true;
> @@ -1197,17 +1181,13 @@ foperator_ge::fold_range (irange &r, tre
>     if (frelop_early_resolve (r, type, op1, op2, rel, VREL_GE))
>       return true;
>   
> -  if (op1.known_isnan () || op2.known_isnan ())
> +  if (op1.known_isnan ()
> +      || op2.known_isnan ()
> +      || !real_compare (GE_EXPR, &op1.upper_bound (), &op2.lower_bound ()))
>       r = range_false (type);
> -  else if (!maybe_isnan (op1, op2))
> -    {
> -      if (real_compare (GE_EXPR, &op1.lower_bound (), &op2.upper_bound ()))
> -	r = range_true (type);
> -      else if (!real_compare (GE_EXPR, &op1.upper_bound (), &op2.lower_bound ()))
> -	r = range_false (type);
> -      else
> -	r = range_true_and_false (type);
> -    }
> +  else if (!maybe_isnan (op1, op2)
> +	   && real_compare (GE_EXPR, &op1.lower_bound (), &op2.upper_bound ()))
> +    r = range_true (type);
>     else
>       r = range_true_and_false (type);
>     return true;
> @@ -2092,6 +2072,87 @@ foperator_unordered_equal::op1_range (fr
>     return true;
>   }
>   
> +class foperator_ltgt : public range_operator_float
> +{
> +  using range_operator_float::fold_range;
> +  using range_operator_float::op1_range;
> +  using range_operator_float::op2_range;
> +public:
> +  bool fold_range (irange &r, tree type,
> +		   const frange &op1, const frange &op2,
> +		   relation_trio rel = TRIO_VARYING) const final override
> +  {
> +    if (op1.known_isnan () || op2.known_isnan ())
> +      {
> +	r = range_false (type);
> +	return true;
> +      }
> +    frange op1_no_nan = op1;
> +    frange op2_no_nan = op2;
> +    if (op1.maybe_isnan ())
> +      op1_no_nan.clear_nan ();
> +    if (op2.maybe_isnan ())
> +      op2_no_nan.clear_nan ();
> +    if (!fop_not_equal.fold_range (r, type, op1_no_nan, op2_no_nan, rel))
> +      return false;
> +    // The result is the same as the ordered version when the
> +    // comparison is true or when the operands cannot be NANs.
> +    if (!maybe_isnan (op1, op2) || r == range_false (type))
> +      return true;
> +    else
> +      {
> +	r = range_true_and_false (type);
> +	return true;
> +      }
> +  }
> +  bool op1_range (frange &r, tree type,
> +		  const irange &lhs, const frange &op2,
> +		  relation_trio = TRIO_VARYING) const final override;
> +  bool op2_range (frange &r, tree type,
> +		  const irange &lhs, const frange &op1,
> +		  relation_trio rel = TRIO_VARYING) const final override
> +  {
> +    return op1_range (r, type, lhs, op1, rel.swap_op1_op2 ());
> +  }
> +} fop_ltgt;
> +
> +bool
> +foperator_ltgt::op1_range (frange &r, tree type,
> +			   const irange &lhs,
> +			   const frange &op2,
> +			   relation_trio) const
> +{
> +  switch (get_bool_state (r, lhs, type))
> +    {
> +    case BRS_TRUE:
> +      // A true LTGT means both operands are !NAN, so it's
> +      // impossible for op2 to be a NAN.
> +      if (op2.known_isnan ())
> +	r.set_undefined ();
> +      else
> +	{
> +	  // The true side indicates !NAN and not equal.  We can at least
> +	  // represent !NAN.
> +	  r.set_varying (type);
> +	  r.clear_nan ();
> +	}
> +      break;
> +
> +    case BRS_FALSE:
> +      // If it's false, the result is the same as OP2 plus a NAN.
> +      r = op2;
> +      // Add both zeros if there's the possibility of zero equality.
> +      frange_add_zeros (r, type);
> +      // Add the possibility of a NAN.
> +      r.update_nan ();
> +      break;
> +
> +    default:
> +      break;
> +    }
> +  return true;
> +}
> +
>   // Final tweaks for float binary op op1_range/op2_range.
>   // Return TRUE if the operation is performed and a valid range is available.
>   
> @@ -2767,6 +2828,7 @@ floating_op_table::floating_op_table ()
>     set (UNEQ_EXPR, fop_unordered_equal);
>     set (ORDERED_EXPR, fop_ordered);
>     set (UNORDERED_EXPR, fop_unordered);
> +  set (LTGT_EXPR, fop_ltgt);
>   
>     set (ABS_EXPR, fop_abs);
>     set (NEGATE_EXPR, fop_negate);
> 
> 
> 	Jakub
>