> -----Original Message-----
> From: Jakub Jelinek <ja...@redhat.com>
> Sent: Monday, August 19, 2024 8:25 PM
> To: Tamar Christina <tamar.christ...@arm.com>
> Cc: Li, Pan2 <pan2...@intel.com>; Richard Biener <richard.guent...@gmail.com>;
> gcc-patches@gcc.gnu.org; juzhe.zh...@rivai.ai; kito.ch...@gmail.com;
> jeffreya...@gmail.com; rdapp....@gmail.com; Liu, Hongtao
> <hongtao....@intel.com>
> Subject: Re: [PATCH v1] Vect: Promote unsigned .SAT_ADD constant operand for
> vectorizable_call
>
> On Mon, Aug 19, 2024 at 01:55:38PM +0000, Tamar Christina wrote:
> > So would this not be the simplest fix:
> >
> > diff --git a/gcc/tree-vect-patterns.cc b/gcc/tree-vect-patterns.cc
> > index 87b3dc413b8..fcbc83a49f0 100644
> > --- a/gcc/tree-vect-patterns.cc
> > +++ b/gcc/tree-vect-patterns.cc
> > @@ -4558,6 +4558,9 @@ vect_recog_sat_add_pattern (vec_info *vinfo,
> stmt_vec_info stmt_vinfo,
> >
> > if (gimple_unsigned_integer_sat_add (lhs, ops, NULL))
>
> But then you call gimple_unsigned_integer_sat_add with mismatching types,
> not sure if that is ok.
>
gimple_unsigned_integer_sat_add is a match.pd predicate. It matches the
expression
rooted in lhs and returns the results in ops. So not sure what you mean here.
> > {
> > + if (TREE_CODE (ops[1]) == INTEGER_CST)
> > + ops[1] = fold_convert (TREE_TYPE (ops[0]), ops[1]);
> > +
>
> This would be only ok if the conversion doesn't change the value
> of the constant.
> .ADD_OVERFLOW etc. could have e.g. int and unsigned arguments, you don't
> want to change the latter to the former if the value has the most
> significant bit set.
> Similarly, .ADD_OVERFLOW could have e.g. unsigned and unsigned __int128
> arguments, you don't want to truncate the constant.
Yes, if the expression truncates or changes the sign of the expression this
wouldn't
work. But then you can also not match at all. So the match should be rejected
then
since the values need to fit in the same type as the argument and be the same
sign.
So the original vect_recog_promote_cst_to_unsigned is also wrong since it
doesn't
stop the match if it doesn't fit.
Imho, the pattern here cannot check this and it should be part of the match
condition.
If the constant cannot fir into the same type as the operand or has a different
sign
the matching should fail.
It's just that in match.pd you can't modify the arguments returned from a
predicate
but since the predicate is intended to be used to rewrite to the IFN I still
think the
above solution is right, and the range check should be done within the
predicate.
Tamar.
> So, you could e.g. do the fold_convert and then verify if
> wi::to_widest on the old and new tree are equal, or you could check for
> TREE_OVERFLOW if fold_convert honors that.
> As I said, for INTEGER_CST operands of .ADD/SUB/MUL_OVERFLOW, the infinite
> precision value (aka wi::to_widest) is all that matters.
>
> Jakub
