[PATCH] Fix bug in recursiveness check for function to be cloned (ipa/pr93707)

Martin Jambor mjambor@suse.cz
Wed Feb 19 16:28:00 GMT 2020


Hi,

On Thu, Feb 13 2020, Feng Xue OS wrote:
> I've submitted a bug tracker, https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93707.
>
> The root cause is that for a self-recursive function, a for-all-contexts clone could generate
> an edge whose callee is not the function. Therefore, to check whether an edge stands for a
> recursive call during cloning, we should not use ordinary way of comparing caller and callee
> of the edge.
>
> Bootstrapped/regtested on x86_64-linux and aarch64-linux, and also tested Spec 2017 with
> option "--param ipa-cp-eval-threshold=1". 
>
> Feng
> ---
> 2020-02-13  Feng Xue  <fxue@os.amperecomputing.com>
>
>         PR ipa/93707
>         * ipa-cp.c (self_recursive_pass_through_p): Add a new parameter "node",
>         and check recursiveness by comparing "node" and cs->caller.
>         (self_recursive_agg_pass_through_p): Likewise.
>         (find_more_scalar_values_for_callers_subset): Add "node" argument to
>         self_recursive_pass_through_p call.
>         (intersect_aggregates_with_edge): Add a new parameter "node", and add
>         "node" argument to self_cursive_agg_pass_through_p call.
>         (find_aggregate_values_for_callers_subset): Add "node" argument to
>         self_recursive_pass_through_p and intersect_aggregates_with_edge calls.
>         (cgraph_edge_brings_all_agg_vals_for_node): Add "node" argument to
>         intersect_aggregates_with_edge call.

first, thanks for a very nice testcase and for working on this.

However, I believe that his approach mostly papers over a bug that
happens earlier, specifically that cgraph_edge_brings_value_p returned
true for the self-recursive edge from all-context clone to itself, even
though when evaluating the second argument.  We assume that all context
clones get at least as many constants as any other potential clone, but
that does not work for self-recursive edges with pass-through parameters
that that just pass along the received constant.

That is how we got a wrong edge in the vector of edges bringing the
constant and it is the primary reason why self_recursive_pass_through_p
and its users did not work correctly.

I would therefore like to fix the problem with the following patch.  It
has passed bootstrap and testing on x86_64-linux, LTO bootstrap is
underway.

Honza, is it OK for trunk?  Tamar, can you please double check it fixes
your problem with perlbench?

Thanks,

Martin


ipa-cp: Avoid wrongly gathering self-recursive edges  (PR 93707)

2020-02-18  Martin Jambor  <mjambor@suse.cz>
	    Feng Xue  <fxue@os.amperecomputing.com>

	PR ipa/93707
	* ipa-cp.c (same_node_or_its_all_contexts_clone_p): Replaced with
	new function calls_same_node_or_its_all_contexts_clone_p.
	(cgraph_edge_brings_value_p): Use it.  Fix comment.
	(cgraph_edge_brings_value_p): Likewise.

	testsuite/
	* gcc.dg/ipa/pr93707.c: New test.
---
 gcc/ChangeLog                      |  9 +++++++++
 gcc/ipa-cp.c                       | 29 ++++++++++++++---------------
 gcc/testsuite/ChangeLog            |  6 ++++++
 gcc/testsuite/gcc.dg/ipa/pr93707.c | 29 +++++++++++++++++++++++++++++
 4 files changed, 58 insertions(+), 15 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/ipa/pr93707.c

diff --git a/gcc/ipa-cp.c b/gcc/ipa-cp.c
index 4f5b72e6994..4609375bf8d 100644
--- a/gcc/ipa-cp.c
+++ b/gcc/ipa-cp.c
@@ -4033,15 +4033,23 @@ edge_clone_summary_t::duplicate (cgraph_edge *src_edge, cgraph_edge *dst_edge,
   src_data->next_clone = dst_edge;
 }
 
-/* Return true is NODE is DEST or its clone for all contexts.  */
+/* Return true is CS calls DEST or its clone for all contexts, except for
+   self-recursive nodes in which it has to be DEST itself.  */
 
 static bool
-same_node_or_its_all_contexts_clone_p (cgraph_node *node, cgraph_node *dest)
+calls_same_node_or_its_all_contexts_clone_p (cgraph_edge *cs, cgraph_node *dest)
 {
-  if (node == dest)
+  enum availability availability;
+  cgraph_node *callee = cs->callee->function_symbol (&availability);
+
+  if (availability <= AVAIL_INTERPOSABLE)
+    return false;
+  if (callee == dest)
     return true;
+  if (cs->caller == callee)
+    return false;
 
-  class ipa_node_params *info = IPA_NODE_REF (node);
+  class ipa_node_params *info = IPA_NODE_REF (callee);
   return info->is_all_contexts_clone && info->ipcp_orig_node == dest;
 }
 
@@ -4053,11 +4061,8 @@ cgraph_edge_brings_value_p (cgraph_edge *cs, ipcp_value_source<tree> *src,
 			    cgraph_node *dest, ipcp_value<tree> *dest_val)
 {
   class ipa_node_params *caller_info = IPA_NODE_REF (cs->caller);
-  enum availability availability;
-  cgraph_node *real_dest = cs->callee->function_symbol (&availability);
 
-  if (availability <= AVAIL_INTERPOSABLE
-      || !same_node_or_its_all_contexts_clone_p (real_dest, dest)
+  if (!calls_same_node_or_its_all_contexts_clone_p (cs, dest)
       || caller_info->node_dead)
     return false;
 
@@ -4076,9 +4081,6 @@ cgraph_edge_brings_value_p (cgraph_edge *cs, ipcp_value_source<tree> *src,
     }
   else
     {
-      /* At the moment we do not propagate over arithmetic jump functions in
-	 SCCs, so it is safe to detect self-feeding recursive calls in this
-	 way.  */
       if (src->val == dest_val)
 	return true;
 
@@ -4113,11 +4115,8 @@ cgraph_edge_brings_value_p (cgraph_edge *cs,
 			    ipcp_value<ipa_polymorphic_call_context> *)
 {
   class ipa_node_params *caller_info = IPA_NODE_REF (cs->caller);
-  enum availability avail;
-  cgraph_node *real_dest = cs->callee->function_symbol (&avail);
 
-  if (avail <= AVAIL_INTERPOSABLE
-      || !same_node_or_its_all_contexts_clone_p (real_dest, dest)
+  if (!calls_same_node_or_its_all_contexts_clone_p (cs, dest)
       || caller_info->node_dead)
     return false;
   if (!src->val)
diff --git a/gcc/testsuite/gcc.dg/ipa/pr93707.c b/gcc/testsuite/gcc.dg/ipa/pr93707.c
new file mode 100644
index 00000000000..e200a3a432b
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/ipa/pr93707.c
@@ -0,0 +1,29 @@
+/* { dg-do compile } */
+/* { dg-options "-O3 --param ipa-cp-eval-threshold=1" } */
+
+int foo();
+int data[100];
+
+__attribute__((noinline)) static int recur_fn (int i, int j, int depth)
+{
+   if (depth > 10)
+     return 1;
+
+   data[i + j]++;
+
+   if (depth & 3)
+     recur_fn (i, 1, depth + 1);
+   else
+     recur_fn (i, j & 1, depth + 1);
+
+   foo();
+
+   return i + j;
+}
+
+int caller (int v, int depth)
+{
+  recur_fn (1, v, depth);
+
+  return 0;
+}
-- 
2.25.0



More information about the Gcc-patches mailing list