public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
From: Tamar Christina <Tamar.Christina@arm.com>
To: Andrew Carlotti <Andrew.Carlotti@arm.com>,
	Andrew MacLeod <amacleod@redhat.com>
Cc: Richard Biener <rguenther@suse.de>,
	Richard Sandiford <Richard.Sandiford@arm.com>,
	Tamar Christina via Gcc-patches <gcc-patches@gcc.gnu.org>,
	nd <nd@arm.com>, "jlaw@ventanamicro.com" <jlaw@ventanamicro.com>
Subject: RE: [PATCH 1/2]middle-end: Fix wrong overmatching of div-bitmask by using new optabs [PR108583]
Date: Wed, 1 Mar 2023 18:16:49 +0000	[thread overview]
Message-ID: <VI1PR08MB5325BBF9BAA5ED3617107F0FFFAD9@VI1PR08MB5325.eurprd08.prod.outlook.com> (raw)
In-Reply-To: <Y/+D/S8sjmsetFoE@e124511.cambridge.arm.com>

> -----Original Message-----
> From: Andrew Carlotti <Andrew.Carlotti@arm.com>
> Sent: Wednesday, March 1, 2023 4:58 PM
> To: Andrew MacLeod <amacleod@redhat.com>
> Cc: Tamar Christina <Tamar.Christina@arm.com>; Richard Biener
> <rguenther@suse.de>; Richard Sandiford <Richard.Sandiford@arm.com>;
> Tamar Christina via Gcc-patches <gcc-patches@gcc.gnu.org>; nd
> <nd@arm.com>; jlaw@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


  reply	other threads:[~2023-03-01 18:17 UTC|newest]

Thread overview: 47+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-02-09 17:16 Tamar Christina
2023-02-09 17:22 ` [PATCH 2/2]AArch64 Update div-bitmask to implement new optab instead of target hook [PR108583] Tamar Christina
2023-02-10 10:35   ` Tamar Christina
2023-02-10 14:10   ` Richard Sandiford
2023-02-10 10:34 ` [PATCH 1/2]middle-end: Fix wrong overmatching of div-bitmask by using new optabs [PR108583] Tamar Christina
2023-02-10 13:13 ` Richard Biener
2023-02-10 13:36 ` Richard Sandiford
2023-02-10 13:52   ` Richard Biener
2023-02-10 14:13   ` Tamar Christina
2023-02-10 14:30     ` Richard Sandiford
2023-02-10 14:54       ` Tamar Christina
2023-02-27 11:09       ` Tamar Christina
2023-02-27 12:11         ` Richard Sandiford
2023-02-27 12:14           ` Tamar Christina
2023-02-27 21:33             ` Richard Sandiford
2023-02-27 22:10               ` Tamar Christina
2023-02-28 11:08                 ` Richard Sandiford
2023-02-28 11:12                   ` Tamar Christina
2023-02-28 12:03                     ` Richard Sandiford
2023-03-01 11:30                       ` Richard Biener
2023-02-10 15:56     ` Richard Sandiford
2023-02-10 16:09       ` Tamar Christina
2023-02-10 16:25         ` Richard Sandiford
2023-02-10 16:33           ` Tamar Christina
2023-02-10 16:57             ` Richard Sandiford
2023-02-10 17:01               ` Richard Sandiford
2023-02-10 17:14               ` Tamar Christina
2023-02-10 18:12                 ` Richard Sandiford
2023-02-10 18:34                   ` Richard Biener
2023-02-10 20:58                     ` Andrew MacLeod
2023-02-13  9:54                       ` Tamar Christina
2023-02-15 12:51                         ` Tamar Christina
2023-02-15 16:05                           ` Andrew MacLeod
2023-02-15 17:13                             ` Tamar Christina
2023-02-15 17:50                               ` Andrew MacLeod
2023-02-15 18:42                                 ` Andrew MacLeod
2023-02-22 12:51                                   ` Tamar Christina
2023-02-22 16:41                                   ` Andrew MacLeod
2023-02-22 18:03                                     ` Tamar Christina
2023-02-22 18:33                                       ` Andrew MacLeod
2023-02-23  8:36                                         ` Tamar Christina
2023-02-23 16:39                                           ` Andrew MacLeod
2023-02-23 16:56                                             ` Tamar Christina
2023-03-01 16:57                                             ` Andrew Carlotti
2023-03-01 18:16                                               ` Tamar Christina [this message]
2023-02-22 13:06                                 ` Tamar Christina
2023-02-22 15:19                                   ` Andrew MacLeod

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=VI1PR08MB5325BBF9BAA5ED3617107F0FFFAD9@VI1PR08MB5325.eurprd08.prod.outlook.com \
    --to=tamar.christina@arm.com \
    --cc=Andrew.Carlotti@arm.com \
    --cc=Richard.Sandiford@arm.com \
    --cc=amacleod@redhat.com \
    --cc=gcc-patches@gcc.gnu.org \
    --cc=jlaw@ventanamicro.com \
    --cc=nd@arm.com \
    --cc=rguenther@suse.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).