> -----Original Message-----
> From: Andrew Carlotti <andrew.carlo...@arm.com>
> Sent: Wednesday, March 1, 2023 4:58 PM
> To: Andrew MacLeod <amacl...@redhat.com>
> Cc: Tamar Christina <tamar.christ...@arm.com>; Richard Biener
> <rguent...@suse.de>; Richard Sandiford <richard.sandif...@arm.com>;
> Tamar Christina via Gcc-patches <gcc-patches@gcc.gnu.org>; nd
> <n...@arm.com>; j...@ventanamicro.com
> Subject: Re: [PATCH 1/2]middle-end: Fix wrong overmatching of div-bitmask
> by using new optabs [PR108583]
>
> 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.
Hi,
Thanks for the concern, but I don't think those "wrong" cases are valid.
There's only one explicit carve-out for this mismatch that I'm aware of which is
for constants that fit in the source type. convert_mult_to_widen doesn't accept
them otherwise.
For every other mismatched sign it will fold an explicit convert into the
sequence
to ensure all three types match.
i.e.
long unsigned d1(int x, int y)
{
return (long unsigned)x * y;
}
Requires a cast.
long unsigned d1(int x, int y)
{
return (long unsigned)x * 4;
}
Does not, and
long unsigned d1(int x, int y)
{
return (long unsigned)x * -4;
}
Does not fit and so is not accepted. The reason it can happen is that the
unsigned
cast on a positive constant is discarded.
Further more, the operation that introduces this widening only looks at the
sign of the left
most operand and that of the result.
So this is correctly handling the normal cases and the abnormal ones the
compiler introduces
as specific optimizations.
Tamar.
>
> 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
