Re: [PATCH][match-and-simplify] Change back default behavior of fold_stmt

[Richard Biener <rguenther at suse dot de>]

On Tue, 14 Oct 2014, Richard Biener wrote:

> On Tue, 14 Oct 2014, Richard Biener wrote:
> 
> > 
> > This changes default behavior of fold_stmt back to _not_ following
> > SSA use-def chains when trying to simplify things.  I had to force
> > that already for one caller and for the merge to trunk I'd rather
> > not track down issues in every other existing caller.
> > 
> > This means that fold_stmt will not become more powerful, at least for now.
> > I still hope to get rid of its use of fold() during the merge process.
> > 
> > Bootstrap and regtest ongoing on x86_64-unknown-linux-gnu.
> > 
> > (yeah, I'm preparing a first batch of changes to merge from the
> > branch)
> 
> Unfortunately this exposes an issue with combining our SSA propagators
> with pattern matching which makes us miscompile tree-vect-generic.c
> from VRP.  Consider
> 
> Visiting PHI node: i_137 = PHI <0(51), i_48(63)>
>     Argument #0 (51 -> 52 executable)
>             0: [0, 0]
>     Argument #1 (63 -> 52 not executable)
> Found new range for i_137: [0, 0]
> ...
> i_48 = delta_25 + i_137;
> Found new range for i_48: VARYING
> _67 = (unsigned int) delta_25;
> Found new range for _67: [0, +INF]
> _78 = (unsigned int) i_48;
> Found new range for _78: [0, +INF]
> _257 = _78 - _67;
> (unsigned int) (delta_25 + i_137) - (unsigned int) delta_25
> Match-and-simplified _78 - _67 to 0
> Found new range for _257: [0, 0]
> 
> now after i_137 is revisited and it becomes VARYING the SSA propagator
> stops at i_48 because its value does not change.  Thus it fails to
> re-visit _257 where a pattern was applied that used the optimistic
> value of i_137 to its advantage.
> 
> The following patch makes sure SSA propagators (CCP and VRP) do
> not get any benefit during their propagation phase from
> match-and-simplify by disabling the following of SSA use-def edges.
> 
> Bootstrapped on x86_64-unknown-linux-gnu, testing in progress.
> 
> Richard.

And here is the patch.

Richard.

2014-10-14  Richard Biener  <rguenther@suse.de>

	* gimple-fold.h (no_follow_ssa_edges): Declare.
	(gimple_fold_stmt_to_constant_1): Add separate valueize hook for
	gimple_simplify, defaulted to no_follow_ssa_edges.
	* gimple-fold.c (fold_stmt): Make old API never follow SSA edges
	when simplifying.
	(no_follow_ssa_edges): New function.
	(gimple_fold_stmt_to_constant_1): Adjust.
	* tree-cfg.c (no_follow_ssa_edges): Remove.
	(replace_uses_by): Use plain fold_stmt again.
	* gimple-match-head.c (gimple_simplify): When simplifying
	a statement do not stop when valueizing its operands yields NULL.

Index: gcc/gimple-fold.h
===================================================================
--- gcc/gimple-fold.h	(revision 216146)
+++ gcc/gimple-fold.h	(working copy)
@@ -32,7 +32,9 @@ extern tree maybe_fold_and_comparisons (
 					enum tree_code, tree, tree);
 extern tree maybe_fold_or_comparisons (enum tree_code, tree, tree,
 				       enum tree_code, tree, tree);
-extern tree gimple_fold_stmt_to_constant_1 (gimple, tree (*) (tree));
+extern tree no_follow_ssa_edges (tree);
+extern tree gimple_fold_stmt_to_constant_1 (gimple, tree (*) (tree),
+					    tree (*) (tree) = no_follow_ssa_edges);
 extern tree gimple_fold_stmt_to_constant (gimple, tree (*) (tree));
 extern tree fold_const_aggregate_ref_1 (tree, tree (*) (tree));
 extern tree fold_const_aggregate_ref (tree);
Index: gcc/gimple-fold.c
===================================================================
--- gcc/gimple-fold.c	(revision 216146)
+++ gcc/gimple-fold.c	(working copy)
@@ -3136,6 +3136,14 @@ fail:
   return changed;
 }
 
+/* Valueziation callback that ends up not following SSA edges.  */
+
+tree
+no_follow_ssa_edges (tree)
+{
+  return NULL_TREE;
+}
+
 /* Fold the statement pointed to by GSI.  In some cases, this function may
    replace the whole statement with a new one.  Returns true iff folding
    makes any changes.
@@ -3146,7 +3154,7 @@ fail:
 bool
 fold_stmt (gimple_stmt_iterator *gsi)
 {
-  return fold_stmt_1 (gsi, false, NULL);
+  return fold_stmt_1 (gsi, false, no_follow_ssa_edges);
 }
 
 bool
@@ -3167,7 +3175,7 @@ bool
 fold_stmt_inplace (gimple_stmt_iterator *gsi)
 {
   gimple stmt = gsi_stmt (*gsi);
-  bool changed = fold_stmt_1 (gsi, true, NULL);
+  bool changed = fold_stmt_1 (gsi, true, no_follow_ssa_edges);
   gcc_assert (gsi_stmt (*gsi) == stmt);
   return changed;
 }
@@ -4527,12 +4535,19 @@ gimple_fold_stmt_to_constant_2 (gimple s
     }
 }
 
+/* ???  The SSA propagators do not correctly deal with following SSA use-def
+   edges if there are intermediate VARYING defs.  For this reason
+   there are two valueization hooks here, one for the legacy code
+   in gimple_fold_stmt_to_constant_2 and one for gimple_simplify
+   which is defaulted to no_follow_ssa_edges.  */
+
 tree
-gimple_fold_stmt_to_constant_1 (gimple stmt, tree (*valueize) (tree))
+gimple_fold_stmt_to_constant_1 (gimple stmt, tree (*valueize) (tree),
+				tree (*gvalueize) (tree))
 {
   code_helper rcode;
   tree ops[3] = {};
-  if (gimple_simplify (stmt, &rcode, ops, NULL, valueize)
+  if (gimple_simplify (stmt, &rcode, ops, NULL, gvalueize)
       && rcode.is_tree_code ()
       && (TREE_CODE_LENGTH ((tree_code) rcode) == 0
 	  || ((tree_code) rcode) == ADDR_EXPR)
Index: gcc/tree-cfg.c
===================================================================
--- gcc/tree-cfg.c	(revision 216146)
+++ gcc/tree-cfg.c	(working copy)
@@ -1709,14 +1709,6 @@ gimple_can_merge_blocks_p (basic_block a
   return true;
 }
 
-/* ???  Maybe this should be a generic overload of fold_stmt.  */
-
-static tree
-no_follow_ssa_edges (tree)
-{
-  return NULL_TREE;
-}
-
 /* Replaces all uses of NAME by VAL.  */
 
 void
@@ -1773,17 +1765,7 @@ replace_uses_by (tree name, tree val)
 		  recompute_tree_invariant_for_addr_expr (op);
 	      }
 
-	  /* If we have sth like
-	       neighbor_29 = <name> + -1;
-	       _33 = <name> + neighbor_29;
-	     and substitute 1 for <name> then when visiting
-	     _33 first then folding will simplify the stmt
-	     to _33 = <name>; and the new immediate use will
-	     be inserted before the stmt iterator marker and
-	     thus we fail to visit it again, ICEing within the
-	     has_zero_uses assert.
-	     Avoid that by never following SSA edges.  */
-	  if (fold_stmt (&gsi, no_follow_ssa_edges))
+	  if (fold_stmt (&gsi))
 	    stmt = gsi_stmt (gsi);
 
 	  if (maybe_clean_or_replace_eh_stmt (orig_stmt, stmt))
Index: gcc/gimple-match-head.c
===================================================================
--- gcc/gimple-match-head.c	(revision 216146)
+++ gcc/gimple-match-head.c	(working copy)
@@ -595,9 +595,9 @@ gimple_simplify (gimple stmt,
 		tree op0 = TREE_OPERAND (gimple_assign_rhs1 (stmt), 0);
 		if (valueize && TREE_CODE (op0) == SSA_NAME)
 		  {
-		    op0 = valueize (op0);
-		    if (!op0)
-		      return false;
+		    tree tem = valueize (op0);
+		    if (tem)
+		      op0 = tem;
 		  }
 		*rcode = code;
 		ops[0] = op0;
@@ -609,9 +609,9 @@ gimple_simplify (gimple stmt,
 		tree op0 = TREE_OPERAND (rhs1, 0);
 		if (valueize && TREE_CODE (op0) == SSA_NAME)
 		  {
-		    op0 = valueize (op0);
-		    if (!op0)
-		      return false;
+		    tree tem = valueize (op0);
+		    if (tem)
+		      op0 = tem;
 		  }
 		*rcode = code;
 		ops[0] = op0;
@@ -636,9 +636,9 @@ gimple_simplify (gimple stmt,
 	      tree rhs1 = gimple_assign_rhs1 (stmt);
 	      if (valueize && TREE_CODE (rhs1) == SSA_NAME)
 		{
-		  rhs1 = valueize (rhs1);
-		  if (!rhs1)
-		    return false;
+		  tree tem = valueize (rhs1);
+		  if (tem)
+		    rhs1 = tem;
 		}
 	      *rcode = code;
 	      ops[0] = rhs1;
@@ -649,16 +649,16 @@ gimple_simplify (gimple stmt,
 	      tree rhs1 = gimple_assign_rhs1 (stmt);
 	      if (valueize && TREE_CODE (rhs1) == SSA_NAME)
 		{
-		  rhs1 = valueize (rhs1);
-		  if (!rhs1)
-		    return false;
+		  tree tem = valueize (rhs1);
+		  if (tem)
+		    rhs1 = tem;
 		}
 	      tree rhs2 = gimple_assign_rhs2 (stmt);
 	      if (valueize && TREE_CODE (rhs2) == SSA_NAME)
 		{
-		  rhs2 = valueize (rhs2);
-		  if (!rhs2)
-		    return false;
+		  tree tem = valueize (rhs2);
+		  if (tem)
+		    rhs2 = tem;
 		}
 	      *rcode = code;
 	      ops[0] = rhs1;
@@ -670,23 +670,23 @@ gimple_simplify (gimple stmt,
 	      tree rhs1 = gimple_assign_rhs1 (stmt);
 	      if (valueize && TREE_CODE (rhs1) == SSA_NAME)
 		{
-		  rhs1 = valueize (rhs1);
-		  if (!rhs1)
-		    return false;
+		  tree tem = valueize (rhs1);
+		  if (tem)
+		    rhs1 = tem;
 		}
 	      tree rhs2 = gimple_assign_rhs2 (stmt);
 	      if (valueize && TREE_CODE (rhs2) == SSA_NAME)
 		{
-		  rhs2 = valueize (rhs2);
-		  if (!rhs2)
-		    return false;
+		  tree tem = valueize (rhs2);
+		  if (tem)
+		    rhs2 = tem;
 		}
 	      tree rhs3 = gimple_assign_rhs3 (stmt);
 	      if (valueize && TREE_CODE (rhs3) == SSA_NAME)
 		{
-		  rhs3 = valueize (rhs3);
-		  if (!rhs3)
-		    return false;
+		  tree tem = valueize (rhs3);
+		  if (tem)
+		    rhs3 = tem;
 		}
 	      *rcode = code;
 	      ops[0] = rhs1;
@@ -708,9 +708,12 @@ gimple_simplify (gimple stmt,
 	  /* ???  Internal function support missing.  */
 	  if (!fn)
 	    return false;
-	  if (TREE_CODE (fn) == SSA_NAME
-	      && valueize)
-	    fn = valueize (fn);
+	  if (valueize && TREE_CODE (fn) == SSA_NAME)
+	    {
+	      tree tem = valueize (fn);
+	      if (tem)
+		fn = tem;
+	    }
 	  if (!fn
 	      || TREE_CODE (fn) != ADDR_EXPR
 	      || TREE_CODE (TREE_OPERAND (fn, 0)) != FUNCTION_DECL
@@ -729,9 +732,9 @@ gimple_simplify (gimple stmt,
 		tree arg1 = gimple_call_arg (stmt, 0);
 		if (valueize && TREE_CODE (arg1) == SSA_NAME)
 		  {
-		    arg1 = valueize (arg1);
-		    if (!arg1)
-		      return false;
+		    tree tem = valueize (arg1);
+		    if (tem)
+		      arg1 = tem;
 		  }
 		*rcode = DECL_FUNCTION_CODE (decl);
 		ops[0] = arg1;
@@ -742,16 +745,16 @@ gimple_simplify (gimple stmt,
 		tree arg1 = gimple_call_arg (stmt, 0);
 		if (valueize && TREE_CODE (arg1) == SSA_NAME)
 		  {
-		    arg1 = valueize (arg1);
-		    if (!arg1)
-		      return false;
+		    tree tem = valueize (arg1);
+		    if (tem)
+		      arg1 = tem;
 		  }
 		tree arg2 = gimple_call_arg (stmt, 1);
 		if (valueize && TREE_CODE (arg2) == SSA_NAME)
 		  {
-		    arg2 = valueize (arg2);
-		    if (!arg2)
-		      return false;
+		    tree tem = valueize (arg2);
+		    if (tem)
+		      arg2 = tem;
 		  }
 		*rcode = DECL_FUNCTION_CODE (decl);
 		ops[0] = arg1;
@@ -763,23 +766,23 @@ gimple_simplify (gimple stmt,
 		tree arg1 = gimple_call_arg (stmt, 0);
 		if (valueize && TREE_CODE (arg1) == SSA_NAME)
 		  {
-		    arg1 = valueize (arg1);
-		    if (!arg1)
-		      return false;
+		    tree tem = valueize (arg1);
+		    if (tem)
+		      arg1 = tem;
 		  }
 		tree arg2 = gimple_call_arg (stmt, 1);
 		if (valueize && TREE_CODE (arg2) == SSA_NAME)
 		  {
-		    arg2 = valueize (arg2);
-		    if (!arg2)
-		      return false;
+		    tree tem = valueize (arg2);
+		    if (tem)
+		      arg2 = tem;
 		  }
 		tree arg3 = gimple_call_arg (stmt, 2);
 		if (valueize && TREE_CODE (arg3) == SSA_NAME)
 		  {
-		    arg3 = valueize (arg3);
-		    if (!arg3)
-		      return false;
+		    tree tem = valueize (arg3);
+		    if (tem)
+		      arg3 = tem;
 		  }
 		*rcode = DECL_FUNCTION_CODE (decl);
 		ops[0] = arg1;
@@ -798,16 +801,16 @@ gimple_simplify (gimple stmt,
 	tree lhs = gimple_cond_lhs (stmt);
 	if (valueize && TREE_CODE (lhs) == SSA_NAME)
 	  {
-	    lhs = valueize (lhs);
-	    if (!lhs)
-	      return false;
+	    tree tem = valueize (lhs);
+	    if (tem)
+	      lhs = tem;
 	  }
 	tree rhs = gimple_cond_rhs (stmt);
 	if (valueize && TREE_CODE (rhs) == SSA_NAME)
 	  {
-	    rhs = valueize (rhs);
-	    if (!rhs)
-	      return false;
+	    tree tem = valueize (rhs);
+	    if (tem)
+	      rhs = tem;
 	  }
 	*rcode = gimple_cond_code (stmt);
 	ops[0] = lhs;