diff mbox

Fix gimple_fold_stmt_to_constant regression

Message ID alpine.LSU.2.11.1411141338380.374@zhemvz.fhfr.qr
State New
Headers show

Commit Message

Richard Biener Nov. 14, 2014, 12:39 p.m. UTC 
Following up https://gcc.gnu.org/ml/gcc-patches/2014-10/msg01233.html and
fixing the regressions this caused as soon as I removed the dispatch
to fold_unary (and more regressions it would have caused if I managed
to finish the idea to also remove the dispatches to fold_binary
and fold_ternary...) the following patch makes CCP and VRP follow
selected SSA edges again when gimple_fold_stmt_to_constant_1
dispatches to gimple_simplify.

The valueization for gimple_simplify of SSA propagator users may
both valueize to anything (in particular constants) and it may
signal to follow SSA edges if the destination will never be
visited again by the propagator (thus its lattice value is stable).
Esp. cutting out valueizing SSA names to constants is what caused
the regressions.

Note that this highlights the fact that overloading the valueization
result with the signal to (not) follow SSA edges isn't the very
best thing to do - for example we can't valueize to a SSA name
(like for looking through SSA copies) but at the same time say
that gimple_simplify shouldn't follow the edge to its definition.
This shouldn't be a serious limitation for CCP and VRP which
care about constants only - but it shows a defect in the
gimple_simplify interface.  I haven't yet concluded on a better
one though - options go from adding a secondary return to
the valueize hook to adding a second hook maybe with additionally
adding a simple flag to turn off SSA edge following globally.

Anyway - the following patch should fix the immediate regression
and allows to go forward with removing GENERIC folding from
both fold_stmt and gimple_fold_stmt_to_constant.  Just not
for this stage1 which will end too soon.

Bootstrapped on x86_64-unknown-linux-gnu, testing in progress.

Thanks,
Richard.
 
2014-11-14  Richard Biener  <rguenther@suse.de>

	* gimple-fold.h (gimple_fold_stmt_to_constant_1): Add 2nd
	valueization hook defaulted to no_follow_ssa_edges.
	* gimple-fold.c (gimple_fold_stmt_to_constant_1): Pass
	2nd valueization hook to gimple_simplify.
	* tree-ssa-ccp.c (valueize_op_1): New function to be
	used for gimple_simplify called via gimple_fold_stmt_to_constant_1.
	(ccp_fold): Adjust.
	* tree-vrp.c (vrp_valueize_1): New function to be
	used for gimple_simplify called via gimple_fold_stmt_to_constant_1.
	(vrp_visit_assignment_or_call): Adjust.

Comments

H.J. Lu Nov. 15, 2014, 11:03 p.m. UTC | #1 
On Fri, Nov 14, 2014 at 4:39 AM, Richard Biener <rguenther@suse.de> wrote:
>
> Following up https://gcc.gnu.org/ml/gcc-patches/2014-10/msg01233.html and
> fixing the regressions this caused as soon as I removed the dispatch
> to fold_unary (and more regressions it would have caused if I managed
> to finish the idea to also remove the dispatches to fold_binary
> and fold_ternary...) the following patch makes CCP and VRP follow
> selected SSA edges again when gimple_fold_stmt_to_constant_1
> dispatches to gimple_simplify.
>
> The valueization for gimple_simplify of SSA propagator users may
> both valueize to anything (in particular constants) and it may
> signal to follow SSA edges if the destination will never be
> visited again by the propagator (thus its lattice value is stable).
> Esp. cutting out valueizing SSA names to constants is what caused
> the regressions.
>
> Note that this highlights the fact that overloading the valueization
> result with the signal to (not) follow SSA edges isn't the very
> best thing to do - for example we can't valueize to a SSA name
> (like for looking through SSA copies) but at the same time say
> that gimple_simplify shouldn't follow the edge to its definition.
> This shouldn't be a serious limitation for CCP and VRP which
> care about constants only - but it shows a defect in the
> gimple_simplify interface.  I haven't yet concluded on a better
> one though - options go from adding a secondary return to
> the valueize hook to adding a second hook maybe with additionally
> adding a simple flag to turn off SSA edge following globally.
>
> Anyway - the following patch should fix the immediate regression
> and allows to go forward with removing GENERIC folding from
> both fold_stmt and gimple_fold_stmt_to_constant.  Just not
> for this stage1 which will end too soon.
>
> Bootstrapped on x86_64-unknown-linux-gnu, testing in progress.
>
> Thanks,
> Richard.
>
> 2014-11-14  Richard Biener  <rguenther@suse.de>
>
>         * gimple-fold.h (gimple_fold_stmt_to_constant_1): Add 2nd
>         valueization hook defaulted to no_follow_ssa_edges.
>         * gimple-fold.c (gimple_fold_stmt_to_constant_1): Pass
>         2nd valueization hook to gimple_simplify.
>         * tree-ssa-ccp.c (valueize_op_1): New function to be
>         used for gimple_simplify called via gimple_fold_stmt_to_constant_1.
>         (ccp_fold): Adjust.
>         * tree-vrp.c (vrp_valueize_1): New function to be
>         used for gimple_simplify called via gimple_fold_stmt_to_constant_1.
>         (vrp_visit_assignment_or_call): Adjust.
>

This caused:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63898
diff mbox

Patch

Index: gcc/gimple-fold.c
===================================================================
--- gcc/gimple-fold.c	(svn+ssh://rguenth@gcc.gnu.org/svn/gcc/trunk/gcc/gimple-fold.c)	(revision 217545)
+++ gcc/gimple-fold.c	(.../gcc/gimple-fold.c)	(working copy)
@@ -4467,7 +4411,8 @@  maybe_fold_or_comparisons (enum tree_cod
    to avoid the indirect function call overhead.  */
 
 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] = {};
@@ -4475,7 +4420,7 @@  gimple_fold_stmt_to_constant_1 (gimple s
      edges if there are intermediate VARYING defs.  For this reason
      do not follow SSA edges here even though SCCVN can technically
      just deal fine with that.  */
-  if (gimple_simplify (stmt, &rcode, ops, NULL, no_follow_ssa_edges)
+  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/gimple-fold.h
===================================================================
--- gcc/gimple-fold.h	(svn+ssh://rguenth@gcc.gnu.org/svn/gcc/trunk/gcc/gimple-fold.h)	(revision 217545)
+++ gcc/gimple-fold.h	(.../gcc/gimple-fold.h)	(working copy)
@@ -36,7 +36,8 @@  extern bool arith_overflowed_p (enum tre
 				const_tree);
 extern tree no_follow_ssa_edges (tree);
 extern tree follow_single_use_edges (tree);
-extern tree gimple_fold_stmt_to_constant_1 (gimple, tree (*) (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/tree-ssa-ccp.c
===================================================================
--- gcc/tree-ssa-ccp.c	(svn+ssh://rguenth@gcc.gnu.org/svn/gcc/trunk/gcc/tree-ssa-ccp.c)	(revision 217545)
+++ gcc/tree-ssa-ccp.c	(.../gcc/tree-ssa-ccp.c)	(working copy)
@@ -1126,6 +1126,27 @@  valueize_op (tree op)
   return op;
 }
 
+/* Return the constant value for OP, but signal to not follow SSA
+   edges if the definition may be simulated again.  */
+
+static tree
+valueize_op_1 (tree op)
+{
+  if (TREE_CODE (op) == SSA_NAME)
+    {
+      tree tem = get_constant_value (op);
+      if (tem)
+	return tem;
+      /* If the definition may be simulated again we cannot follow
+         this SSA edge as the SSA propagator does not necessarily
+	 re-visit the use.  */
+      gimple def_stmt = SSA_NAME_DEF_STMT (op);
+      if (prop_simulate_again_p (def_stmt))
+	return NULL_TREE;
+    }
+  return op;
+}
+
 /* CCP specific front-end to the non-destructive constant folding
    routines.
 
@@ -1158,7 +1179,8 @@  ccp_fold (gimple stmt)
 
     case GIMPLE_ASSIGN:
     case GIMPLE_CALL:
-      return gimple_fold_stmt_to_constant_1 (stmt, valueize_op);
+      return gimple_fold_stmt_to_constant_1 (stmt,
+					     valueize_op, valueize_op_1);
 
     default:
       gcc_unreachable ();
Index: gcc/tree-vrp.c
===================================================================
--- gcc/tree-vrp.c	(svn+ssh://rguenth@gcc.gnu.org/svn/gcc/trunk/gcc/tree-vrp.c)	(revision 217545)
+++ gcc/tree-vrp.c	(.../gcc/tree-vrp.c)	(working copy)
@@ -7025,6 +7025,27 @@  vrp_valueize (tree name)
   return name;
 }
 
+/* Return the singleton value-range for NAME if that is a constant
+   but signal to not follow SSA edges.  */
+
+static inline tree
+vrp_valueize_1 (tree name)
+{
+  if (TREE_CODE (name) == SSA_NAME)
+    {
+      value_range_t *vr = get_value_range (name);
+      if (range_int_cst_singleton_p (vr))
+	return vr->min;
+      /* If the definition may be simulated again we cannot follow
+         this SSA edge as the SSA propagator does not necessarily
+	 re-visit the use.  */
+      gimple def_stmt = SSA_NAME_DEF_STMT (name);
+      if (prop_simulate_again_p (def_stmt))
+	return NULL_TREE;
+    }
+  return name;
+}
+
 /* Visit assignment STMT.  If it produces an interesting range, record
    the SSA name in *OUTPUT_P.  */
 
@@ -7048,8 +7069,9 @@  vrp_visit_assignment_or_call (gimple stm
       value_range_t new_vr = VR_INITIALIZER;
 
       /* Try folding the statement to a constant first.  */
-      tree tem = gimple_fold_stmt_to_constant (stmt, vrp_valueize);
-      if (tem)
+      tree tem = gimple_fold_stmt_to_constant_1 (stmt, vrp_valueize,
+						 vrp_valueize_1);
+      if (tem && is_gimple_min_invariant (tem))
 	set_value_range_to_value (&new_vr, tem, NULL);
       /* Then dispatch to value-range extracting functions.  */
       else if (code == GIMPLE_CALL)