diff mbox series

Mostly fix PR90726 - exponential compile-time/memory behavior

Message ID alpine.LSU.2.20.1906031537310.10704@zhemvz.fhfr.qr
State New
Headers show
Series Mostly fix PR90726 - exponential compile-time/memory behavior | expand

Commit Message

Richard Biener June 3, 2019, 1:44 p.m. UTC
This mostly fixes PR90726, employing proper visited mechanisms to
avoid exponential behavior when walking a GENERIC expression
with shared trees.  It also fixes the code-generation side where
gimplification of such tree naturally explodes as well - but
not by more optimal gimplifying but accounting for out
stupidness in the expression_expensive_p costing.  Ideally
we'd fix the gimplifier to deal with tree sharing (need to
remember gimplified operands and re-use the gimplified result
or preprocess the GENERIC inserting save-exprs, or ...).
But gratious use of unshare_expr everywhere defeats this
(unsharing also makes a tree of graphs rather than just
deep-copying a tree graph).

There's one piece left which is why the testcase uses -fno-ivopts.
expand_simple_operations runs into the same exponential behavior
walking the GENERIC expression _and_ the SSA graph reached from it.
It also will end up turning all modified sub-graphs into trees.
Changing that behemont warrants a separate patch ;)

Cutting the thing off at the SCEV analysis boundary would be
another option (there's already expression size limits but
those do not really work - sth on my list).  Ideally the
GENERIC graph is just a complementary representation of
the SSA graph, that we handle it by exploding is the thing
to fix (IMHO), even if that's somewhat more painful than
giving up during SCEV.

Bootstrap / regtest running on x86_64-unknown-linux-gnu.

Richard.

2019-06-03  Richard Biener  <rguenther@suse.de>

	PR middle-end/90726
	* tree-chrec.c (chrec_contains_symbols): Add to visited.
	(tree_contains_chrecs): Likewise.
	(chrec_contains_symbols_defined_in_loop): Move here and avoid
	exponential behaivor from ...
	* tree-scalar-evolution.c (chrec_contains_symbols_defined_in_loop):
	... here.
	(expression_expensive_p): Avoid exponential behavior and compute
	expanded size, rejecting any expansion.
	* tree-ssa-loop-ivopts.c (abnormal_ssa_name_p): Remove.
	(idx_contains_abnormal_ssa_name_p): Likewise.
	(contains_abnormal_ssa_name_p_1): New helper for walk_tree.
	(contains_abnormal_ssa_name_p): Simplify and use
	walk_tree_without_duplicates.

	* gcc.dg/pr90726.c: New testcase.

Comments

Richard Biener June 4, 2019, 10:29 a.m. UTC | #1
On Mon, 3 Jun 2019, Richard Biener wrote:

> 
> This mostly fixes PR90726, employing proper visited mechanisms to
> avoid exponential behavior when walking a GENERIC expression
> with shared trees.  It also fixes the code-generation side where
> gimplification of such tree naturally explodes as well - but
> not by more optimal gimplifying but accounting for out
> stupidness in the expression_expensive_p costing.  Ideally
> we'd fix the gimplifier to deal with tree sharing (need to
> remember gimplified operands and re-use the gimplified result
> or preprocess the GENERIC inserting save-exprs, or ...).
> But gratious use of unshare_expr everywhere defeats this
> (unsharing also makes a tree of graphs rather than just
> deep-copying a tree graph).
> 
> There's one piece left which is why the testcase uses -fno-ivopts.
> expand_simple_operations runs into the same exponential behavior
> walking the GENERIC expression _and_ the SSA graph reached from it.
> It also will end up turning all modified sub-graphs into trees.
> Changing that behemont warrants a separate patch ;)
> 
> Cutting the thing off at the SCEV analysis boundary would be
> another option (there's already expression size limits but
> those do not really work - sth on my list).  Ideally the
> GENERIC graph is just a complementary representation of
> the SSA graph, that we handle it by exploding is the thing
> to fix (IMHO), even if that's somewhat more painful than
> giving up during SCEV.

This one fixes the issue in expand_simple_operations.  Up
to the point where it starts walking the SSA graph but I
haven't got a testcase for that yet.

Bootstrap / testing running on x86_64-unknown-linux-gnu.

I wonder if it would make sense to have auto-storage
hash_{map,set} for the usual cases with small number of
entries?

Richard.

2019-06-04  Richard Biener  <rguenther@suse.de>

	PR middle-end/90726
	* tree-ssa-loop-niter.c (expand_simple_operations): Do not
	turn an expression graph into a tree.

	* gcc.dg/pr90726.c: Enable IVOPTs.

Index: gcc/tree-ssa-loop-niter.c
===================================================================
--- gcc/tree-ssa-loop-niter.c	(revision 271904)
+++ gcc/tree-ssa-loop-niter.c	(working copy)
@@ -1977,8 +1977,8 @@ simplify_replace_tree (tree expr, tree o
    enough, and return the new expression.  If STOP is specified, stop
    expanding if EXPR equals to it.  */
 
-tree
-expand_simple_operations (tree expr, tree stop)
+static tree
+expand_simple_operations (tree expr, tree stop, hash_map<tree, tree> &cache)
 {
   unsigned i, n;
   tree ret = NULL_TREE, e, ee, e1;
@@ -1998,7 +1998,24 @@ expand_simple_operations (tree expr, tre
       for (i = 0; i < n; i++)
 	{
 	  e = TREE_OPERAND (expr, i);
-	  ee = expand_simple_operations (e, stop);
+	  /* SCEV analysis feeds us with a proper expression
+	     graph matching the SSA graph.  Avoid turning it
+	     into a tree here, thus handle tree sharing
+	     properly.
+	     ???  The SSA walk below still turns the SSA graph
+	     into a tree but until we find a testcase do not
+	     introduce additional tree sharing here.  */
+	  bool existed_p;
+	  tree &cee = cache.get_or_insert (e, &existed_p);
+	  if (existed_p)
+	    ee = cee;
+	  else
+	    {
+	      cee = e;
+	      ee = expand_simple_operations (e, stop, cache);
+	      if (ee != e)
+		*cache.get (e) = ee;
+	    }
 	  if (e == ee)
 	    continue;
 
@@ -2038,7 +2055,7 @@ expand_simple_operations (tree expr, tre
 	  && src->loop_father != dest->loop_father)
 	return expr;
 
-      return expand_simple_operations (e, stop);
+      return expand_simple_operations (e, stop, cache);
     }
   if (gimple_code (stmt) != GIMPLE_ASSIGN)
     return expr;
@@ -2058,7 +2075,7 @@ expand_simple_operations (tree expr, tre
 	return e;
 
       if (code == SSA_NAME)
-	return expand_simple_operations (e, stop);
+	return expand_simple_operations (e, stop, cache);
       else if (code == ADDR_EXPR)
 	{
 	  poly_int64 offset;
@@ -2067,7 +2084,8 @@ expand_simple_operations (tree expr, tre
 	  if (base
 	      && TREE_CODE (base) == MEM_REF)
 	    {
-	      ee = expand_simple_operations (TREE_OPERAND (base, 0), stop);
+	      ee = expand_simple_operations (TREE_OPERAND (base, 0), stop,
+					     cache);
 	      return fold_build2 (POINTER_PLUS_EXPR, TREE_TYPE (expr), ee,
 				  wide_int_to_tree (sizetype,
 						    mem_ref_offset (base)
@@ -2082,7 +2100,7 @@ expand_simple_operations (tree expr, tre
     {
     CASE_CONVERT:
       /* Casts are simple.  */
-      ee = expand_simple_operations (e, stop);
+      ee = expand_simple_operations (e, stop, cache);
       return fold_build1 (code, TREE_TYPE (expr), ee);
 
     case PLUS_EXPR:
@@ -2097,7 +2115,7 @@ expand_simple_operations (tree expr, tre
       if (!is_gimple_min_invariant (e1))
 	return expr;
 
-      ee = expand_simple_operations (e, stop);
+      ee = expand_simple_operations (e, stop, cache);
       return fold_build2 (code, TREE_TYPE (expr), ee, e1);
 
     default:
@@ -2105,6 +2123,13 @@ expand_simple_operations (tree expr, tre
     }
 }
 
+tree
+expand_simple_operations (tree expr, tree stop)
+{
+  hash_map<tree, tree> cache;
+  return expand_simple_operations (expr, stop, cache);
+}
+
 /* Tries to simplify EXPR using the condition COND.  Returns the simplified
    expression (or EXPR unchanged, if no simplification was possible).  */
 
Index: gcc/testsuite/gcc.dg/pr90726.c
===================================================================
--- gcc/testsuite/gcc.dg/pr90726.c	(revision 271904)
+++ gcc/testsuite/gcc.dg/pr90726.c	(working copy)
@@ -1,5 +1,5 @@
 /* { dg-do compile } */
-/* { dg-options "-fgimple -O2 -fno-ivopts" } */
+/* { dg-options "-fgimple -O2" } */
 
 int __GIMPLE (ssa,guessed_local(12348030),startwith("fix_loops"))
 un (int dd)
diff mbox series

Patch

Index: gcc/tree-chrec.c
===================================================================
--- gcc/tree-chrec.c	(revision 271867)
+++ gcc/tree-chrec.c	(working copy)
@@ -35,6 +35,8 @@  along with GCC; see the file COPYING3.
 #include "tree-ssa-loop-ivopts.h"
 #include "tree-ssa-loop-niter.h"
 #include "tree-chrec.h"
+#include "gimple.h"
+#include "tree-ssa-loop.h"
 #include "dumpfile.h"
 #include "params.h"
 #include "tree-scalar-evolution.h"
@@ -959,6 +961,9 @@  chrec_contains_symbols (const_tree chrec
       && flow_loop_nested_p (get_chrec_loop (chrec), loop))
     return true;
 
+  if (visited.add (chrec))
+    return false;
+
   n = TREE_OPERAND_LENGTH (chrec);
   for (i = 0; i < n; i++)
     if (chrec_contains_symbols (TREE_OPERAND (chrec, i), visited, loop))
@@ -978,6 +983,63 @@  chrec_contains_symbols (const_tree chrec
   return chrec_contains_symbols (chrec, visited, loop);
 }
 
+/* Return true when CHREC contains symbolic names defined in
+   LOOP_NB.  */
+
+static bool
+chrec_contains_symbols_defined_in_loop (const_tree chrec, unsigned loop_nb,
+					hash_set<const_tree> &visited)
+{
+  int i, n;
+
+  if (chrec == NULL_TREE)
+    return false;
+
+  if (is_gimple_min_invariant (chrec))
+    return false;
+
+  if (TREE_CODE (chrec) == SSA_NAME)
+    {
+      gimple *def;
+      loop_p def_loop, loop;
+
+      if (SSA_NAME_IS_DEFAULT_DEF (chrec))
+	return false;
+
+      def = SSA_NAME_DEF_STMT (chrec);
+      def_loop = loop_containing_stmt (def);
+      loop = get_loop (cfun, loop_nb);
+
+      if (def_loop == NULL)
+	return false;
+
+      if (loop == def_loop || flow_loop_nested_p (loop, def_loop))
+	return true;
+
+      return false;
+    }
+
+  if (visited.add (chrec))
+    return false;
+
+  n = TREE_OPERAND_LENGTH (chrec);
+  for (i = 0; i < n; i++)
+    if (chrec_contains_symbols_defined_in_loop (TREE_OPERAND (chrec, i),
+						loop_nb, visited))
+      return true;
+  return false;
+}
+
+/* Return true when CHREC contains symbolic names defined in
+   LOOP_NB.  */
+
+bool
+chrec_contains_symbols_defined_in_loop (const_tree chrec, unsigned loop_nb)
+{
+  hash_set<const_tree> visited;
+  return chrec_contains_symbols_defined_in_loop (chrec, loop_nb, visited);
+}
+
 /* Determines whether the chrec contains undetermined coefficients.  */
 
 static bool
@@ -1026,6 +1088,9 @@  tree_contains_chrecs (const_tree expr, i
   if (tree_is_chrec (expr))
     return true;
 
+  if (visited.add (expr))
+    return false;
+
   n = TREE_OPERAND_LENGTH (expr);
   for (i = 0; i < n; i++)
     if (tree_contains_chrecs (TREE_OPERAND (expr, i), size, visited))
Index: gcc/tree-scalar-evolution.c
===================================================================
--- gcc/tree-scalar-evolution.c	(revision 271867)
+++ gcc/tree-scalar-evolution.c	(working copy)
@@ -411,49 +411,6 @@  instantiate_cache_type::~instantiate_cac
 static instantiate_cache_type *global_cache;
 
 
-/* Return true when CHREC contains symbolic names defined in
-   LOOP_NB.  */
-
-bool
-chrec_contains_symbols_defined_in_loop (const_tree chrec, unsigned loop_nb)
-{
-  int i, n;
-
-  if (chrec == NULL_TREE)
-    return false;
-
-  if (is_gimple_min_invariant (chrec))
-    return false;
-
-  if (TREE_CODE (chrec) == SSA_NAME)
-    {
-      gimple *def;
-      loop_p def_loop, loop;
-
-      if (SSA_NAME_IS_DEFAULT_DEF (chrec))
-	return false;
-
-      def = SSA_NAME_DEF_STMT (chrec);
-      def_loop = loop_containing_stmt (def);
-      loop = get_loop (cfun, loop_nb);
-
-      if (def_loop == NULL)
-	return false;
-
-      if (loop == def_loop || flow_loop_nested_p (loop, def_loop))
-	return true;
-
-      return false;
-    }
-
-  n = TREE_OPERAND_LENGTH (chrec);
-  for (i = 0; i < n; i++)
-    if (chrec_contains_symbols_defined_in_loop (TREE_OPERAND (chrec, i),
-						loop_nb))
-      return true;
-  return false;
-}
-
 /* Return true when PHI is a loop-phi-node.  */
 
 static bool
@@ -3505,8 +3462,9 @@  scev_finalize (void)
 /* Returns true if the expression EXPR is considered to be too expensive
    for scev_const_prop.  */
 
-bool
-expression_expensive_p (tree expr)
+static bool
+expression_expensive_p (tree expr, hash_map<tree, uint64_t> &cache,
+			uint64_t &cost)
 {
   enum tree_code code;
 
@@ -3530,6 +3488,19 @@  expression_expensive_p (tree expr)
 	return true;
     }
 
+  bool visited_p;
+  uint64_t &local_cost = cache.get_or_insert (expr, &visited_p);
+  if (visited_p)
+    {
+      uint64_t tem = cost + local_cost;
+      if (tem < cost)
+	return true;
+      cost = tem;
+      return false;
+    }
+  local_cost = 1;
+
+  uint64_t op_cost = 0;
   if (code == CALL_EXPR)
     {
       tree arg;
@@ -3568,39 +3539,62 @@  expression_expensive_p (tree expr)
       if (!is_inexpensive_builtin (get_callee_fndecl (expr)))
 	return true;
       FOR_EACH_CALL_EXPR_ARG (arg, iter, expr)
-	if (expression_expensive_p (arg))
+	if (expression_expensive_p (arg, cache, op_cost))
 	  return true;
+      *cache.get (expr) += op_cost;
+      cost += op_cost + 1;
       return false;
     }
 
   if (code == COND_EXPR)
-    return (expression_expensive_p (TREE_OPERAND (expr, 0))
-	    || (EXPR_P (TREE_OPERAND (expr, 1))
-		&& EXPR_P (TREE_OPERAND (expr, 2)))
-	    /* If either branch has side effects or could trap.  */
-	    || TREE_SIDE_EFFECTS (TREE_OPERAND (expr, 1))
-	    || generic_expr_could_trap_p (TREE_OPERAND (expr, 1))
-	    || TREE_SIDE_EFFECTS (TREE_OPERAND (expr, 0))
-	    || generic_expr_could_trap_p (TREE_OPERAND (expr, 0))
-	    || expression_expensive_p (TREE_OPERAND (expr, 1))
-	    || expression_expensive_p (TREE_OPERAND (expr, 2)));
+    {
+      if (expression_expensive_p (TREE_OPERAND (expr, 0), cache, op_cost)
+	  || (EXPR_P (TREE_OPERAND (expr, 1))
+	      && EXPR_P (TREE_OPERAND (expr, 2)))
+	  /* If either branch has side effects or could trap.  */
+	  || TREE_SIDE_EFFECTS (TREE_OPERAND (expr, 1))
+	  || generic_expr_could_trap_p (TREE_OPERAND (expr, 1))
+	  || TREE_SIDE_EFFECTS (TREE_OPERAND (expr, 0))
+	  || generic_expr_could_trap_p (TREE_OPERAND (expr, 0))
+	  || expression_expensive_p (TREE_OPERAND (expr, 1),
+				     cache, op_cost)
+	  || expression_expensive_p (TREE_OPERAND (expr, 2),
+				     cache, op_cost))
+	return true;
+      *cache.get (expr) += op_cost;
+      cost += op_cost + 1;
+      return false;
+    }
 
   switch (TREE_CODE_CLASS (code))
     {
     case tcc_binary:
     case tcc_comparison:
-      if (expression_expensive_p (TREE_OPERAND (expr, 1)))
+      if (expression_expensive_p (TREE_OPERAND (expr, 1), cache, op_cost))
 	return true;
 
       /* Fallthru.  */
     case tcc_unary:
-      return expression_expensive_p (TREE_OPERAND (expr, 0));
+      if (expression_expensive_p (TREE_OPERAND (expr, 0), cache, op_cost))
+	return true;
+      *cache.get (expr) += op_cost;
+      cost += op_cost + 1;
+      return false;
 
     default:
       return true;
     }
 }
 
+bool
+expression_expensive_p (tree expr)
+{
+  hash_map<tree, uint64_t> cache;
+  uint64_t expanded_size = 0;
+  return (expression_expensive_p (expr, cache, expanded_size)
+	  || expanded_size > cache.elements ());
+}
+
 /* Do final value replacement for LOOP, return true if we did anything.  */
 
 bool
Index: gcc/tree-ssa-loop-ivopts.c
===================================================================
--- gcc/tree-ssa-loop-ivopts.c	(revision 271867)
+++ gcc/tree-ssa-loop-ivopts.c	(working copy)
@@ -944,36 +944,19 @@  stmt_after_increment (struct loop *loop,
     }
 }
 
-/* Returns true if EXP is a ssa name that occurs in an abnormal phi node.  */
+/* walk_tree callback for contains_abnormal_ssa_name_p.  */
 
-static bool
-abnormal_ssa_name_p (tree exp)
+static tree
+contains_abnormal_ssa_name_p_1 (tree *tp, int *walk_subtrees, void *)
 {
-  if (!exp)
-    return false;
+  if (TREE_CODE (*tp) == SSA_NAME
+      && SSA_NAME_OCCURS_IN_ABNORMAL_PHI (*tp))
+    return *tp;
 
-  if (TREE_CODE (exp) != SSA_NAME)
-    return false;
+  if (!EXPR_P (*tp))
+    *walk_subtrees = 0;
 
-  return SSA_NAME_OCCURS_IN_ABNORMAL_PHI (exp) != 0;
-}
-
-/* Returns false if BASE or INDEX contains a ssa name that occurs in an
-   abnormal phi node.  Callback for for_each_index.  */
-
-static bool
-idx_contains_abnormal_ssa_name_p (tree base, tree *index,
-				  void *data ATTRIBUTE_UNUSED)
-{
-  if (TREE_CODE (base) == ARRAY_REF || TREE_CODE (base) == ARRAY_RANGE_REF)
-    {
-      if (abnormal_ssa_name_p (TREE_OPERAND (base, 2)))
-	return false;
-      if (abnormal_ssa_name_p (TREE_OPERAND (base, 3)))
-	return false;
-    }
-
-  return !abnormal_ssa_name_p (*index);
+  return NULL_TREE;
 }
 
 /* Returns true if EXPR contains a ssa name that occurs in an
@@ -982,61 +965,8 @@  idx_contains_abnormal_ssa_name_p (tree b
 bool
 contains_abnormal_ssa_name_p (tree expr)
 {
-  enum tree_code code;
-  enum tree_code_class codeclass;
-
-  if (!expr)
-    return false;
-
-  code = TREE_CODE (expr);
-  codeclass = TREE_CODE_CLASS (code);
-
-  if (code == CALL_EXPR)
-    {
-      tree arg;
-      call_expr_arg_iterator iter;
-      FOR_EACH_CALL_EXPR_ARG (arg, iter, expr)
-	if (contains_abnormal_ssa_name_p (arg))
-	  return true;
-      return false;
-    }
-
-  if (code == SSA_NAME)
-    return SSA_NAME_OCCURS_IN_ABNORMAL_PHI (expr) != 0;
-
-  if (code == INTEGER_CST
-      || is_gimple_min_invariant (expr))
-    return false;
-
-  if (code == ADDR_EXPR)
-    return !for_each_index (&TREE_OPERAND (expr, 0),
-			    idx_contains_abnormal_ssa_name_p,
-			    NULL);
-
-  if (code == COND_EXPR)
-    return contains_abnormal_ssa_name_p (TREE_OPERAND (expr, 0))
-      || contains_abnormal_ssa_name_p (TREE_OPERAND (expr, 1))
-      || contains_abnormal_ssa_name_p (TREE_OPERAND (expr, 2));
-
-  switch (codeclass)
-    {
-    case tcc_binary:
-    case tcc_comparison:
-      if (contains_abnormal_ssa_name_p (TREE_OPERAND (expr, 1)))
-	return true;
-
-      /* Fallthru.  */
-    case tcc_unary:
-      if (contains_abnormal_ssa_name_p (TREE_OPERAND (expr, 0)))
-	return true;
-
-      break;
-
-    default:
-      gcc_unreachable ();
-    }
-
-  return false;
+  return walk_tree_without_duplicates
+	   (&expr, contains_abnormal_ssa_name_p_1, NULL) != NULL_TREE;
 }
 
 /*  Returns the structure describing number of iterations determined from
Index: gcc/testsuite/gcc.dg/pr90726.c
===================================================================
--- gcc/testsuite/gcc.dg/pr90726.c	(nonexistent)
+++ gcc/testsuite/gcc.dg/pr90726.c	(working copy)
@@ -0,0 +1,56 @@ 
+/* { dg-do compile } */
+/* { dg-options "-fgimple -O2 -fno-ivopts" } */
+
+int __GIMPLE (ssa,guessed_local(12348030),startwith("fix_loops"))
+un (int dd)
+{
+  int s2;
+  int q8;
+  int nz;
+
+  __BB(2,guessed_local(12348030)):
+  goto __BB3(guessed(134217728));
+
+  __BB(3,loop_header(1),guessed_local(37044096)):
+  nz_7 = __PHI (__BB2: 0, __BB5: nz_10);
+  q8_13 = dd_9(D) * dd_9(D);
+  q8_17 = q8_13 * q8_13;
+  q8_21 = q8_17 * q8_17;
+  q8_25 = q8_21 * q8_21;
+  q8_29 = q8_25 * q8_25;
+  q8_33 = q8_29 * q8_29;
+  q8_37 = q8_33 * q8_33;
+  q8_41 = q8_37 * q8_37;
+  q8_45 = q8_41 * q8_41;
+  q8_49 = q8_45 * q8_45;
+  q8_53 = q8_49 * q8_49;
+  q8_57 = q8_53 * q8_53;
+  q8_61 = q8_57 * q8_57;
+  q8_65 = q8_61 * q8_61;
+  q8_69 = q8_65 * q8_65;
+  q8_73 = q8_69 * q8_69;
+  q8_77 = q8_73 * q8_73;
+  q8_81 = q8_77 * q8_77;
+  q8_85 = q8_81 * q8_81;
+  q8_89 = q8_85 * q8_85;
+  q8_93 = q8_89 * q8_89;
+  q8_97 = q8_93 * q8_93;
+  q8_101 = q8_97 * q8_97;
+  q8_105 = q8_101 * q8_101;
+  q8_109 = q8_105 * q8_105;
+  q8_113 = q8_109 * q8_109;
+  q8_117 = q8_113 * q8_113;
+  q8_121 = q8_117 * q8_117;
+  nz_10 = nz_7 + 1;
+  if (nz_10 != 3)
+    goto __BB5(guessed(89478485));
+  else
+    goto __BB4(guessed(44739243));
+
+  __BB(5,guessed_local(24696064)):
+  goto __BB3(precise(134217728));
+
+  __BB(4,guessed_local(12348031)):
+  return q8_121;
+
+}