Re: [PATCH] range-op: Implement op[12]_range operators for {PLUS,MINUS,MULT,RDIV}_EXPR

[Aldy Hernandez <aldyh@redhat.com>]

On 11/11/22 12:50, Jakub Jelinek wrote:
> On Wed, Nov 09, 2022 at 04:43:56PM +0100, Aldy Hernandez wrote:
>> On Wed, Nov 9, 2022 at 3:58 PM Jakub Jelinek <jakub@redhat.com> wrote:
>>>
>>> On Wed, Nov 09, 2022 at 10:02:46AM +0100, Aldy Hernandez wrote:
>>>> We can implement the op[12]_range entries for plus and minus in terms
>>>> of each other.  These are adapted from the integer versions.
>>>
>>> I think for NANs the op[12]_range shouldn't act this way.
>>> For the forward binary operations, we have the (maybe/known) NAN handling
>>> of one or both NAN operands resulting in VARYING sign (maybe/known) NAN
>>> result, that is the somehow the case for the reverse binary operations too,
>>> if result is (maybe/known) NAN and the other op is not NAN, op is
>>> VARYING sign (maybe/known) NAN, if other op is (maybe/known) NAN,
>>> then op is VARYING sign maybe NAN (always maybe, never known).
>>> But then for + we have the -INF + INF or vice versa into NAN, and that
>>> is something that shouldn't be considered.  If result isn't NAN, then
>>> neither operand can be NAN, regardless of whether result can be
>>> +/- INF and the other op -/+ INF.
>>
>> Heh.  I just ran into this while debugging the problem reported by Xi.
>>
>> We are solving NAN = op1 - VARYING, and trying to do it with op1 = NAN
>> + VARYING, which returns op1 = NAN (incorrectly).
>>
>> I suppose in the above case op1 should ideally be
>> [-INF,-INF][+INF,+INF]+-NAN, but since we can't represent that then
>> [-INF,+INF] +-NAN, which is actually VARYING.  Do you agree?
>>
>> I'm reverting this patch as attached, while I sort this out.
> 
> Here is my (so far only on the testcase tested) patch which reinstalls
> your change, add the fixups I've talked about and also hooks up
> reverse operators for MULT_EXPR/RDIV_EXPR.

OMG, you're a rockstar (or salsa or bachata star if that's your thing)! 
:-P).

Thank you so much.  I was just looking at that now.

> 
> 2022-11-11  Aldy Hernandez  <aldyh@redhat.com>
> 	    Jakub Jelinek  <jakub@redhat.com>
> 
> 	* range-op-float.cc (float_binary_op_range_finish): New function.
> 	(foperator_plus::op1_range): New.
>          (foperator_plus::op2_range): New.
>          (foperator_minus::op1_range): New.
>          (foperator_minus::op2_range): New.
> 	(foperator_mult::op1_range): New.
>          (foperator_mult::op2_range): New.
>          (foperator_div::op1_range): New.
>          (foperator_div::op2_range): New.
> 
> 	* gcc.c-torture/execute/ieee/inf-4.c: New test.
> 
> --- gcc/range-op-float.cc.jj	2022-11-11 10:55:57.602617289 +0100
> +++ gcc/range-op-float.cc	2022-11-11 12:32:19.378633983 +0100
> @@ -1861,8 +1861,64 @@ foperator_unordered_equal::op1_range (fr
>     return true;
>   }
>   
> +// Final tweaks for float binary op op1_range/op2_range.
> +
> +static bool
> +float_binary_op_range_finish (bool ret, frange &r, tree type,
> +			      const frange &lhs)
> +{

Can you document the return value, even if it's just "the same as 
op1/2_range" ;-).

> +  if (!ret)
> +    return ret;
> +
> +  // If we get a known NAN from reverse op, it means either that
> +  // the other operand was known NAN (in that case we know nothing),
> +  // or the reverse operation introduced a known NAN.
> +  // Say for lhs = op1 * op2 if lhs is [-0, +0] and op2 is too,
> +  // 0 / 0 is known NAN.  Just punt in that case.
> +  // Or if lhs is a known NAN, we also don't know anything.
> +  if (r.known_isnan () || lhs.known_isnan ())
> +    {
> +      r.set_varying (type);
> +      return false;
> +    }

A return of false means the operation is not handled, similar to what 
the default operators defined at the top of range-op*.cc do.  The caller 
(gori?) is free to disregard the range altogether.  In practice this 
means VARYING, so you're getting the same behavior.  But you should 
probably return TRUE, which is what we do in other operators. 
Technically you could also not set "r" and just return false.

Otherwise LGTM.

I'll look at your other patches next.
Aldy

> +
> +  // If lhs isn't NAN, then neither operand could be NAN,
> +  // even if the reverse operation does introduce a maybe_nan.
> +  if (!lhs.maybe_isnan ())
> +    r.clear_nan ();
> +  // If lhs is a maybe or known NAN, the operand could be
> +  // NAN.
> +  else
> +    r.update_nan ();
> +  return true;
> +}
> +
>   class foperator_plus : public range_operator_float
>   {
> +  using range_operator_float::op1_range;
> +  using range_operator_float::op2_range;
> +public:
> +  virtual bool op1_range (frange &r, tree type,
> +			  const frange &lhs,
> +			  const frange &op2,
> +			  relation_trio = TRIO_VARYING) const final override
> +  {
> +    if (lhs.undefined_p ())
> +      return false;
> +    range_op_handler minus (MINUS_EXPR, type);
> +    if (!minus)
> +      return false;
> +    return float_binary_op_range_finish (minus.fold_range (r, type, lhs, op2),
> +					 r, type, lhs);
> +  } > +  virtual bool op2_range (frange &r, tree type,
> +			  const frange &lhs,
> +			  const frange &op1,
> +			  relation_trio = TRIO_VARYING) const final override
> +  {
> +    return op1_range (r, type, lhs, op1);
> +  }
> +private:
>     void rv_fold (REAL_VALUE_TYPE &lb, REAL_VALUE_TYPE &ub, bool &maybe_nan,
>   		tree type,
>   		const REAL_VALUE_TYPE &lh_lb,
> @@ -1888,6 +1944,31 @@ class foperator_plus : public range_oper
>   
>   class foperator_minus : public range_operator_float
>   {
> +  using range_operator_float::op1_range;
> +  using range_operator_float::op2_range;
> +public:
> +  virtual bool op1_range (frange &r, tree type,
> +			  const frange &lhs,
> +			  const frange &op2,
> +			  relation_trio = TRIO_VARYING) const final override
> +  {
> +    if (lhs.undefined_p ())
> +      return false;
> +    return float_binary_op_range_finish (fop_plus.fold_range (r, type, lhs,
> +							      op2),
> +					 r, type, lhs);
> +  }
> +  virtual bool op2_range (frange &r, tree type,
> +			  const frange &lhs,
> +			  const frange &op1,
> +			  relation_trio = TRIO_VARYING) const final override
> +  {
> +    if (lhs.undefined_p ())
> +      return false;
> +    return float_binary_op_range_finish (fold_range (r, type, op1, lhs),
> +					 r, type, lhs);
> +  }
> +private:
>     void rv_fold (REAL_VALUE_TYPE &lb, REAL_VALUE_TYPE &ub, bool &maybe_nan,
>   		tree type,
>   		const REAL_VALUE_TYPE &lh_lb,
> @@ -2031,6 +2112,30 @@ protected:
>   
>   class foperator_mult : public foperator_mult_div_base
>   {
> +  using range_operator_float::op1_range;
> +  using range_operator_float::op2_range;
> +public:
> +  virtual bool op1_range (frange &r, tree type,
> +			  const frange &lhs,
> +			  const frange &op2,
> +			  relation_trio = TRIO_VARYING) const final override
> +  {
> +    if (lhs.undefined_p ())
> +      return false;
> +    range_op_handler rdiv (RDIV_EXPR, type);
> +    if (!rdiv)
> +      return false;
> +    return float_binary_op_range_finish (rdiv.fold_range (r, type, lhs, op2),
> +					 r, type, lhs);
> +  }
> +  virtual bool op2_range (frange &r, tree type,
> +			  const frange &lhs,
> +			  const frange &op1,
> +			  relation_trio = TRIO_VARYING) const final override
> +  {
> +    return op1_range (r, type, lhs, op1);
> +  }
> +private:
>     void rv_fold (REAL_VALUE_TYPE &lb, REAL_VALUE_TYPE &ub, bool &maybe_nan,
>   		tree type,
>   		const REAL_VALUE_TYPE &lh_lb,
> @@ -2138,6 +2243,31 @@ class foperator_mult : public foperator_
>   
>   class foperator_div : public foperator_mult_div_base
>   {
> +  using range_operator_float::op1_range;
> +  using range_operator_float::op2_range;
> +public:
> +  virtual bool op1_range (frange &r, tree type,
> +			  const frange &lhs,
> +			  const frange &op2,
> +			  relation_trio = TRIO_VARYING) const final override
> +  {
> +    if (lhs.undefined_p ())
> +      return false;
> +    return float_binary_op_range_finish (fop_mult.fold_range (r, type, lhs,
> +							      op2),
> +					 r, type, lhs);
> +  }
> +  virtual bool op2_range (frange &r, tree type,
> +			  const frange &lhs,
> +			  const frange &op1,
> +			  relation_trio = TRIO_VARYING) const final override
> +  {
> +    if (lhs.undefined_p ())
> +      return false;
> +    return float_binary_op_range_finish (fold_range (r, type, op1, lhs),
> +					 r, type, lhs);
> +  }
> +private:
>     void rv_fold (REAL_VALUE_TYPE &lb, REAL_VALUE_TYPE &ub, bool &maybe_nan,
>   		tree type,
>   		const REAL_VALUE_TYPE &lh_lb,
> --- gcc/testsuite/gcc.c-torture/execute/ieee/inf-4.c.jj	2022-11-11 12:44:57.615274471 +0100
> +++ gcc/testsuite/gcc.c-torture/execute/ieee/inf-4.c	2022-11-11 12:44:13.351879222 +0100
> @@ -0,0 +1,26 @@
> +__attribute__((noipa)) int
> +foo (double a, double b)
> +{
> +  double c = a - b;
> +  if (!__builtin_isfinite (c))
> +    {
> +      if (__builtin_isnan (c))
> +	{
> +	  if (!__builtin_isnan (a) && !__builtin_isnan (b))
> +	    return 1;
> +	}
> +      else if (__builtin_isfinite (a) && __builtin_isfinite (b))
> +	return 2;
> +    }
> +  else if (c == 0 && a != b)
> +    return 3;
> +  return 4;
> +}
> +
> +int
> +main ()
> +{
> +  double a = __builtin_inf ();
> +  if (foo (a, a) != 1)
> +    __builtin_abort ();
> +}
> 
> 
> 	Jakub
>