Re: [PATCH 1/2]middle-end: Fix wrong overmatching of div-bitmask by using new optabs [PR108583]

[Andrew Carlotti <andrew.carlotti@arm.com>]

On Thu, Feb 23, 2023 at 11:39:51AM -0500, Andrew MacLeod via Gcc-patches wrote:
> 
> 
> Inheriting from operator_mult is also going to be hazardous because it also
> has an op1_range and op2_range...� you should at least define those and
> return VARYING to avoid other issues.� Same thing applies to widen_plus I
> think, and it has relation processing and other things as well.� Your widen
> operands are not what those classes expect, so I think you probably just
> want a fresh range operator.
> 
> It also looks like the mult operation is sign/zero extending both upper
> bounds, and neither lower bound..�� I think that should be the LH upper and
> lower bound?
> 
> I've attached a second patch� (newversion.patch) which incorporates my fix,
> the fix to the sign of only op1's bounds,� as well as a simplification of
> the classes to not inherit from operator_mult/plus..�� I think this still
> does what you want?� and it wont get you into unexpected trouble later :-)
> 
> let me know if this is still doing what you are expecting...
> 
> Andrew
> 

Hi,

This patch still uses the wrong signedness for some of the extensions in
WIDEN_MULT_EXPR. It currently bases it's promotion decisions on whether there
is any signed argument, and whether the result is signed - i.e.:

		Patch extends as:
UUU		UU
UUS -> USU
USU		SU
USS		SU	wrong
SUU		US	wrong
SUS -> SSU
SSU		SS	wrong
SSS		SS

The documentation in tree.def is unclear about whether the output signedness is
linked to the input signedness, but at least the SSU case seems valid, and is
mishandled here.

I think it would be clearer and simpler to have four (or three) different
versions for each combnation of signedness of the input operands. This could be
implemented without extra code duplication by creating four different instances
of an operator_widen_mult class (perhaps extending a range_operator_mixed_sign
class), with the signedness indicated by two additional class members.

The documentation for WIDEN_PLUS_EXPR (and several other expressions added in
the same commit) is completely missing. If the signs are required to be
matching, then this should be clarified; otherwise it would need the same
special handling as WIDEN_MULT_EXPR.

Andrew

> diff --git a/gcc/gimple-range-op.cc b/gcc/gimple-range-op.cc
> index d9dfdc56939..824e0338f34 100644
> --- a/gcc/gimple-range-op.cc
> +++ b/gcc/gimple-range-op.cc
> @@ -179,6 +179,8 @@ gimple_range_op_handler::gimple_range_op_handler (gimple *s)
>    // statements.
>    if (is_a <gcall *> (m_stmt))
>      maybe_builtin_call ();
> +  else
> +    maybe_non_standard ();
>  }
>  
>  // Calculate what we can determine of the range of this unary
> @@ -764,6 +766,36 @@ public:
>    }
>  } op_cfn_parity;
>  
> +// Set up a gimple_range_op_handler for any nonstandard function which can be
> +// supported via range-ops.
> +
> +void
> +gimple_range_op_handler::maybe_non_standard ()
> +{
> +  if (gimple_code (m_stmt) == GIMPLE_ASSIGN)
> +    switch (gimple_assign_rhs_code (m_stmt))
> +      {
> +	case WIDEN_MULT_EXPR:
> +	{
> +	  m_valid = true;
> +	  m_op1 = gimple_assign_rhs1 (m_stmt);
> +	  m_op2 = gimple_assign_rhs2 (m_stmt);
> +	  bool signed1 = TYPE_SIGN (TREE_TYPE (m_op1)) == SIGNED;
> +	  bool signed2 = TYPE_SIGN (TREE_TYPE (m_op2)) == SIGNED;
> +	  if (signed2 && !signed1)
> +	    std::swap (m_op1, m_op2);
> +
> +	  if (signed1 || signed2)
> +	    m_int = ptr_op_widen_mult_signed;
> +	  else
> +	    m_int = ptr_op_widen_mult_unsigned;
> +	  break;
> +	}
> +	default:
> +	  break;
> +      }
> +}
> +
>  // Set up a gimple_range_op_handler for any built in function which can be
>  // supported via range-ops.
>  
> diff --git a/gcc/gimple-range-op.h b/gcc/gimple-range-op.h
> index 743b858126e..1bf63c5ce6f 100644
> --- a/gcc/gimple-range-op.h
> +++ b/gcc/gimple-range-op.h
> @@ -41,6 +41,7 @@ public:
>  		 relation_trio = TRIO_VARYING);
>  private:
>    void maybe_builtin_call ();
> +  void maybe_non_standard ();
>    gimple *m_stmt;
>    tree m_op1, m_op2;
>  };
> diff --git a/gcc/range-op.cc b/gcc/range-op.cc
> index 5c67bce6d3a..7cd19a92d00 100644
> --- a/gcc/range-op.cc
> +++ b/gcc/range-op.cc
> @@ -1556,6 +1556,34 @@ operator_plus::op2_range (irange &r, tree type,
>    return op1_range (r, type, lhs, op1, rel.swap_op1_op2 ());
>  }
>  
> +class operator_widen_plus : public range_operator
> +{
> +public:
> +  virtual void wi_fold (irange &r, tree type,
> +			const wide_int &lh_lb,
> +			const wide_int &lh_ub,
> +			const wide_int &rh_lb,
> +			const wide_int &rh_ub) const;
> +} op_widen_plus;
> +
> +void
> +operator_widen_plus::wi_fold (irange &r, tree type,
> +			const wide_int &lh_lb, const wide_int &lh_ub,
> +			const wide_int &rh_lb, const wide_int &rh_ub) const
> +{
> +   wi::overflow_type ov_lb, ov_ub;
> +   signop s = TYPE_SIGN (type);
> +
> +   wide_int lh_wlb = wide_int::from (lh_lb, wi::get_precision (lh_lb) * 2, s);
> +   wide_int rh_wlb = wide_int::from (rh_lb, wi::get_precision (rh_lb) * 2, s);
> +   wide_int lh_wub = wide_int::from (lh_ub, wi::get_precision (lh_ub) * 2, s);
> +   wide_int rh_wub = wide_int::from (rh_ub, wi::get_precision (rh_ub) * 2, s);
> +
> +   wide_int new_lb = wi::add (lh_wlb, rh_wlb, s, &ov_lb);
> +   wide_int new_ub = wi::add (lh_wub, rh_wub, s, &ov_ub);
> +
> +   r = int_range<2> (type, new_lb, new_ub);
> +}
>  
>  class operator_minus : public range_operator
>  {
> @@ -2031,6 +2059,70 @@ operator_mult::wi_fold (irange &r, tree type,
>      }
>  }
>  
> +class operator_widen_mult_signed : public range_operator
> +{
> +public:
> +  virtual void wi_fold (irange &r, tree type,
> +			const wide_int &lh_lb,
> +			const wide_int &lh_ub,
> +			const wide_int &rh_lb,
> +			const wide_int &rh_ub)
> +    const;
> +} op_widen_mult_signed;
> +range_operator *ptr_op_widen_mult_signed = &op_widen_mult_signed;
> +
> +void
> +operator_widen_mult_signed::wi_fold (irange &r, tree type,
> +				     const wide_int &lh_lb,
> +				     const wide_int &lh_ub,
> +				     const wide_int &rh_lb,
> +				     const wide_int &rh_ub) const
> +{
> +  signop s = TYPE_SIGN (type);
> +
> +  wide_int lh_wlb = wide_int::from (lh_lb, wi::get_precision (lh_lb) * 2, SIGNED);
> +  wide_int lh_wub = wide_int::from (lh_ub, wi::get_precision (lh_ub) * 2, SIGNED);
> +  wide_int rh_wlb = wide_int::from (rh_lb, wi::get_precision (rh_lb) * 2, s);
> +  wide_int rh_wub = wide_int::from (rh_ub, wi::get_precision (rh_ub) * 2, s);
> +
> +  /* We don't expect a widening multiplication to be able to overflow but range
> +     calculations for multiplications are complicated.  After widening the
> +     operands lets call the base class.  */
> +  return op_mult.wi_fold (r, type, lh_wlb, lh_wub, rh_wlb, rh_wub);
> +}
> +
> +
> +class operator_widen_mult_unsigned : public range_operator
> +{
> +public:
> +  virtual void wi_fold (irange &r, tree type,
> +			const wide_int &lh_lb,
> +			const wide_int &lh_ub,
> +			const wide_int &rh_lb,
> +			const wide_int &rh_ub)
> +    const;
> +} op_widen_mult_unsigned;
> +range_operator *ptr_op_widen_mult_unsigned = &op_widen_mult_unsigned;
> +
> +void
> +operator_widen_mult_unsigned::wi_fold (irange &r, tree type,
> +				       const wide_int &lh_lb,
> +				       const wide_int &lh_ub,
> +				       const wide_int &rh_lb,
> +				       const wide_int &rh_ub) const
> +{
> +  signop s = TYPE_SIGN (type);
> +
> +  wide_int lh_wlb = wide_int::from (lh_lb, wi::get_precision (lh_lb) * 2, UNSIGNED);
> +  wide_int lh_wub = wide_int::from (lh_ub, wi::get_precision (lh_ub) * 2, UNSIGNED);
> +  wide_int rh_wlb = wide_int::from (rh_lb, wi::get_precision (rh_lb) * 2, s);
> +  wide_int rh_wub = wide_int::from (rh_ub, wi::get_precision (rh_ub) * 2, s);
> +
> +  /* We don't expect a widening multiplication to be able to overflow but range
> +     calculations for multiplications are complicated.  After widening the
> +     operands lets call the base class.  */
> +  return op_mult.wi_fold (r, type, lh_wlb, lh_wub, rh_wlb, rh_wub);
> +}
>  
>  class operator_div : public cross_product_operator
>  {
> @@ -4473,6 +4565,7 @@ integral_table::integral_table ()
>    set (GT_EXPR, op_gt);
>    set (GE_EXPR, op_ge);
>    set (PLUS_EXPR, op_plus);
> +  set (WIDEN_PLUS_EXPR, op_widen_plus);
>    set (MINUS_EXPR, op_minus);
>    set (MIN_EXPR, op_min);
>    set (MAX_EXPR, op_max);
> diff --git a/gcc/range-op.h b/gcc/range-op.h
> index f00b747f08a..5fe463234ae 100644
> --- a/gcc/range-op.h
> +++ b/gcc/range-op.h
> @@ -311,4 +311,6 @@ private:
>  // This holds the range op table for floating point operations.
>  extern floating_op_table *floating_tree_table;
>  
> +extern range_operator *ptr_op_widen_mult_signed;
> +extern range_operator *ptr_op_widen_mult_unsigned;
>  #endif // GCC_RANGE_OP_H