[1/2] ipa-sra: Introduce a mini-DCE to tree-inline.c (PR 93385)

Message ID ri635x0lf63.fsf@suse.cz
State New
Headers show
Series
  • [1/2] ipa-sra: Introduce a mini-DCE to tree-inline.c (PR 93385)
Related show

Commit Message

Martin Jambor March 12, 2021, 12:39 p.m.
Hi,

PR 93385 reveals that if the user explicitely disables DCE, IPA-SRA
can leave behind statements which are useless because their results
are eventually not used but can have problematic side effects,
especially since their inputs are now bogus that useless parameters
were removed.

This patch fixes the problem by doing a def-use walk when
materializing clones, marking which statements should not be copied
and which SSA_NAMEs do not need to be computed because eventually they
would be DCEd.  Then tree-inline.c code simply does not copy them.

See the follow-up patch for what this means for debug statements.

There is one exception to the above non-copying rule: If such an
SSA_NAME appears as an argument of a call, it needs to be removed by
call redirection and not as part of clone materialization.  So to have
something valid there until that time, this patch uses the VAR_DECL that
is already created to serve as a base of non-default-def SSA_NAMEs of
the removed parameter.  This is technically incorrect because the
argument may be only causally related to the removed parameter, but it
would be only there until call redirection and this avoids creating
another VAR_DECL, which was the major objection to this patch in summer.

I think I have managed to design a way how to avoid it and generally
make the call statements a bit less clunky in between clone
materialization and call redirection. It involves storing essential
information in call summaries (also moving information from
clone_info::performed_splits there) but the patch is big even though I
have not yet finished writing it and it is not stage 4 material.

So, if we want to fix the issue in GCC 11 (and 10?), I would propose
this one - almost all of it is needed for the sophisticated fix too.  It
has passed bootstrap, LTO bootstrap and profiled LTO bootstrap and
testing on x86_64-linux and normal bootstrap and testing on
aarch64-linux.  Alternatively, we can just mitigate all known
manifestations of the issue with
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93385#c11 and only include
the sophisticated fix using call summaries in GCC 12.  (Or we can of
course decide to do nothing for 11.1 and wait for the final patch in GCC
12.)

Thanks,

Martin



gcc/ChangeLog:

2021-03-10  Martin Jambor  <mjambor@suse.cz>

	PR ipa/93385
	* ipa-param-manipulation.h (class ipa_param_body_adjustments): New
	members m_dead_stmts, m_dead_ssas, mark_dead_statements and
	modify_call_argument.
	* ipa-param-manipulation.c (phi_arg_will_live_p): New function.
	(ipa_param_body_adjustments::mark_dead_statements): New method.
	(ipa_param_body_adjustments::common_initialization): Call it.
	(ipa_param_body_adjustments::ipa_param_body_adjustments): Initialize
	new mwmbers.
	(ipa_param_body_adjustments::modify_call_argument): New.
	(ipa_param_body_adjustments::modify_call_stmt): Use
	modify_call_argument.
	* tree-inline.c (remap_gimple_stmt): Do not copy dead statements,
	reset dead debug statements.
	(copy_phis_for_bb): Do not copy dead PHI nodes.

gcc/testsuite/ChangeLog:

2020-05-14  Martin Jambor  <mjambor@suse.cz>

	PR ipa/93385
	* gcc.dg/ipa/pr93385.c: New test.
	* gcc.dg/ipa/ipa-sra-23.c: Likewise.
---
 gcc/ipa-param-manipulation.c          | 144 ++++++++++++++++++++++++--
 gcc/ipa-param-manipulation.h          |   9 ++
 gcc/testsuite/gcc.dg/ipa/ipa-sra-23.c |  24 +++++
 gcc/testsuite/gcc.dg/ipa/pr93385.c    |  27 +++++
 gcc/tree-inline.c                     |  18 +++-
 5 files changed, 208 insertions(+), 14 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/ipa/ipa-sra-23.c
 create mode 100644 gcc/testsuite/gcc.dg/ipa/pr93385.c

-- 
2.30.1

Patch

diff --git a/gcc/ipa-param-manipulation.c b/gcc/ipa-param-manipulation.c
index 132bb24f76a..f6b569a8a8b 100644
--- a/gcc/ipa-param-manipulation.c
+++ b/gcc/ipa-param-manipulation.c
@@ -970,6 +970,97 @@  ipa_param_body_adjustments::carry_over_param (tree t)
   return new_parm;
 }
 
+/* Return true if BLOCKS_TO_COPY is NULL or if PHI has an argument ARG in
+   position that corresponds to an edge that is coming from a block that has
+   the corresponding bit set in BLOCKS_TO_COPY.  */
+
+static bool
+phi_arg_will_live_p (gphi *phi, bitmap blocks_to_copy, tree arg)
+{
+  bool arg_will_survive = false;
+  if (!blocks_to_copy)
+    arg_will_survive = true;
+  else
+    for (unsigned i = 0; i < gimple_phi_num_args (phi); i++)
+      if (gimple_phi_arg_def (phi, i) == arg
+	  && bitmap_bit_p (blocks_to_copy,
+			   gimple_phi_arg_edge (phi, i)->src->index))
+	{
+	  arg_will_survive = true;
+	  break;
+	}
+  return arg_will_survive;
+}
+
+/* Populate m_dead_stmts given that DEAD_PARAM is going to be removed without
+   any replacement or splitting.  REPL is the replacement VAR_SECL to base any
+   remaining uses of a removed parameter on.  */
+
+void
+ipa_param_body_adjustments::mark_dead_statements (tree dead_param, tree repl)
+{
+  /* Current IPA analyses which remove unused parameters never remove a
+     non-gimple register ones which have any use except as parameters in other
+     calls, so we can safely leve them as they are.  */
+  if (!is_gimple_reg (dead_param))
+    return;
+  tree parm_ddef = ssa_default_def (m_id->src_cfun, dead_param);
+  if (!parm_ddef || has_zero_uses (parm_ddef))
+    return;
+
+  auto_vec<tree, 4> stack;
+  m_dead_ssas.put (parm_ddef, repl);
+  stack.safe_push (parm_ddef);
+  while (!stack.is_empty ())
+    {
+      tree t = stack.pop ();
+
+      imm_use_iterator imm_iter;
+      gimple *stmt;
+
+      insert_decl_map (m_id, t, error_mark_node);
+      FOR_EACH_IMM_USE_STMT (stmt, imm_iter, t)
+	{
+	  if (is_gimple_call (stmt)
+	      || (m_id->blocks_to_copy
+		  && !bitmap_bit_p (m_id->blocks_to_copy,
+				    gimple_bb (stmt)->index)))
+	    continue;
+
+	  if (is_gimple_debug (stmt))
+	    {
+	      m_dead_stmts.add (stmt);
+	      gcc_assert (gimple_debug_bind_p (stmt));
+	    }
+	  else if (gimple_code (stmt) == GIMPLE_PHI)
+	    {
+	      gphi *phi = as_a <gphi *> (stmt);
+	      if (phi_arg_will_live_p (phi, m_id->blocks_to_copy, t))
+		{
+		  m_dead_stmts.add (phi);
+		  tree res = gimple_phi_result (phi);
+		  if (!m_dead_ssas.put (res, repl))
+		    stack.safe_push (res);
+		}
+	    }
+	  else if (is_gimple_assign (stmt))
+	    {
+	      m_dead_stmts.add (stmt);
+	      if (!gimple_clobber_p (stmt))
+		{
+		  tree lhs = gimple_assign_lhs (stmt);
+		  gcc_assert (TREE_CODE (lhs) == SSA_NAME);
+		  if (!m_dead_ssas.put (lhs, repl))
+		    stack.safe_push (lhs);
+		}
+	    }
+	  else
+	    /* IPA-SRA does not analyze other types of statements.  */
+	    gcc_unreachable ();
+	}
+    }
+}
+
 /* Common initialization performed by all ipa_param_body_adjustments
    constructors.  OLD_FNDECL is the declaration we take original arguments
    from, (it may be the same as M_FNDECL).  VARS, if non-NULL, is a pointer to
@@ -1113,6 +1204,11 @@  ipa_param_body_adjustments::common_initialization (tree old_fndecl,
 		/* Declare this new variable.  */
 		DECL_CHAIN (var) = *vars;
 		*vars = var;
+
+		/* If this is not a split but a real removal, init hash sets
+		   that will guide what not to copy to the new body.  */
+		if (!isra_dummy_decls[i])
+		  mark_dead_statements (m_oparms[i], var);
 	      }
 	  }
 	else
@@ -1169,9 +1265,10 @@  ipa_param_body_adjustments
 ::ipa_param_body_adjustments (vec<ipa_adjusted_param, va_gc> *adj_params,
 			      tree fndecl)
   : m_adj_params (adj_params), m_adjustments (NULL), m_reset_debug_decls (),
-    m_split_modifications_p (false), m_fndecl (fndecl), m_id (NULL),
-    m_oparms (), m_new_decls (), m_new_types (), m_replacements (),
-    m_removed_decls (), m_removed_map (), m_method2func (false)
+    m_split_modifications_p (false), m_dead_stmts (), m_dead_ssas (),
+    m_fndecl (fndecl), m_id (NULL), m_oparms (), m_new_decls (),
+    m_new_types (), m_replacements (), m_removed_decls (), m_removed_map (),
+    m_method2func (false)
 {
   common_initialization (fndecl, NULL, NULL);
 }
@@ -1185,9 +1282,9 @@  ipa_param_body_adjustments
 ::ipa_param_body_adjustments (ipa_param_adjustments *adjustments,
 			      tree fndecl)
   : m_adj_params (adjustments->m_adj_params), m_adjustments (adjustments),
-    m_reset_debug_decls (), m_split_modifications_p (false), m_fndecl (fndecl),
-    m_id (NULL), m_oparms (), m_new_decls (), m_new_types (),
-    m_replacements (), m_removed_decls (), m_removed_map (),
+    m_reset_debug_decls (), m_split_modifications_p (false), m_dead_stmts (),
+    m_dead_ssas (), m_fndecl (fndecl), m_id (NULL), m_oparms (), m_new_decls (),
+    m_new_types (), m_replacements (), m_removed_decls (), m_removed_map (),
     m_method2func (false)
 {
   common_initialization (fndecl, NULL, NULL);
@@ -1208,9 +1305,10 @@  ipa_param_body_adjustments
 			      copy_body_data *id, tree *vars,
 			      vec<ipa_replace_map *, va_gc> *tree_map)
   : m_adj_params (adjustments->m_adj_params), m_adjustments (adjustments),
-    m_reset_debug_decls (), m_split_modifications_p (false), m_fndecl (fndecl),
-    m_id (id), m_oparms (), m_new_decls (), m_new_types (), m_replacements (),
-    m_removed_decls (), m_removed_map (), m_method2func (false)
+    m_reset_debug_decls (), m_split_modifications_p (false), m_dead_stmts (),
+    m_dead_ssas (),m_fndecl (fndecl), m_id (id), m_oparms (), m_new_decls (),
+    m_new_types (), m_replacements (), m_removed_decls (), m_removed_map (),
+    m_method2func (false)
 {
   common_initialization (old_fndecl, vars, tree_map);
 }
@@ -1538,6 +1636,30 @@  remap_split_decl_to_dummy (tree *tp, int *walk_subtrees, void *data)
 }
 
 
+
+/* If the expression *EXPR_P, which is a call argument, should be replaced, do
+   so and retrn true.  Otherwise return false.  */
+
+bool
+ipa_param_body_adjustments::modify_call_argument (tree *expr_p)
+{
+  tree *r;
+  if (TREE_CODE (*expr_p) == SSA_NAME
+      && (r = m_dead_ssas.get (*expr_p)))
+    {
+      /* The argument is not necessary in the callee and IPA-SRA is removing it
+	 from both functions.  The statement argument will be removed during
+	 call redirection, we just need a placeholder here, which we will
+	 create by using the default def of the VAR_DECL already created as a
+	 base to thos SSA names of the removed parameter which remain in the
+	 function (original non-default-defs).  */
+      *expr_p = get_or_create_ssa_default_def (cfun, *r);
+      return true;
+    }
+  else
+    return modify_expression (expr_p, true);
+}
+
 /* If the call statement pointed at by STMT_P contains any expressions that
    need to replaced with a different one as noted by ADJUSTMENTS, do so.  f the
    statement needs to be rebuilt, do so.  Return true if any modifications have
@@ -1676,7 +1798,7 @@  ipa_param_body_adjustments::modify_call_stmt (gcall **stmt_p)
 	  else
 	    {
 	      tree t = gimple_call_arg (stmt, i);
-	      modify_expression (&t, true);
+	      modify_call_argument (&t);
 	      vargs.safe_push (t);
 	    }
 	}
@@ -1700,7 +1822,7 @@  ipa_param_body_adjustments::modify_call_stmt (gcall **stmt_p)
   for (unsigned i = 0; i < nargs; i++)
     {
       tree *t = gimple_call_arg_ptr (stmt, i);
-      modified |= modify_expression (t, true);
+      modified |= modify_call_argument (t);
     }
 
   if (gimple_call_lhs (stmt))
diff --git a/gcc/ipa-param-manipulation.h b/gcc/ipa-param-manipulation.h
index c80e1bc5d6b..2700028537f 100644
--- a/gcc/ipa-param-manipulation.h
+++ b/gcc/ipa-param-manipulation.h
@@ -370,6 +370,13 @@  public:
   /* Set to true if there are any IPA_PARAM_OP_SPLIT adjustments among stored
      adjustments.  */
   bool m_split_modifications_p;
+
+  /* Statements and SSA_NAMEs that only manipulate data from parameters removed
+     because they are not necessary.  SSA_NAMES are mapped to the replacement
+     VAR_DECL of the removed argument.  */
+  hash_set<gimple *> m_dead_stmts;
+  hash_map<tree, tree> m_dead_ssas;
+
 private:
   void common_initialization (tree old_fndecl, tree *vars,
 			      vec<ipa_replace_map *, va_gc> *tree_map);
@@ -379,10 +386,12 @@  private:
 						    unsigned unit_offset);
   tree replace_removed_params_ssa_names (tree old_name, gimple *stmt);
   bool modify_expression (tree *expr_p, bool convert);
+  bool modify_call_argument (tree *expr_p);
   bool modify_assignment (gimple *stmt, gimple_seq *extra_stmts);
   bool modify_call_stmt (gcall **stmt_p);
   bool modify_cfun_body ();
   void reset_debug_stmts ();
+  void mark_dead_statements (tree dead_param, tree repl);
 
   /* Declaration of the function that is being transformed.  */
 
diff --git a/gcc/testsuite/gcc.dg/ipa/ipa-sra-23.c b/gcc/testsuite/gcc.dg/ipa/ipa-sra-23.c
new file mode 100644
index 00000000000..f438b509614
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/ipa/ipa-sra-23.c
@@ -0,0 +1,24 @@ 
+/* { dg-do compile } */
+/* { dg-options "-O2"  } */
+
+extern int g;
+
+static int __attribute__((noinline))
+bar (int i, int j)
+{
+  return 2*g + i;
+}
+
+static int __attribute__((noinline))
+foo (int i, int j)
+{
+  if (i > 5)
+    j = 22;
+  return bar (i, j) + 1;
+}
+
+int
+entry (int l, int k)
+{
+  return foo (l, k);
+}
diff --git a/gcc/testsuite/gcc.dg/ipa/pr93385.c b/gcc/testsuite/gcc.dg/ipa/pr93385.c
new file mode 100644
index 00000000000..6d1d0d7cd27
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/ipa/pr93385.c
@@ -0,0 +1,27 @@ 
+/* { dg-do run } */
+/* { dg-options "-O2 -fno-dce -fno-ipa-cp -fno-tree-dce" } */
+
+char a, b;
+
+#ifdef __SIZEOF_INT128__
+#define T unsigned __int128
+#else
+#define T unsigned
+#endif
+
+static inline int
+c (T d)
+{
+  char e = 0;
+  d %= (unsigned) d;
+  e -= 0;
+  __builtin_strncpy (&a, &e, 1);
+  return e + b;
+}
+
+int
+main (void)
+{
+  c (~0);
+  return 0;
+}
diff --git a/gcc/tree-inline.c b/gcc/tree-inline.c
index 1dcb31c0267..8922ecbb52a 100644
--- a/gcc/tree-inline.c
+++ b/gcc/tree-inline.c
@@ -1528,6 +1528,11 @@  remap_gimple_stmt (gimple *stmt, copy_body_data *id)
 	  : !opt_for_fn (id->dst_fn, flag_var_tracking_assignments)))
     return NULL;
 
+  if (!is_gimple_debug (stmt)
+      && id->param_body_adjs
+      && id->param_body_adjs->m_dead_stmts.contains (stmt))
+    return NULL;
+
   /* Begin by recognizing trees that we'll completely rewrite for the
      inlining context.  Our output for these trees is completely
      different from our input (e.g. RETURN_EXPR is deleted and morphs
@@ -1792,10 +1797,15 @@  remap_gimple_stmt (gimple *stmt, copy_body_data *id)
 
       if (gimple_debug_bind_p (stmt))
 	{
+	  tree value;
+	  if (id->param_body_adjs
+	      && id->param_body_adjs->m_dead_stmts.contains (stmt))
+	    value = NULL_TREE;
+	  else
+	    value = gimple_debug_bind_get_value (stmt);
 	  gdebug *copy
 	    = gimple_build_debug_bind (gimple_debug_bind_get_var (stmt),
-				       gimple_debug_bind_get_value (stmt),
-				       stmt);
+				       value, stmt);
 	  if (id->reset_location)
 	    gimple_set_location (copy, input_location);
 	  id->debug_stmts.safe_push (copy);
@@ -2674,7 +2684,9 @@  copy_phis_for_bb (basic_block bb, copy_body_data *id)
       phi = si.phi ();
       res = PHI_RESULT (phi);
       new_res = res;
-      if (!virtual_operand_p (res))
+      if (!virtual_operand_p (res)
+	  && (!id->param_body_adjs
+	      || !id->param_body_adjs->m_dead_stmts.contains (phi)))
 	{
 	  walk_tree (&new_res, copy_tree_body_r, id, NULL);
 	  if (EDGE_COUNT (new_bb->preds) == 0)