This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] Fix reassociation (PR tree-optimization/65216)


On February 26, 2015 8:13:46 PM CET, Jakub Jelinek <jakub@redhat.com> wrote:
>Hi!
>
>On the following testcase, there is a redundant & 1 and when
>reassociating,
>we remove the redundant & 1.  The outer 2 ops are the same, so changed
>is still false when we reach rewrite_expr_tree with the last 2 ops
>(i.e.
>last statement).  But, as we don't keep that statement as is nor just
>swap
>the arguments, that isn't a guarantee that the SSA_NAME has always the
>same
>value as previously (in this case we've dropped the & 1 there), so the
>following patch ensures we don't reuse the SSA_NAME unless:
>1) we don't change the stmt at all
>2) we just swap the two arguments
>3) opindex == 0 (reassociate_bb doesn't really expect to see a new
>SSA_NAME
>if it hasn't called it with changed = true, and in that case the
>SSA_NAME
>   is necessarily always the same as before the optimization, otherwise
>   the optimization would be wrong.
>This way, we don't preserve bogus VR info, nor generate incorrect debug
>info.
>
>Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

Ok.

Thanks,
Richard.

>2015-02-26  Jakub Jelinek  <jakub@redhat.com>
>
>	PR tree-optimization/65216
>	* tree-ssa-reassoc.c (rewrite_expr_tree): Force creation of
>	new stmt and new SSA_NAME for lhs whenever the arguments have
>	changed and weren't just swapped.  Fix comment typo.
>
>	* gcc.c-torture/execute/pr65216.c: New test.
>
>--- gcc/tree-ssa-reassoc.c.jj	2015-02-14 09:21:56.000000000 +0100
>+++ gcc/tree-ssa-reassoc.c	2015-02-26 13:03:24.308614169 +0100
>@@ -3532,7 +3532,7 @@ rewrite_expr_tree (gimple stmt, unsigned
> 
>   /* The final recursion case for this function is that you have
>      exactly two operations left.
>-     If we had one exactly one op in the entire list to start with, we
>+     If we had exactly one op in the entire list to start with, we
>      would have never called this function, and the tail recursion
>      rewrites them one at a time.  */
>   if (opindex + 2 == ops.length ())
>@@ -3553,7 +3553,11 @@ rewrite_expr_tree (gimple stmt, unsigned
> 	      print_gimple_stmt (dump_file, stmt, 0, 0);
> 	    }
> 
>-	  if (changed)
>+	  /* Even when changed is false, reassociation could have e.g.
>removed
>+	     some redundant operations, so unless we are just swapping the
>+	     arguments or unless there is no change at all (then we just
>+	     return lhs), force creation of a new SSA_NAME.  */
>+	  if (changed || ((rhs1 != oe2->op || rhs2 != oe1->op) && opindex))
> 	    {
>	      gimple insert_point = find_insert_point (stmt, oe1->op,
>oe2->op);
> 	      lhs = make_ssa_name (TREE_TYPE (lhs));
>--- gcc/testsuite/gcc.c-torture/execute/pr65216.c.jj	2015-02-26
>13:05:12.199816826 +0100
>+++ gcc/testsuite/gcc.c-torture/execute/pr65216.c	2015-02-26
>13:04:53.000000000 +0100
>@@ -0,0 +1,20 @@
>+/* PR tree-optimization/65216 */
>+
>+int a, b = 62, e;
>+volatile int c, d;
>+
>+int
>+main ()
>+{
>+  int f = 0;
>+  for (a = 0; a < 2; a++)
>+    {
>+      b &= (8 ^ f) & 1;
>+      for (e = 0; e < 6; e++)
>+	if (c)
>+	  f = d;
>+    }
>+  if (b != 0)
>+    __builtin_abort ();
>+  return 0;
>+}
>
>	Jakub



Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]