[PATCH] Fix PR 29715, fold produces &a - 4

[PATCH] Fix PR 29715, fold produces &a - 4

[Andrew_Pinski]

[-- Attachment #1: Type: text/plain, Size: 1006 bytes --]

Hi,
  The problem here is that fold assumes that for ptr-- > a, fold assumes 
that the pointers will not overflow which is true but that is wrong as you 
can produce an extra overflow by doing this optimization if a was 
((ptr_type)-1).  This can also cause us to produce &a[-1] which is 
actually undefined in our IR.

This patch removes the check that the type is a pointer and for the 
testcase in the bug report (which was found in the testsuite already), we 
produce better code now on the tree level in that we just generate a 
return 0 if we change the second return to f();.  Also this fixes a 
regression on the ptr_plus_expr branch as we would try to generate a 
MINUS_EXPR with a Pointer type which is invalid on that branch.

OK? Bootstrapped and tested on i686-linux-gnu on the trunk with no 
regressions.


Thanks,
Andrew Pinski

ChangeLog:

        * fold-const.c (fold_comparison): For the
        POSTDECREMENT_EXPR/POSTINCREMENT_EXPR optimization, don't
        do it for Pointer types.


[-- Attachment #2: fixpr29715.diff.txt --]
[-- Type: text/plain, Size: 862 bytes --]

Index: fold-const.c
===================================================================
--- fold-const.c	(revision 122422)
+++ fold-const.c	(working copy)
@@ -8641,11 +8641,9 @@ fold_comparison (enum tree_code code, tr
 	  || TREE_CODE (arg0) == POSTDECREMENT_EXPR)
       /* This optimization is invalid for ordered comparisons
          if CONST+INCR overflows or if foo+incr might overflow.
-	 This optimization is invalid for floating point due to rounding.
-	 For pointer types we assume overflow doesn't happen.  */
-      && (POINTER_TYPE_P (TREE_TYPE (arg0))
-	  || (INTEGRAL_TYPE_P (TREE_TYPE (arg0))
-	      && (code == EQ_EXPR || code == NE_EXPR))))
+	 This optimization is invalid for floating point due to rounding. */
+      && (INTEGRAL_TYPE_P (TREE_TYPE (arg0))
+	  && (code == EQ_EXPR || code == NE_EXPR)))
     {
       tree varop, newconst;
 

Re: [PATCH] Fix PR 29715, fold produces &a - 4

[Richard Guenther]

On 3/1/07, Andrew_Pinski@playstation.sony.com
<Andrew_Pinski@playstation.sony.com> wrote:
> Hi,
>   The problem here is that fold assumes that for ptr-- > a, fold assumes
> that the pointers will not overflow which is true but that is wrong as you
> can produce an extra overflow by doing this optimization if a was
> ((ptr_type)-1).  This can also cause us to produce &a[-1] which is
> actually undefined in our IR.
>
> This patch removes the check that the type is a pointer and for the
> testcase in the bug report (which was found in the testsuite already), we
> produce better code now on the tree level in that we just generate a
> return 0 if we change the second return to f();.  Also this fixes a
> regression on the ptr_plus_expr branch as we would try to generate a
> MINUS_EXPR with a Pointer type which is invalid on that branch.
>
> OK? Bootstrapped and tested on i686-linux-gnu on the trunk with no
> regressions.

This optimization doesn't look too useful at all now that we gimplify all this
post/preinc/decrement anyway.  This is actually the only place where we look
at this operators in fold, so I suggest removing the whole
transformation instead.

Richard.

Re: [PATCH] Fix PR 29715, fold produces &a - 4

[Andrew Pinski]

[-- Attachment #1: Type: text/plain, Size: 1032 bytes --]

On 3/1/07, Richard Guenther <richard.guenther@gmail.com> wrote:
> This optimization doesn't look too useful at all now that we gimplify all this
> post/preinc/decrement anyway.  This is actually the only place where we look
> at this operators in fold, so I suggest removing the whole
> transformation instead.

Like this?  I agree that this is no longer needed as we do
gimplification and then optimize it later if we can (though sometimes
we don't because we don't recombined the PLUS with the comparision but
that is not actually tested :) ).

This is not the first time this code has bought troubles:
http://gcc.gnu.org/ml/gcc-patches/2004-03/msg00582.html
http://gcc.gnu.org/ml/gcc-patches/2000-07/msg00575.html
http://gcc.gnu.org/ml/gcc-patches/2004-03/msg02369.html


This was bootstrapped and tested many times on powerpc64-linux-gnu
with no regressions and built and tested for spu-elf with no
regressions.

Thanks,
Andrew Pinski

ChangeLog:
  * fold-const.c (fold_comparision): Remove the "foo++ == CONST" transformation.

[-- Attachment #2: fixfold.diff.txt --]
[-- Type: text/plain, Size: 2730 bytes --]

Index: fold-const.c
===================================================================
--- fold-const.c	(revision 124343)
+++ fold-const.c	(working copy)
@@ -8649,69 +8649,6 @@ fold_comparison (enum tree_code code, tr
 	}
     }
 
-  /* Convert foo++ == CONST into ++foo == CONST + INCR.  */
-  if (TREE_CONSTANT (arg1)
-      && (TREE_CODE (arg0) == POSTINCREMENT_EXPR
-	  || TREE_CODE (arg0) == POSTDECREMENT_EXPR)
-      /* This optimization is invalid for ordered comparisons
-         if CONST+INCR overflows or if foo+incr might overflow.
-	 This optimization is invalid for floating point due to rounding.
-	 For pointer types we assume overflow doesn't happen.  */
-      && (POINTER_TYPE_P (TREE_TYPE (arg0))
-	  || (INTEGRAL_TYPE_P (TREE_TYPE (arg0))
-	      && (code == EQ_EXPR || code == NE_EXPR))))
-    {
-      tree varop, newconst;
-
-      if (TREE_CODE (arg0) == POSTINCREMENT_EXPR)
-	{
-	  newconst = fold_build2 (PLUS_EXPR, TREE_TYPE (arg0),
-				  arg1, TREE_OPERAND (arg0, 1));
-	  varop = build2 (PREINCREMENT_EXPR, TREE_TYPE (arg0),
-			  TREE_OPERAND (arg0, 0),
-			  TREE_OPERAND (arg0, 1));
-	}
-      else
-	{
-	  newconst = fold_build2 (MINUS_EXPR, TREE_TYPE (arg0),
-				  arg1, TREE_OPERAND (arg0, 1));
-	  varop = build2 (PREDECREMENT_EXPR, TREE_TYPE (arg0),
-			  TREE_OPERAND (arg0, 0),
-			  TREE_OPERAND (arg0, 1));
-	}
-
-
-      /* If VAROP is a reference to a bitfield, we must mask
-	 the constant by the width of the field.  */
-      if (TREE_CODE (TREE_OPERAND (varop, 0)) == COMPONENT_REF
-	  && DECL_BIT_FIELD (TREE_OPERAND (TREE_OPERAND (varop, 0), 1))
-	  && host_integerp (DECL_SIZE (TREE_OPERAND
-					 (TREE_OPERAND (varop, 0), 1)), 1))
-	{
-	  tree fielddecl = TREE_OPERAND (TREE_OPERAND (varop, 0), 1);
-	  HOST_WIDE_INT size = tree_low_cst (DECL_SIZE (fielddecl), 1);
-	  tree folded_compare, shift;
-
-	  /* First check whether the comparison would come out
-	     always the same.  If we don't do that we would
-	     change the meaning with the masking.  */
-	  folded_compare = fold_build2 (code, type,
-					TREE_OPERAND (varop, 0), arg1);
-	  if (TREE_CODE (folded_compare) == INTEGER_CST)
-	    return omit_one_operand (type, folded_compare, varop);
-
-	  shift = build_int_cst (NULL_TREE,
-				 TYPE_PRECISION (TREE_TYPE (varop)) - size);
-	  shift = fold_convert (TREE_TYPE (varop), shift);
-	  newconst = fold_build2 (LSHIFT_EXPR, TREE_TYPE (varop),
-				  newconst, shift);
-	  newconst = fold_build2 (RSHIFT_EXPR, TREE_TYPE (varop),
-				  newconst, shift);
-	}
-
-      return fold_build2 (code, type, varop, newconst);
-    }
-
   if (TREE_CODE (TREE_TYPE (arg0)) == INTEGER_TYPE
       && (TREE_CODE (arg0) == NOP_EXPR
 	  || TREE_CODE (arg0) == CONVERT_EXPR))

Re: [PATCH] Fix PR 29715, fold produces &a - 4

[Richard Guenther]

On 5/2/07, Andrew Pinski <pinskia@gmail.com> wrote:
> On 3/1/07, Richard Guenther <richard.guenther@gmail.com> wrote:
> > This optimization doesn't look too useful at all now that we gimplify all this
> > post/preinc/decrement anyway.  This is actually the only place where we look
> > at this operators in fold, so I suggest removing the whole
> > transformation instead.
>
> Like this?  I agree that this is no longer needed as we do
> gimplification and then optimize it later if we can (though sometimes
> we don't because we don't recombined the PLUS with the comparision but
> that is not actually tested :) ).
>
> This is not the first time this code has bought troubles:
> http://gcc.gnu.org/ml/gcc-patches/2004-03/msg00582.html
> http://gcc.gnu.org/ml/gcc-patches/2000-07/msg00575.html
> http://gcc.gnu.org/ml/gcc-patches/2004-03/msg02369.html
>
>
> This was bootstrapped and tested many times on powerpc64-linux-gnu
> with no regressions and built and tested for spu-elf with no
> regressions.

This is ok.

Thanks,
Richard.

> Thanks,
> Andrew Pinski
>
> ChangeLog:
>   * fold-const.c (fold_comparision): Remove the "foo++ == CONST" transformation.
>
>