[PATCH] middle-end/110495 - avoid associating constants with (VL) vectors

[PATCH] middle-end/110495 - avoid associating constants with (VL) vectors

[Richard Biener]

When trying to associate (v + INT_MAX) + INT_MAX we are using
the TREE_OVERFLOW bit to check for correctness.  That isn't
working for VECTOR_CSTs and it can't in general when one considers
VL vectors.  It looks like it should work for COMPLEX_CSTs but
I didn't try to single out _Complex int in this change.

The following makes sure that for vectors we use the fallback of
using unsigned arithmetic when associating the above to
v + (INT_MAX + INT_MAX).

Bootstrapped and tested on x86_64-unknown-linux-gnu, OK?

Thanks,
Richard.

	PR middle-end/110495
	* tree.h (TREE_OVERFLOW): Do not mention VECTOR_CSTs
	since we do not set TREE_OVERFLOW on those since the
	introduction of VL vectors.
	* match.pd (x +- CST +- CST): For VECTOR_CST do not look
	at TREE_OVERFLOW to determine validity of association.

	* gcc.dg/tree-ssa/addadd-2.c: Amend.
	* gcc.dg/tree-ssa/forwprop-27.c: Adjust.
---
 gcc/match.pd                                | 9 +++++----
 gcc/testsuite/gcc.dg/tree-ssa/addadd-2.c    | 1 +
 gcc/testsuite/gcc.dg/tree-ssa/forwprop-27.c | 4 +++-
 gcc/tree.h                                  | 2 +-
 4 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/gcc/match.pd b/gcc/match.pd
index f09583bbcac..d193a572005 100644
--- a/gcc/match.pd
+++ b/gcc/match.pd
@@ -3025,7 +3025,7 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
 	(with { tree cst = const_binop (outer_op == inner_op
 					? PLUS_EXPR : MINUS_EXPR,
 					type, @1, @2); }
-	 (if (cst && !TREE_OVERFLOW (cst))
+	 (if (INTEGRAL_TYPE_P (type) && cst && !TREE_OVERFLOW (cst))
 	  (inner_op @0 { cst; } )
 	  /* X+INT_MAX+1 is X-INT_MIN.  */
 	  (if (INTEGRAL_TYPE_P (type) && cst
@@ -3037,7 +3037,8 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
 	     (view_convert (inner_op
 			    (view_convert:utype @0)
 			    (view_convert:utype
-			     { drop_tree_overflow (cst); }))))))))))))))
+			     { TREE_OVERFLOW (cst)
+			       ? drop_tree_overflow (cst) : cst; }))))))))))))))
 
   /* (CST1 - A) +- CST2 -> CST3 - A  */
   (for outer_op (plus minus)
@@ -3049,7 +3050,7 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
 	forever if something doesn't simplify into a constant.  */
      (if (!CONSTANT_CLASS_P (@0))
       (minus (outer_op! (view_convert @1) @2) (view_convert @0)))
-     (if (!ANY_INTEGRAL_TYPE_P (TREE_TYPE (@0))
+     (if (!INTEGRAL_TYPE_P (TREE_TYPE (@0))
 	  || TYPE_OVERFLOW_WRAPS (TREE_TYPE (@0)))
       (view_convert (minus (outer_op! @1 (view_convert @2)) @0))
       (if (types_match (type, @0) && !TYPE_OVERFLOW_SANITIZED (type))
@@ -3068,7 +3069,7 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
       forever if something doesn't simplify into a constant.  */
     (if (!CONSTANT_CLASS_P (@0))
      (plus (view_convert @0) (minus! @1 (view_convert @2))))
-    (if (!ANY_INTEGRAL_TYPE_P (TREE_TYPE (@0))
+    (if (!INTEGRAL_TYPE_P (TREE_TYPE (@0))
 	 || TYPE_OVERFLOW_WRAPS (TREE_TYPE (@0)))
      (view_convert (plus @0 (minus! (view_convert @1) @2)))
      (if (types_match (type, @0) && !TYPE_OVERFLOW_SANITIZED (type))
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/addadd-2.c b/gcc/testsuite/gcc.dg/tree-ssa/addadd-2.c
index 39aa032c9b1..8c05911f473 100644
--- a/gcc/testsuite/gcc.dg/tree-ssa/addadd-2.c
+++ b/gcc/testsuite/gcc.dg/tree-ssa/addadd-2.c
@@ -12,4 +12,5 @@ void k(S*x){
   *x = (S)(y + __INT_MAX__);
 }
 
+/* { dg-final { scan-tree-dump "4294967294" "optimized" { target int32plus } } } */
 /* { dg-final { scan-tree-dump-not "2147483647" "optimized" } } */
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/forwprop-27.c b/gcc/testsuite/gcc.dg/tree-ssa/forwprop-27.c
index 9775a4c6367..6c71a4fc81c 100644
--- a/gcc/testsuite/gcc.dg/tree-ssa/forwprop-27.c
+++ b/gcc/testsuite/gcc.dg/tree-ssa/forwprop-27.c
@@ -33,7 +33,9 @@ void i (V *v1, V *v2){
   *v2 = (c1-*v2)+c2;
 }
 
-/* { dg-final { scan-tree-dump-not "\\\+" "forwprop1"} } */
+/* { dg-final { scan-tree-dump-times "\\\+" 1 "forwprop1"} } */
 /* { dg-final { scan-tree-dump "{ 0, 4 }" "forwprop1"} } */
 /* { dg-final { scan-tree-dump "{ 37, -5 }" "forwprop1"} } */
+/* { dg-final { scan-tree-dump "{ 27, 23 }" "forwprop1"} } */
+/* { dg-final { scan-tree-dump "{ 37, 3 }" "forwprop1"} } */
 
diff --git a/gcc/tree.h b/gcc/tree.h
index f11c758afb9..bedbbb0bcc0 100644
--- a/gcc/tree.h
+++ b/gcc/tree.h
@@ -824,7 +824,7 @@ extern void omp_clause_range_check_failed (const_tree, const char *, int,
 #define TYPE_REF_CAN_ALIAS_ALL(NODE) \
   (PTR_OR_REF_CHECK (NODE)->base.static_flag)
 
-/* In an INTEGER_CST, REAL_CST, COMPLEX_CST, or VECTOR_CST, this means
+/* In an INTEGER_CST, REAL_CST, or COMPLEX_CST, this means
    there was an overflow in folding.  */
 
 #define TREE_OVERFLOW(NODE) (CST_CHECK (NODE)->base.public_flag)
-- 
2.35.3

Re: [PATCH] middle-end/110495 - avoid associating constants with (VL) vectors

[Richard Biener]

On Mon, 3 Jul 2023, Richard Sandiford wrote:

> Richard Biener via Gcc-patches <gcc-patches@gcc.gnu.org> writes:
> > When trying to associate (v + INT_MAX) + INT_MAX we are using
> > the TREE_OVERFLOW bit to check for correctness.  That isn't
> > working for VECTOR_CSTs and it can't in general when one considers
> > VL vectors.  It looks like it should work for COMPLEX_CSTs but
> > I didn't try to single out _Complex int in this change.
> >
> > The following makes sure that for vectors we use the fallback of
> > using unsigned arithmetic when associating the above to
> > v + (INT_MAX + INT_MAX).
> >
> > Bootstrapped and tested on x86_64-unknown-linux-gnu, OK?
> >
> > Thanks,
> > Richard.
> >
> > 	PR middle-end/110495
> > 	* tree.h (TREE_OVERFLOW): Do not mention VECTOR_CSTs
> > 	since we do not set TREE_OVERFLOW on those since the
> > 	introduction of VL vectors.
> > 	* match.pd (x +- CST +- CST): For VECTOR_CST do not look
> > 	at TREE_OVERFLOW to determine validity of association.
> >
> > 	* gcc.dg/tree-ssa/addadd-2.c: Amend.
> > 	* gcc.dg/tree-ssa/forwprop-27.c: Adjust.
> > ---
> >  gcc/match.pd                                | 9 +++++----
> >  gcc/testsuite/gcc.dg/tree-ssa/addadd-2.c    | 1 +
> >  gcc/testsuite/gcc.dg/tree-ssa/forwprop-27.c | 4 +++-
> >  gcc/tree.h                                  | 2 +-
> >  4 files changed, 10 insertions(+), 6 deletions(-)
> >
> > diff --git a/gcc/match.pd b/gcc/match.pd
> > index f09583bbcac..d193a572005 100644
> > --- a/gcc/match.pd
> > +++ b/gcc/match.pd
> > @@ -3025,7 +3025,7 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
> >  	(with { tree cst = const_binop (outer_op == inner_op
> >  					? PLUS_EXPR : MINUS_EXPR,
> >  					type, @1, @2); }
> > -	 (if (cst && !TREE_OVERFLOW (cst))
> > +	 (if (INTEGRAL_TYPE_P (type) && cst && !TREE_OVERFLOW (cst))
> >  	  (inner_op @0 { cst; } )
> >  	  /* X+INT_MAX+1 is X-INT_MIN.  */
> >  	  (if (INTEGRAL_TYPE_P (type) && cst
> > @@ -3037,7 +3037,8 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
> >  	     (view_convert (inner_op
> >  			    (view_convert:utype @0)
> >  			    (view_convert:utype
> > -			     { drop_tree_overflow (cst); }))))))))))))))
> > +			     { TREE_OVERFLOW (cst)
> > +			       ? drop_tree_overflow (cst) : cst; }))))))))))))))
> 
> It looks like the whole ?(with ?)? expects cst to be nonnull,
> but the ?last resort? doesn't check it (unless I'm misreading).
> Would it be easier to add a top-level ?if (cst)??  (Obviously
> a preexisting thing.)

Hmm, indeed.  I've added an outer if (cst).

> >  
> >    /* (CST1 - A) +- CST2 -> CST3 - A  */
> >    (for outer_op (plus minus)
> > @@ -3049,7 +3050,7 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
> >  	forever if something doesn't simplify into a constant.  */
> >       (if (!CONSTANT_CLASS_P (@0))
> >        (minus (outer_op! (view_convert @1) @2) (view_convert @0)))
> > -     (if (!ANY_INTEGRAL_TYPE_P (TREE_TYPE (@0))
> > +     (if (!INTEGRAL_TYPE_P (TREE_TYPE (@0))
> >  	  || TYPE_OVERFLOW_WRAPS (TREE_TYPE (@0)))
> >        (view_convert (minus (outer_op! @1 (view_convert @2)) @0))
> >        (if (types_match (type, @0) && !TYPE_OVERFLOW_SANITIZED (type))
> > @@ -3068,7 +3069,7 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
> >        forever if something doesn't simplify into a constant.  */
> >      (if (!CONSTANT_CLASS_P (@0))
> >       (plus (view_convert @0) (minus! @1 (view_convert @2))))
> > -    (if (!ANY_INTEGRAL_TYPE_P (TREE_TYPE (@0))
> > +    (if (!INTEGRAL_TYPE_P (TREE_TYPE (@0))
> >  	 || TYPE_OVERFLOW_WRAPS (TREE_TYPE (@0)))
> >       (view_convert (plus @0 (minus! (view_convert @1) @2)))
> >       (if (types_match (type, @0) && !TYPE_OVERFLOW_SANITIZED (type))
> 
> I didn't understand this part.  Doesn't it mean that we allow
> overflow-inducing reassociations for all vector integer types,
> albeit not immediately folded away?

Oh, indeed - I though I can circumvent the TREE_OVERFLOW check for
those (where I don't yet have a testcase) by altering the guarding
check - but that check is to guard the TYPE_OVERFLOW_* checks.

To fix this we'd have to add unsigned fallbacks like for the above
pattern.  I'm going to remove the two hunks for now.

> Also, why do we keep the:
> 
>   !ANY_INTEGRAL_TYPE_P (type) || TYPE_OVERFLOW_WRAPS (type)
> 
> in the outer ifs?

I think this is distict types since both patterns have conditiona
conversions in the patterns they match.  Otherwise it would be
redundant checking and would have been better if placed as 'else'
branch of the inner ifs.

As said, going to fix the missing conditional on non-null 'cst'
and drop the two hunks unrelated to the PR (which also were
wrong - thanks for noticing).

Richard.


> 
> But that's just me not understanding match.pd very well.
> Feel free to ignore if it's nonsense. :)
> 
> Thanks,
> Richard
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg,
Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman;
HRB 36809 (AG Nuernberg)

Re: [PATCH] middle-end/110495 - avoid associating constants with (VL) vectors

[Richard Sandiford]

Richard Biener via Gcc-patches <gcc-patches@gcc.gnu.org> writes:
> When trying to associate (v + INT_MAX) + INT_MAX we are using
> the TREE_OVERFLOW bit to check for correctness.  That isn't
> working for VECTOR_CSTs and it can't in general when one considers
> VL vectors.  It looks like it should work for COMPLEX_CSTs but
> I didn't try to single out _Complex int in this change.
>
> The following makes sure that for vectors we use the fallback of
> using unsigned arithmetic when associating the above to
> v + (INT_MAX + INT_MAX).
>
> Bootstrapped and tested on x86_64-unknown-linux-gnu, OK?
>
> Thanks,
> Richard.
>
> 	PR middle-end/110495
> 	* tree.h (TREE_OVERFLOW): Do not mention VECTOR_CSTs
> 	since we do not set TREE_OVERFLOW on those since the
> 	introduction of VL vectors.
> 	* match.pd (x +- CST +- CST): For VECTOR_CST do not look
> 	at TREE_OVERFLOW to determine validity of association.
>
> 	* gcc.dg/tree-ssa/addadd-2.c: Amend.
> 	* gcc.dg/tree-ssa/forwprop-27.c: Adjust.
> ---
>  gcc/match.pd                                | 9 +++++----
>  gcc/testsuite/gcc.dg/tree-ssa/addadd-2.c    | 1 +
>  gcc/testsuite/gcc.dg/tree-ssa/forwprop-27.c | 4 +++-
>  gcc/tree.h                                  | 2 +-
>  4 files changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/gcc/match.pd b/gcc/match.pd
> index f09583bbcac..d193a572005 100644
> --- a/gcc/match.pd
> +++ b/gcc/match.pd
> @@ -3025,7 +3025,7 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
>  	(with { tree cst = const_binop (outer_op == inner_op
>  					? PLUS_EXPR : MINUS_EXPR,
>  					type, @1, @2); }
> -	 (if (cst && !TREE_OVERFLOW (cst))
> +	 (if (INTEGRAL_TYPE_P (type) && cst && !TREE_OVERFLOW (cst))
>  	  (inner_op @0 { cst; } )
>  	  /* X+INT_MAX+1 is X-INT_MIN.  */
>  	  (if (INTEGRAL_TYPE_P (type) && cst
> @@ -3037,7 +3037,8 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
>  	     (view_convert (inner_op
>  			    (view_convert:utype @0)
>  			    (view_convert:utype
> -			     { drop_tree_overflow (cst); }))))))))))))))
> +			     { TREE_OVERFLOW (cst)
> +			       ? drop_tree_overflow (cst) : cst; }))))))))))))))

It looks like the whole “(with …)” expects cst to be nonnull,
but the “last resort” doesn't check it (unless I'm misreading).
Would it be easier to add a top-level “if (cst)”?  (Obviously
a preexisting thing.)
>  
>    /* (CST1 - A) +- CST2 -> CST3 - A  */
>    (for outer_op (plus minus)
> @@ -3049,7 +3050,7 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
>  	forever if something doesn't simplify into a constant.  */
>       (if (!CONSTANT_CLASS_P (@0))
>        (minus (outer_op! (view_convert @1) @2) (view_convert @0)))
> -     (if (!ANY_INTEGRAL_TYPE_P (TREE_TYPE (@0))
> +     (if (!INTEGRAL_TYPE_P (TREE_TYPE (@0))
>  	  || TYPE_OVERFLOW_WRAPS (TREE_TYPE (@0)))
>        (view_convert (minus (outer_op! @1 (view_convert @2)) @0))
>        (if (types_match (type, @0) && !TYPE_OVERFLOW_SANITIZED (type))
> @@ -3068,7 +3069,7 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
>        forever if something doesn't simplify into a constant.  */
>      (if (!CONSTANT_CLASS_P (@0))
>       (plus (view_convert @0) (minus! @1 (view_convert @2))))
> -    (if (!ANY_INTEGRAL_TYPE_P (TREE_TYPE (@0))
> +    (if (!INTEGRAL_TYPE_P (TREE_TYPE (@0))
>  	 || TYPE_OVERFLOW_WRAPS (TREE_TYPE (@0)))
>       (view_convert (plus @0 (minus! (view_convert @1) @2)))
>       (if (types_match (type, @0) && !TYPE_OVERFLOW_SANITIZED (type))

I didn't understand this part.  Doesn't it mean that we allow
overflow-inducing reassociations for all vector integer types,
albeit not immediately folded away?

Also, why do we keep the:

  !ANY_INTEGRAL_TYPE_P (type) || TYPE_OVERFLOW_WRAPS (type)

in the outer ifs?

But that's just me not understanding match.pd very well.
Feel free to ignore if it's nonsense. :)

Thanks,
Richard