Re: [PATCH] Further improvements for the (T)(P+A)-(T)(P+B) folding (PR sanitizer/81281)

[Richard Biener <rguenther at suse dot de>]

On Thu, 7 Dec 2017, Jakub Jelinek wrote:

> Hi!
> 
> When committing the previous PR81281 patch, I've removed all the @@0 cases
> on plus:c, used @0 instead, to make sure we don't regress.
> 
> This patch readds those where possible.  For the cases where there is
> just P and A, it was mostly a matter of @@0 and convert? instead of convert
> plus using type from @1 instead of @0, though if @0 is INTEGER_CST, what we
> usually end up with is a (plus (convert (plus @1 @0) @2) where @2 negated
> is equal to @0, so the patch adds a simplification for that too.
> 
> For the case with P, A and B, the patch limits it to the case where either
> both A and B are narrower or both are wider.
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

Comments below.

> 2017-12-07  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR sanitizer/81281
> 	* match.pd ((T)(P + A) - (T)P -> (T) A): Use @@0 instead of @0 and
> 	convert? on @0 instead of convert.  Check type of @1, not @0.
> 	Add a simplify for (T)(P + A) + Q where -Q is equal to P.
> 	((T)P - (T)(P + A) -> -(T) A): Use @@0 instead of @0 and
> 	convert? on @0 instead of convert.  Check type of @1, not @0.
> 	((T)(P + A) - (T)(P + B) -> (T)A - (T)B): Use @@0 instead of @0,
> 	only optimize if either both @1 and @2 types are narrower
> 	precision, or both are wider or equal precision, and in the former
> 	case only if both have undefined overflow.
> 
> 	* gcc.dg/pr81281-3.c: New test.
> 
> --- gcc/match.pd.jj	2017-12-07 14:00:51.083048186 +0100
> +++ gcc/match.pd	2017-12-07 15:17:49.132784931 +0100
> @@ -1784,8 +1784,8 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
>  
>    /* (T)(P + A) - (T)P -> (T) A */
>    (simplify
> -   (minus (convert (plus:c @0 @1))
> -    (convert @0))
> +   (minus (convert (plus:c @@0 @1))
> +    (convert? @0))
>     (if (element_precision (type) <= element_precision (TREE_TYPE (@1))
>  	/* For integer types, if A has a smaller type
>  	   than T the result depends on the possible
> @@ -1794,10 +1794,29 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
>  	   However, if an overflow in P + A would cause
>  	   undefined behavior, we can assume that there
>  	   is no overflow.  */
> -	|| (INTEGRAL_TYPE_P (TREE_TYPE (@0))
> -	    && TYPE_OVERFLOW_UNDEFINED (TREE_TYPE (@0))))
> +	|| (INTEGRAL_TYPE_P (TREE_TYPE (@1))
> +	    && TYPE_OVERFLOW_UNDEFINED (TREE_TYPE (@1))))

Given @1 and @@0 are in the same plus this change isn't technically
necessary but it makes it clearer which type we look at (thus ok).

>      (convert @1)))
>    (simplify
> +   (plus (convert (plus @1 INTEGER_CST@0)) INTEGER_CST@2)
> +   (with { bool overflow;
> +	   wide_int w = wi::neg (wi::to_wide (@2), &overflow); }
> +    (if (wi::to_widest (@0) == widest_int::from (w, TYPE_SIGN (TREE_TYPE (@2)))
> +	 && (!overflow
> +	     || (INTEGRAL_TYPE_P (TREE_TYPE (@2))
> +		 && TYPE_UNSIGNED (TREE_TYPE (@2))))
> +	 && (element_precision (type) <= element_precision (TREE_TYPE (@1))
> +	     /* For integer types, if A has a smaller type
> +		than T the result depends on the possible
> +		overflow in P + A.
> +		E.g. T=size_t, A=(unsigned)429497295, P>0.
> +		However, if an overflow in P + A would cause
> +		undefined behavior, we can assume that there
> +		is no overflow.  */
> +	     || (INTEGRAL_TYPE_P (TREE_TYPE (@1))
> +		 && TYPE_OVERFLOW_UNDEFINED (TREE_TYPE (@1)))))

I think we don't need to worry about definedness of overflow.  All
that matters is whether twos complement arithmetic will simplify
the expression to (convert @1).  Specifically the possible overflow
of the negation of @2 for the case element_precision (type) <= 
element_precision (TREE_TYPE (@1)) shouldn't matter, likewise
for the widening case (we'd never get the equality).

Don't we want to compare @0 and -@2 in the type of @2?  Like
for (unsigned int)(unsigned-long-x + 0x100000005) + -5U which
we should be able to simplify?  For the widening case that would
work as well as far as I can see?

If you can split out this new pattern the rest is ok with honoring
the comment below.

> +     (convert @1))))
> +  (simplify
>     (minus (convert (pointer_plus @@0 @1))
>      (convert @0))
>     (if (element_precision (type) <= element_precision (TREE_TYPE (@1))
> @@ -1818,8 +1837,8 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
>  
>    /* (T)P - (T)(P + A) -> -(T) A */
>    (simplify
> -   (minus (convert @0)
> -    (convert (plus:c @0 @1)))
> +   (minus (convert? @0)
> +    (convert (plus:c @@0 @1)))
>     (if (INTEGRAL_TYPE_P (type)
>  	&& TYPE_OVERFLOW_UNDEFINED (type)
>  	&& element_precision (type) <= element_precision (TREE_TYPE (@1)))
> @@ -1833,8 +1852,8 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
>  	    However, if an overflow in P + A would cause
>  	    undefined behavior, we can assume that there
>  	    is no overflow.  */
> -	 || (INTEGRAL_TYPE_P (TREE_TYPE (@0))
> -	     && TYPE_OVERFLOW_UNDEFINED (TREE_TYPE (@0))))
> +	 || (INTEGRAL_TYPE_P (TREE_TYPE (@1))
> +	     && TYPE_OVERFLOW_UNDEFINED (TREE_TYPE (@1))))
>       (negate (convert @1)))))
>    (simplify
>     (minus (convert @0)
> @@ -1862,23 +1881,28 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
>  
>    /* (T)(P + A) - (T)(P + B) -> (T)A - (T)B */
>    (simplify
> -   (minus (convert (plus:c @0 @1))
> +   (minus (convert (plus:c @@0 @1))
>      (convert (plus:c @0 @2)))
>     (if (INTEGRAL_TYPE_P (type)
>  	&& TYPE_OVERFLOW_UNDEFINED (type)
> -	&& element_precision (type) <= element_precision (TREE_TYPE (@1)))
> +	&& element_precision (type) <= element_precision (TREE_TYPE (@1))
> +	&& element_precision (type) <= element_precision (TREE_TYPE (@2)))
>      (with { tree utype = unsigned_type_for (type); }
>       (convert (minus (convert:utype @1) (convert:utype @2))))
> -    (if (element_precision (type) <= element_precision (TREE_TYPE (@1))
> -	 /* For integer types, if A has a smaller type
> -	    than T the result depends on the possible
> -	    overflow in P + A.
> -	    E.g. T=size_t, A=(unsigned)429497295, P>0.
> -	    However, if an overflow in P + A would cause
> -	    undefined behavior, we can assume that there
> -	    is no overflow.  */
> -	 || (INTEGRAL_TYPE_P (TREE_TYPE (@0))
> -	     && TYPE_OVERFLOW_UNDEFINED (TREE_TYPE (@0))))
> +    (if (((element_precision (type) <= element_precision (TREE_TYPE (@1)))
> +	  == (element_precision (type) <= element_precision (TREE_TYPE (@1))))
> +	 && (element_precision (type) <= element_precision (TREE_TYPE (@1))

Huh.  This looks like a copy&paste error (@1 vs. @2?).

> +	     /* For integer types, if A has a smaller type
> +		than T the result depends on the possible
> +		overflow in P + A.
> +		E.g. T=size_t, A=(unsigned)429497295, P>0.
> +		However, if an overflow in P + A would cause
> +		undefined behavior, we can assume that there
> +		is no overflow.  */
> +	     || (INTEGRAL_TYPE_P (TREE_TYPE (@1))
> +		 && INTEGRAL_TYPE_P (TREE_TYPE (@2))
> +		 && TYPE_OVERFLOW_UNDEFINED (TREE_TYPE (@1))
> +		 && TYPE_OVERFLOW_UNDEFINED (TREE_TYPE (@2)))))
>       (minus (convert @1) (convert @2)))))
>    (simplify
>     (minus (convert (pointer_plus @@0 @1))
> --- gcc/testsuite/gcc.dg/pr81281-3.c.jj	2017-12-07 15:19:06.334840988 +0100
> +++ gcc/testsuite/gcc.dg/pr81281-3.c	2017-12-07 14:38:25.000000000 +0100
> @@ -0,0 +1,102 @@
> +/* PR sanitizer/81281 */
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -fdump-tree-optimized" } */
> +/* { dg-final { scan-tree-dump-not "\[+=-] \?123\[ ;]" "optimized" } } */
> +
> +#ifdef __SIZEOF_INT128__
> +__int128
> +f1 (int a, long long b)
> +{
> +  __int128 f = 123 + a;
> +  __int128 g = 123 + b;
> +  return f - g;
> +}
> +#endif
> +
> +signed char
> +f2 (int a, long long b)
> +{
> +  signed char f = 123 + a;
> +  signed char g = 123 + b;
> +  return f - g;
> +}
> +
> +signed char
> +f3 (unsigned int a, unsigned long long b)
> +{
> +  signed char f = 123 + a;
> +  signed char g = 123 + b;
> +  return f - g;
> +}
> +
> +unsigned char
> +f4 (unsigned int a, unsigned long long b)
> +{
> +  unsigned char f = 123 + a;
> +  unsigned char g = 123 + b;
> +  return f - g;
> +}
> +
> +long long
> +f5 (int a)
> +{
> +  long long f = 123 + a;
> +  long long g = 123;
> +  return f - g;
> +}
> +
> +signed char
> +f6 (long long a)
> +{
> +  signed char f = 123 + a;
> +  signed char g = 123;
> +  return f - g;
> +}
> +
> +signed char
> +f7 (unsigned int a)
> +{
> +  signed char f = 123 + a;
> +  signed char g = 123;
> +  return f - g;
> +}
> +
> +unsigned char
> +f8 (unsigned long int a)
> +{
> +  unsigned char f = 123 + a;
> +  unsigned char g = 123;
> +  return f - g;
> +}
> +
> +long long
> +f9 (int a)
> +{
> +  long long f = 123;
> +  long long g = 123 + a;
> +  return f - g;
> +}
> +
> +signed char
> +f10 (long long a)
> +{
> +  signed char f = 123;
> +  signed char g = 123 + a;
> +  return f - g;
> +}
> +
> +signed char
> +f11 (unsigned int a)
> +{
> +  signed char f = 123;
> +  signed char g = 123 + a;
> +  return f - g;
> +}
> +
> +unsigned char
> +f12 (unsigned long int a)
> +{
> +  unsigned char f = 123;
> +  unsigned char g = 123 + a;
> +  return f - g;
> +}
> 
> 	Jakub
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)