[PATCH 1/2, PR 63814] Strengthen cgraph_edge_brings_value_p

[PATCH 1/2, PR 63814] Strengthen cgraph_edge_brings_value_p

[Martin Jambor]

Hi,

PR 63814 is caused by cgraph_edge_brings_value_p misidentifying an
edge to an expanded artificial thunk as an edge to the original node,
which then leads to crazy double-cloning and doubling the thunks along
the call.

This patch fixes the bug by strengthening the predicate so that it
knows where the value is supposed to go and can check that it goes
there and not anywhere else.  It also adds an extra availability check
that was probably missing in it.

Bootstrapped and tested on x86_64-linux, and i686-linux.  OK for
trunk?

Thanks,

Martin


2014-11-20  Martin Jambor  <mjambor@suse.cz>

	PR ipa/63814
	* ipa-cp.c (same_node_or_its_all_contexts_clone_p): New function.
	(cgraph_edge_brings_value_p): New parameter dest, use
	same_node_or_its_all_contexts_clone_p and check availability.
	(cgraph_edge_brings_value_p): Likewise.
	(get_info_about_necessary_edges): New parameter dest, pass it to
	cgraph_edge_brings_value_p.  Update caller.
	(gather_edges_for_value): Likewise.
	(perhaps_add_new_callers): Use cgraph_edge_brings_value_p to check
	both the destination and availability.


Index: src/gcc/ipa-cp.c
===================================================================
--- src.orig/gcc/ipa-cp.c
+++ src/gcc/ipa-cp.c
@@ -2785,17 +2785,31 @@ get_clone_agg_value (struct cgraph_node
   return NULL_TREE;
 }
 
-/* Return true if edge CS does bring about the value described by SRC.  */
+/* Return true is NODE is DEST or its clone for all contexts.  */
 
 static bool
-cgraph_edge_brings_value_p (struct cgraph_edge *cs,
-			    ipcp_value_source<tree> *src)
+same_node_or_its_all_contexts_clone_p (cgraph_node *node, cgraph_node *dest)
+{
+  if (node == dest)
+    return true;
+
+  struct ipa_node_params *info = IPA_NODE_REF (node);
+  return info->is_all_contexts_clone && info->ipcp_orig_node == dest;
+}
+
+/* Return true if edge CS does bring about the value described by SRC to node
+   DEST or its clone for all contexts.  */
+
+static bool
+cgraph_edge_brings_value_p (cgraph_edge *cs, ipcp_value_source<tree> *src,
+			    cgraph_node *dest)
 {
   struct ipa_node_params *caller_info = IPA_NODE_REF (cs->caller);
-  cgraph_node *real_dest = cs->callee->function_symbol ();
-  struct ipa_node_params *dst_info = IPA_NODE_REF (real_dest);
+  enum availability availability;
+  cgraph_node *real_dest = cs->callee->function_symbol (&availability);
 
-  if ((dst_info->ipcp_orig_node && !dst_info->is_all_contexts_clone)
+  if (!same_node_or_its_all_contexts_clone_p (real_dest, dest)
+      || availability <= AVAIL_INTERPOSABLE
       || caller_info->node_dead)
     return false;
   if (!src->val)
@@ -2834,18 +2848,18 @@ cgraph_edge_brings_value_p (struct cgrap
     }
 }
 
-/* Return true if edge CS does bring about the value described by SRC.  */
+/* Return true if edge CS does bring about the value described by SRC to node
+   DEST or its clone for all contexts.  */
 
 static bool
-cgraph_edge_brings_value_p (struct cgraph_edge *cs,
-			    ipcp_value_source<ipa_polymorphic_call_context>
-			    *src)
+cgraph_edge_brings_value_p (cgraph_edge *cs,
+			    ipcp_value_source<ipa_polymorphic_call_context> *src,
+			    cgraph_node *dest)
 {
   struct ipa_node_params *caller_info = IPA_NODE_REF (cs->caller);
   cgraph_node *real_dest = cs->callee->function_symbol ();
-  struct ipa_node_params *dst_info = IPA_NODE_REF (real_dest);
 
-  if ((dst_info->ipcp_orig_node && !dst_info->is_all_contexts_clone)
+  if (!same_node_or_its_all_contexts_clone_p (real_dest, dest)
       || caller_info->node_dead)
     return false;
   if (!src->val)
@@ -2871,13 +2885,14 @@ get_next_cgraph_edge_clone (struct cgrap
   return next_edge_clone[cs->uid];
 }
 
-/* Given VAL, iterate over all its sources and if they still hold, add their
-   edge frequency and their number into *FREQUENCY and *CALLER_COUNT
-   respectively.  */
+/* Given VAL that is intended for DEST, iterate over all its sources and if
+   they still hold, add their edge frequency and their number into *FREQUENCY
+   and *CALLER_COUNT respectively.  */
 
 template <typename valtype>
 static bool
-get_info_about_necessary_edges (ipcp_value<valtype> *val, int *freq_sum,
+get_info_about_necessary_edges (ipcp_value<valtype> *val, cgraph_node *dest,
+				int *freq_sum,
 				gcov_type *count_sum, int *caller_count)
 {
   ipcp_value_source<valtype> *src;
@@ -2890,7 +2905,7 @@ get_info_about_necessary_edges (ipcp_val
       struct cgraph_edge *cs = src->cs;
       while (cs)
 	{
-	  if (cgraph_edge_brings_value_p (cs, src))
+	  if (cgraph_edge_brings_value_p (cs, src, dest))
 	    {
 	      count++;
 	      freq += cs->frequency;
@@ -2907,12 +2922,13 @@ get_info_about_necessary_edges (ipcp_val
   return hot;
 }
 
-/* Return a vector of incoming edges that do bring value VAL.  It is assumed
-   their number is known and equal to CALLER_COUNT.  */
+/* Return a vector of incoming edges that do bring value VAL to node DEST.  It
+   is assumed their number is known and equal to CALLER_COUNT.  */
 
 template <typename valtype>
 static vec<cgraph_edge *>
-gather_edges_for_value (ipcp_value<valtype> *val, int caller_count)
+gather_edges_for_value (ipcp_value<valtype> *val, cgraph_node *dest,
+			int caller_count)
 {
   ipcp_value_source<valtype> *src;
   vec<cgraph_edge *> ret;
@@ -2923,7 +2939,7 @@ gather_edges_for_value (ipcp_value<valty
       struct cgraph_edge *cs = src->cs;
       while (cs)
 	{
-	  if (cgraph_edge_brings_value_p (cs, src))
+	  if (cgraph_edge_brings_value_p (cs, src, dest))
 	    ret.quick_push (cs);
 	  cs = get_next_cgraph_edge_clone (cs);
 	}
@@ -3784,27 +3800,20 @@ perhaps_add_new_callers (cgraph_node *no
       struct cgraph_edge *cs = src->cs;
       while (cs)
 	{
-	  enum availability availability;
-	  struct cgraph_node *dst = cs->callee->function_symbol (&availability);
-	  if ((dst == node || IPA_NODE_REF (dst)->is_all_contexts_clone)
-	      && availability > AVAIL_INTERPOSABLE
-	      && cgraph_edge_brings_value_p (cs, src))
+	  if (cgraph_edge_brings_value_p (cs, src, node)
+	      && cgraph_edge_brings_all_scalars_for_node (cs, val->spec_node)
+	      && cgraph_edge_brings_all_agg_vals_for_node (cs, val->spec_node))
 	    {
-	      if (cgraph_edge_brings_all_scalars_for_node (cs, val->spec_node)
-		  && cgraph_edge_brings_all_agg_vals_for_node (cs,
-							       val->spec_node))
-		{
-		  if (dump_file)
-		    fprintf (dump_file, " - adding an extra caller %s/%i"
-			     " of %s/%i\n",
-			     xstrdup (cs->caller->name ()),
-			     cs->caller->order,
-			     xstrdup (val->spec_node->name ()),
-			     val->spec_node->order);
+	      if (dump_file)
+		fprintf (dump_file, " - adding an extra caller %s/%i"
+			 " of %s/%i\n",
+			 xstrdup (cs->caller->name ()),
+			 cs->caller->order,
+			 xstrdup (val->spec_node->name ()),
+			 val->spec_node->order);
 
-		  cs->redirect_callee (val->spec_node);
-		  redirected_sum += cs->count;
-		}
+	      cs->redirect_callee (val->spec_node);
+	      redirected_sum += cs->count;
 	    }
 	  cs = get_next_cgraph_edge_clone (cs);
 	}
@@ -3929,7 +3938,7 @@ decide_about_value (struct cgraph_node *
 		 val->local_size_cost + overall_size);
       return false;
     }
-  else if (!get_info_about_necessary_edges (val, &freq_sum, &count_sum,
+  else if (!get_info_about_necessary_edges (val, node, &freq_sum, &count_sum,
 					    &caller_count))
     return false;
 
@@ -3959,7 +3968,7 @@ decide_about_value (struct cgraph_node *
     fprintf (dump_file, "  Creating a specialized node of %s/%i.\n",
 	     node->name (), node->order);
 
-  callers = gather_edges_for_value (val, caller_count);
+  callers = gather_edges_for_value (val, node, caller_count);
   if (offset == -1)
     modify_known_vectors_with_val (&known_csts, &known_contexts, val, index);
   else

Re: [PATCH 1/2, PR 63814] Strengthen cgraph_edge_brings_value_p

[Martin Jambor]

Ping.

Thx,

Martin

On Fri, Nov 21, 2014 at 07:59:11PM +0100, Martin Jambor wrote:
> Hi,
> 
> PR 63814 is caused by cgraph_edge_brings_value_p misidentifying an
> edge to an expanded artificial thunk as an edge to the original node,
> which then leads to crazy double-cloning and doubling the thunks along
> the call.
> 
> This patch fixes the bug by strengthening the predicate so that it
> knows where the value is supposed to go and can check that it goes
> there and not anywhere else.  It also adds an extra availability check
> that was probably missing in it.
> 
> Bootstrapped and tested on x86_64-linux, and i686-linux.  OK for
> trunk?
> 
> Thanks,
> 
> Martin
> 
> 
> 2014-11-20  Martin Jambor  <mjambor@suse.cz>
> 
> 	PR ipa/63814
> 	* ipa-cp.c (same_node_or_its_all_contexts_clone_p): New function.
> 	(cgraph_edge_brings_value_p): New parameter dest, use
> 	same_node_or_its_all_contexts_clone_p and check availability.
> 	(cgraph_edge_brings_value_p): Likewise.
> 	(get_info_about_necessary_edges): New parameter dest, pass it to
> 	cgraph_edge_brings_value_p.  Update caller.
> 	(gather_edges_for_value): Likewise.
> 	(perhaps_add_new_callers): Use cgraph_edge_brings_value_p to check
> 	both the destination and availability.
> 
> 
> Index: src/gcc/ipa-cp.c
> ===================================================================
> --- src.orig/gcc/ipa-cp.c
> +++ src/gcc/ipa-cp.c
> @@ -2785,17 +2785,31 @@ get_clone_agg_value (struct cgraph_node
>    return NULL_TREE;
>  }
>  
> -/* Return true if edge CS does bring about the value described by SRC.  */
> +/* Return true is NODE is DEST or its clone for all contexts.  */
>  
>  static bool
> -cgraph_edge_brings_value_p (struct cgraph_edge *cs,
> -			    ipcp_value_source<tree> *src)
> +same_node_or_its_all_contexts_clone_p (cgraph_node *node, cgraph_node *dest)
> +{
> +  if (node == dest)
> +    return true;
> +
> +  struct ipa_node_params *info = IPA_NODE_REF (node);
> +  return info->is_all_contexts_clone && info->ipcp_orig_node == dest;
> +}
> +
> +/* Return true if edge CS does bring about the value described by SRC to node
> +   DEST or its clone for all contexts.  */
> +
> +static bool
> +cgraph_edge_brings_value_p (cgraph_edge *cs, ipcp_value_source<tree> *src,
> +			    cgraph_node *dest)
>  {
>    struct ipa_node_params *caller_info = IPA_NODE_REF (cs->caller);
> -  cgraph_node *real_dest = cs->callee->function_symbol ();
> -  struct ipa_node_params *dst_info = IPA_NODE_REF (real_dest);
> +  enum availability availability;
> +  cgraph_node *real_dest = cs->callee->function_symbol (&availability);
>  
> -  if ((dst_info->ipcp_orig_node && !dst_info->is_all_contexts_clone)
> +  if (!same_node_or_its_all_contexts_clone_p (real_dest, dest)
> +      || availability <= AVAIL_INTERPOSABLE
>        || caller_info->node_dead)
>      return false;
>    if (!src->val)
> @@ -2834,18 +2848,18 @@ cgraph_edge_brings_value_p (struct cgrap
>      }
>  }
>  
> -/* Return true if edge CS does bring about the value described by SRC.  */
> +/* Return true if edge CS does bring about the value described by SRC to node
> +   DEST or its clone for all contexts.  */
>  
>  static bool
> -cgraph_edge_brings_value_p (struct cgraph_edge *cs,
> -			    ipcp_value_source<ipa_polymorphic_call_context>
> -			    *src)
> +cgraph_edge_brings_value_p (cgraph_edge *cs,
> +			    ipcp_value_source<ipa_polymorphic_call_context> *src,
> +			    cgraph_node *dest)
>  {
>    struct ipa_node_params *caller_info = IPA_NODE_REF (cs->caller);
>    cgraph_node *real_dest = cs->callee->function_symbol ();
> -  struct ipa_node_params *dst_info = IPA_NODE_REF (real_dest);
>  
> -  if ((dst_info->ipcp_orig_node && !dst_info->is_all_contexts_clone)
> +  if (!same_node_or_its_all_contexts_clone_p (real_dest, dest)
>        || caller_info->node_dead)
>      return false;
>    if (!src->val)
> @@ -2871,13 +2885,14 @@ get_next_cgraph_edge_clone (struct cgrap
>    return next_edge_clone[cs->uid];
>  }
>  
> -/* Given VAL, iterate over all its sources and if they still hold, add their
> -   edge frequency and their number into *FREQUENCY and *CALLER_COUNT
> -   respectively.  */
> +/* Given VAL that is intended for DEST, iterate over all its sources and if
> +   they still hold, add their edge frequency and their number into *FREQUENCY
> +   and *CALLER_COUNT respectively.  */
>  
>  template <typename valtype>
>  static bool
> -get_info_about_necessary_edges (ipcp_value<valtype> *val, int *freq_sum,
> +get_info_about_necessary_edges (ipcp_value<valtype> *val, cgraph_node *dest,
> +				int *freq_sum,
>  				gcov_type *count_sum, int *caller_count)
>  {
>    ipcp_value_source<valtype> *src;
> @@ -2890,7 +2905,7 @@ get_info_about_necessary_edges (ipcp_val
>        struct cgraph_edge *cs = src->cs;
>        while (cs)
>  	{
> -	  if (cgraph_edge_brings_value_p (cs, src))
> +	  if (cgraph_edge_brings_value_p (cs, src, dest))
>  	    {
>  	      count++;
>  	      freq += cs->frequency;
> @@ -2907,12 +2922,13 @@ get_info_about_necessary_edges (ipcp_val
>    return hot;
>  }
>  
> -/* Return a vector of incoming edges that do bring value VAL.  It is assumed
> -   their number is known and equal to CALLER_COUNT.  */
> +/* Return a vector of incoming edges that do bring value VAL to node DEST.  It
> +   is assumed their number is known and equal to CALLER_COUNT.  */
>  
>  template <typename valtype>
>  static vec<cgraph_edge *>
> -gather_edges_for_value (ipcp_value<valtype> *val, int caller_count)
> +gather_edges_for_value (ipcp_value<valtype> *val, cgraph_node *dest,
> +			int caller_count)
>  {
>    ipcp_value_source<valtype> *src;
>    vec<cgraph_edge *> ret;
> @@ -2923,7 +2939,7 @@ gather_edges_for_value (ipcp_value<valty
>        struct cgraph_edge *cs = src->cs;
>        while (cs)
>  	{
> -	  if (cgraph_edge_brings_value_p (cs, src))
> +	  if (cgraph_edge_brings_value_p (cs, src, dest))
>  	    ret.quick_push (cs);
>  	  cs = get_next_cgraph_edge_clone (cs);
>  	}
> @@ -3784,27 +3800,20 @@ perhaps_add_new_callers (cgraph_node *no
>        struct cgraph_edge *cs = src->cs;
>        while (cs)
>  	{
> -	  enum availability availability;
> -	  struct cgraph_node *dst = cs->callee->function_symbol (&availability);
> -	  if ((dst == node || IPA_NODE_REF (dst)->is_all_contexts_clone)
> -	      && availability > AVAIL_INTERPOSABLE
> -	      && cgraph_edge_brings_value_p (cs, src))
> +	  if (cgraph_edge_brings_value_p (cs, src, node)
> +	      && cgraph_edge_brings_all_scalars_for_node (cs, val->spec_node)
> +	      && cgraph_edge_brings_all_agg_vals_for_node (cs, val->spec_node))
>  	    {
> -	      if (cgraph_edge_brings_all_scalars_for_node (cs, val->spec_node)
> -		  && cgraph_edge_brings_all_agg_vals_for_node (cs,
> -							       val->spec_node))
> -		{
> -		  if (dump_file)
> -		    fprintf (dump_file, " - adding an extra caller %s/%i"
> -			     " of %s/%i\n",
> -			     xstrdup (cs->caller->name ()),
> -			     cs->caller->order,
> -			     xstrdup (val->spec_node->name ()),
> -			     val->spec_node->order);
> +	      if (dump_file)
> +		fprintf (dump_file, " - adding an extra caller %s/%i"
> +			 " of %s/%i\n",
> +			 xstrdup (cs->caller->name ()),
> +			 cs->caller->order,
> +			 xstrdup (val->spec_node->name ()),
> +			 val->spec_node->order);
>  
> -		  cs->redirect_callee (val->spec_node);
> -		  redirected_sum += cs->count;
> -		}
> +	      cs->redirect_callee (val->spec_node);
> +	      redirected_sum += cs->count;
>  	    }
>  	  cs = get_next_cgraph_edge_clone (cs);
>  	}
> @@ -3929,7 +3938,7 @@ decide_about_value (struct cgraph_node *
>  		 val->local_size_cost + overall_size);
>        return false;
>      }
> -  else if (!get_info_about_necessary_edges (val, &freq_sum, &count_sum,
> +  else if (!get_info_about_necessary_edges (val, node, &freq_sum, &count_sum,
>  					    &caller_count))
>      return false;
>  
> @@ -3959,7 +3968,7 @@ decide_about_value (struct cgraph_node *
>      fprintf (dump_file, "  Creating a specialized node of %s/%i.\n",
>  	     node->name (), node->order);
>  
> -  callers = gather_edges_for_value (val, caller_count);
> +  callers = gather_edges_for_value (val, node, caller_count);
>    if (offset == -1)
>      modify_known_vectors_with_val (&known_csts, &known_contexts, val, index);
>    else

Re: [PATCH 1/2, PR 63814] Strengthen cgraph_edge_brings_value_p

[Jan Hubicka]

> > Hi,
> > 
> > PR 63814 is caused by cgraph_edge_brings_value_p misidentifying an
> > edge to an expanded artificial thunk as an edge to the original node,
> > which then leads to crazy double-cloning and doubling the thunks along
> > the call.
> > 
> > This patch fixes the bug by strengthening the predicate so that it
> > knows where the value is supposed to go and can check that it goes
> > there and not anywhere else.  It also adds an extra availability check
> > that was probably missing in it.
> > 
> > Bootstrapped and tested on x86_64-linux, and i686-linux.  OK for
> > trunk?
> > 
> > Thanks,
> > 
> > Martin
> > 
> > 
> > 2014-11-20  Martin Jambor  <mjambor@suse.cz>
> > 
> > 	PR ipa/63814
> > 	* ipa-cp.c (same_node_or_its_all_contexts_clone_p): New function.
> > 	(cgraph_edge_brings_value_p): New parameter dest, use
> > 	same_node_or_its_all_contexts_clone_p and check availability.
> > 	(cgraph_edge_brings_value_p): Likewise.
> > 	(get_info_about_necessary_edges): New parameter dest, pass it to
> > 	cgraph_edge_brings_value_p.  Update caller.
> > 	(gather_edges_for_value): Likewise.
> > 	(perhaps_add_new_callers): Use cgraph_edge_brings_value_p to check
> > 	both the destination and availability.

OK

Re: [PATCH 1/2, PR 63814] Strengthen cgraph_edge_brings_value_p

[H.J. Lu]

On Fri, Nov 21, 2014 at 10:59 AM, Martin Jambor <mjambor@suse.cz> wrote:
> Hi,
>
> PR 63814 is caused by cgraph_edge_brings_value_p misidentifying an
> edge to an expanded artificial thunk as an edge to the original node,
> which then leads to crazy double-cloning and doubling the thunks along
> the call.
>
> This patch fixes the bug by strengthening the predicate so that it
> knows where the value is supposed to go and can check that it goes
> there and not anywhere else.  It also adds an extra availability check
> that was probably missing in it.
>
> Bootstrapped and tested on x86_64-linux, and i686-linux.  OK for
> trunk?
>
> Thanks,
>
> Martin
>
>
> 2014-11-20  Martin Jambor  <mjambor@suse.cz>
>
>         PR ipa/63814
>         * ipa-cp.c (same_node_or_its_all_contexts_clone_p): New function.
>         (cgraph_edge_brings_value_p): New parameter dest, use
>         same_node_or_its_all_contexts_clone_p and check availability.
>         (cgraph_edge_brings_value_p): Likewise.
>         (get_info_about_necessary_edges): New parameter dest, pass it to
>         cgraph_edge_brings_value_p.  Update caller.
>         (gather_edges_for_value): Likewise.
>         (perhaps_add_new_callers): Use cgraph_edge_brings_value_p to check
>         both the destination and availability.
>
>

I checked in this testcase:

diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog
index a17cc6c..1410f10 100644
--- a/gcc/testsuite/ChangeLog
+++ b/gcc/testsuite/ChangeLog
@@ -1,3 +1,8 @@
+2014-12-02  H.J. Lu  <hongjiu.lu@intel.com>
+
+ PR ipa/63814
+ * g++.dg/ipa/pr63814.C: New test.
+
 2014-12-02  Wilco Dijkstra  <wilco.dijkstra@arm.com>

  * gcc.target/aarch64/remat1.c: New testcase.
diff --git a/gcc/testsuite/g++.dg/ipa/pr63814.C
b/gcc/testsuite/g++.dg/ipa/pr63814.C
new file mode 100644
index 0000000..15a7dda
--- /dev/null
+++ b/gcc/testsuite/g++.dg/ipa/pr63814.C
@@ -0,0 +1,29 @@
+// { dg-do run { target fpic } }
+// { dg-options "-O3 -fpic" }
+
+struct CBase {
+  virtual void BaseFunc () {}
+};
+
+struct MMixin {
+  virtual void * MixinFunc (int, int) = 0;
+};
+
+struct CExample: CBase, public MMixin
+{
+  void *MixinFunc (int arg, int arg2)
+  {
+    return this;
+  }
+};
+
+void *test (MMixin & anExample)
+{
+  return anExample.MixinFunc (0, 0);
+}
+
+int main ()
+{
+  CExample c;
+  return (test (c) != &c);
+}

H.J.