diff mbox

Fix reassoc SSA_NAME reuse (PR tree-optimization/81555, PR tree-optimization/81556)

Message ID 20170727081917.GA2123@tucnak
State New
Headers show

Commit Message

Jakub Jelinek July 27, 2017, 8:19 a.m. UTC
Hi!

Reassoc non-parallel rewrite_expr_tree uses changed flag to determine
if SSA_NAMEs can't be reused.  It is initialized with detected powi or
negate optimization and during recursion set also if we detect
oe->op != rhs2.  This works well if there are no redundant operations
removed from the ops list, if the N outermost stmts have the same rhs2
after reassoc as before, then it really should have the same value and thus
we can reuse the SSA_NAME with its remembered value range etc.
The problem is on the following testcase, where there is a redundant
operation optimized away.  The outermost (latest) stmt's lhs can still be
reused, that is the final value of the whole computation, but for the rest
we don't really know where the removed operation has been.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk/7.2?

2017-07-27  Jakub Jelinek  <jakub@redhat.com>

	PR tree-optimization/81555
	PR tree-optimization/81556
	* tree-ssa-reassoc.c (rewrite_expr_tree): Add NEXT_CHANGED argument,
	if true, force CHANGED for the recursive invocation.
	(reassociate_bb): Remember original length of ops array, pass
	len != orig_len as NEXT_CHANGED in rewrite_expr_tree call.

	* gcc.c-torture/execute/pr81555.c: New test.
	* gcc.c-torture/execute/pr81556.c: New test.


	Jakub

Comments

Richard Biener July 27, 2017, 8:23 a.m. UTC | #1
On Thu, 27 Jul 2017, Jakub Jelinek wrote:

> Hi!
> 
> Reassoc non-parallel rewrite_expr_tree uses changed flag to determine
> if SSA_NAMEs can't be reused.  It is initialized with detected powi or
> negate optimization and during recursion set also if we detect
> oe->op != rhs2.  This works well if there are no redundant operations
> removed from the ops list, if the N outermost stmts have the same rhs2
> after reassoc as before, then it really should have the same value and thus
> we can reuse the SSA_NAME with its remembered value range etc.
> The problem is on the following testcase, where there is a redundant
> operation optimized away.  The outermost (latest) stmt's lhs can still be
> reused, that is the final value of the whole computation, but for the rest
> we don't really know where the removed operation has been.
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk/7.2?

Please document the new argument to rewrite_expr_tree.

Ok with that change.

Thanks,
Richard.

> 2017-07-27  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR tree-optimization/81555
> 	PR tree-optimization/81556
> 	* tree-ssa-reassoc.c (rewrite_expr_tree): Add NEXT_CHANGED argument,
> 	if true, force CHANGED for the recursive invocation.
> 	(reassociate_bb): Remember original length of ops array, pass
> 	len != orig_len as NEXT_CHANGED in rewrite_expr_tree call.
> 
> 	* gcc.c-torture/execute/pr81555.c: New test.
> 	* gcc.c-torture/execute/pr81556.c: New test.
> 
> --- gcc/tree-ssa-reassoc.c.jj	2017-06-30 09:49:32.000000000 +0200
> +++ gcc/tree-ssa-reassoc.c	2017-07-26 11:24:59.279726267 +0200
> @@ -4209,7 +4209,7 @@ insert_stmt_before_use (gimple *stmt, gi
>  
>  static tree
>  rewrite_expr_tree (gimple *stmt, unsigned int opindex,
> -		   vec<operand_entry *> ops, bool changed)
> +		   vec<operand_entry *> ops, bool changed, bool next_changed)
>  {
>    tree rhs1 = gimple_assign_rhs1 (stmt);
>    tree rhs2 = gimple_assign_rhs2 (stmt);
> @@ -4300,7 +4300,8 @@ rewrite_expr_tree (gimple *stmt, unsigne
>       be the non-leaf side.  */
>    tree new_rhs1
>      = rewrite_expr_tree (SSA_NAME_DEF_STMT (rhs1), opindex + 1, ops,
> -			 changed || oe->op != rhs2);
> +			 changed || oe->op != rhs2 || next_changed,
> +			 false);
>  
>    if (oe->op != rhs2 || new_rhs1 != rhs1)
>      {
> @@ -5654,6 +5655,7 @@ reassociate_bb (basic_block bb)
>  	      gimple_set_visited (stmt, true);
>  	      linearize_expr_tree (&ops, stmt, true, true);
>  	      ops.qsort (sort_by_operand_rank);
> +	      int orig_len = ops.length ();
>  	      optimize_ops_list (rhs_code, &ops);
>  	      if (undistribute_ops_list (rhs_code, &ops,
>  					 loop_containing_stmt (stmt)))
> @@ -5744,7 +5746,8 @@ reassociate_bb (basic_block bb)
>  
>  		      new_lhs = rewrite_expr_tree (stmt, 0, ops,
>  						   powi_result != NULL
> -						   || negate_result);
> +						   || negate_result,
> +						   len != orig_len);
>                      }
>  
>  		  /* If we combined some repeated factors into a 
> --- gcc/testsuite/gcc.c-torture/execute/pr81555.c.jj	2017-07-26 11:39:59.427708268 +0200
> +++ gcc/testsuite/gcc.c-torture/execute/pr81555.c	2017-07-26 11:39:32.000000000 +0200
> @@ -0,0 +1,24 @@
> +/* PR tree-optimization/81555 */
> +
> +unsigned int a = 1, d = 0xfaeU, e = 0xe376U;
> +_Bool b = 0, f = 1;
> +unsigned char g = 1;
> +
> +void
> +foo (void)
> +{
> +  _Bool c = a != b;
> +  if (c)
> +    f = 0;
> +  if (e & c & (unsigned char)d & c)
> +    g = 0;
> +}
> +
> +int
> +main ()
> +{
> +  foo ();
> +  if (f || g != 1)
> +    __builtin_abort ();
> +  return 0;
> +}
> --- gcc/testsuite/gcc.c-torture/execute/pr81556.c.jj	2017-07-26 11:35:51.054748405 +0200
> +++ gcc/testsuite/gcc.c-torture/execute/pr81556.c	2017-07-26 11:41:24.512666811 +0200
> @@ -0,0 +1,23 @@
> +/* PR tree-optimization/81556 */
> +
> +unsigned long long int b = 0xb82ff73c5c020599ULL;
> +unsigned long long int c = 0xd4e8188733a29d8eULL;
> +unsigned long long int d = 2, f = 1, g = 0, h = 0;
> +unsigned long long int e = 0xf27771784749f32bULL;
> +
> +__attribute__((noinline, noclone)) void
> +foo (void)
> +{
> +  _Bool a = d > 1;
> +  g = f % ((d > 1) << 9);
> +  h = a & (e & (a & b & c));
> +}
> +
> +int
> +main ()
> +{
> +  foo ();
> +  if (g != 1 || h != 0)
> +    __builtin_abort ();
> +  return 0;
> +}
> 
> 	Jakub
> 
>
diff mbox

Patch

--- gcc/tree-ssa-reassoc.c.jj	2017-06-30 09:49:32.000000000 +0200
+++ gcc/tree-ssa-reassoc.c	2017-07-26 11:24:59.279726267 +0200
@@ -4209,7 +4209,7 @@  insert_stmt_before_use (gimple *stmt, gi
 
 static tree
 rewrite_expr_tree (gimple *stmt, unsigned int opindex,
-		   vec<operand_entry *> ops, bool changed)
+		   vec<operand_entry *> ops, bool changed, bool next_changed)
 {
   tree rhs1 = gimple_assign_rhs1 (stmt);
   tree rhs2 = gimple_assign_rhs2 (stmt);
@@ -4300,7 +4300,8 @@  rewrite_expr_tree (gimple *stmt, unsigne
      be the non-leaf side.  */
   tree new_rhs1
     = rewrite_expr_tree (SSA_NAME_DEF_STMT (rhs1), opindex + 1, ops,
-			 changed || oe->op != rhs2);
+			 changed || oe->op != rhs2 || next_changed,
+			 false);
 
   if (oe->op != rhs2 || new_rhs1 != rhs1)
     {
@@ -5654,6 +5655,7 @@  reassociate_bb (basic_block bb)
 	      gimple_set_visited (stmt, true);
 	      linearize_expr_tree (&ops, stmt, true, true);
 	      ops.qsort (sort_by_operand_rank);
+	      int orig_len = ops.length ();
 	      optimize_ops_list (rhs_code, &ops);
 	      if (undistribute_ops_list (rhs_code, &ops,
 					 loop_containing_stmt (stmt)))
@@ -5744,7 +5746,8 @@  reassociate_bb (basic_block bb)
 
 		      new_lhs = rewrite_expr_tree (stmt, 0, ops,
 						   powi_result != NULL
-						   || negate_result);
+						   || negate_result,
+						   len != orig_len);
                     }
 
 		  /* If we combined some repeated factors into a 
--- gcc/testsuite/gcc.c-torture/execute/pr81555.c.jj	2017-07-26 11:39:59.427708268 +0200
+++ gcc/testsuite/gcc.c-torture/execute/pr81555.c	2017-07-26 11:39:32.000000000 +0200
@@ -0,0 +1,24 @@ 
+/* PR tree-optimization/81555 */
+
+unsigned int a = 1, d = 0xfaeU, e = 0xe376U;
+_Bool b = 0, f = 1;
+unsigned char g = 1;
+
+void
+foo (void)
+{
+  _Bool c = a != b;
+  if (c)
+    f = 0;
+  if (e & c & (unsigned char)d & c)
+    g = 0;
+}
+
+int
+main ()
+{
+  foo ();
+  if (f || g != 1)
+    __builtin_abort ();
+  return 0;
+}
--- gcc/testsuite/gcc.c-torture/execute/pr81556.c.jj	2017-07-26 11:35:51.054748405 +0200
+++ gcc/testsuite/gcc.c-torture/execute/pr81556.c	2017-07-26 11:41:24.512666811 +0200
@@ -0,0 +1,23 @@ 
+/* PR tree-optimization/81556 */
+
+unsigned long long int b = 0xb82ff73c5c020599ULL;
+unsigned long long int c = 0xd4e8188733a29d8eULL;
+unsigned long long int d = 2, f = 1, g = 0, h = 0;
+unsigned long long int e = 0xf27771784749f32bULL;
+
+__attribute__((noinline, noclone)) void
+foo (void)
+{
+  _Bool a = d > 1;
+  g = f % ((d > 1) << 9);
+  h = a & (e & (a & b & c));
+}
+
+int
+main ()
+{
+  foo ();
+  if (g != 1 || h != 0)
+    __builtin_abort ();
+  return 0;
+}