[PR,61160] Artificial thunks need combined_args_to_skip

Hi,

the second issue in PR 61160 is that because artificial thunks
(produced by duplicate_thunk_for_node) do not have
combined_args_to_skip, calls to them do not get actual arguments
removed, while the actual functions do loose their formal parameters,
leading to mismatches.

Currently, the combined_args_to_skip is computed in of
cgraph_create_virtual_clone only after all the edge redirection and
thunk duplication is done so it had to be moved to a spot before
that.  Since we already pass args_to_skip to cgraph_clone_node, I
moved the computation there (otherwise it would have to duplicate the
old value and also pass the new one to the redirection routine).

I have also noticed that the code producing combined_args_to_skip from
an old value and new args_to_skip cannot work in LTO because we do not
have DECL_ARGUMENTS available at WPA in LTO.  The wrong code is
however never executed and so I replaced it with a simple bitmap_ior.
This changes the semantics of args_to_skip for any user of
cgraph_create_virtual_clone that would like to remove some parameters
from something which is already a clone.  However, currently there are
no such users and the new semantics is saner because WPA code will be
happier using the old indices rather than remapping everything the
whole time.

I am still in the process of bootstrapping and testing this patch on
trunk, I will test it on the 4.9 branch too.  OK if it passes
everywhere?

Thanks,

Martin



2014-05-29  Martin Jambor  <mjambor@suse.cz>

	PR ipa/61160
	* cgraphclones.c (duplicate_thunk_for_node): Removed parameter
	args_to_skip, use those from node instead.  Copy args_to_skip and
	combined_args_to_skip from node to the new thunk.
	(redirect_edge_duplicating_thunks): Removed parameter args_to_skip.
	(cgraph_create_virtual_clone): Moved computation of
	combined_args_to_skip...
	(cgraph_clone_node): ...here, simplify it to bitmap_ior..

testsuite/
	* g++.dg/ipa/pr61160-2.C: New test.
	* g++.dg/ipa/pr61160-3.C: Likewise.

Hi,

Ping.

Thanks,

Martin


On Sat, May 31, 2014 at 01:08:31AM +0200, Martin Jambor wrote:
> Hi,
> 
> the second issue in PR 61160 is that because artificial thunks
> (produced by duplicate_thunk_for_node) do not have
> combined_args_to_skip, calls to them do not get actual arguments
> removed, while the actual functions do loose their formal parameters,
> leading to mismatches.
> 
> Currently, the combined_args_to_skip is computed in of
> cgraph_create_virtual_clone only after all the edge redirection and
> thunk duplication is done so it had to be moved to a spot before
> that.  Since we already pass args_to_skip to cgraph_clone_node, I
> moved the computation there (otherwise it would have to duplicate the
> old value and also pass the new one to the redirection routine).
> 
> I have also noticed that the code producing combined_args_to_skip from
> an old value and new args_to_skip cannot work in LTO because we do not
> have DECL_ARGUMENTS available at WPA in LTO.  The wrong code is
> however never executed and so I replaced it with a simple bitmap_ior.
> This changes the semantics of args_to_skip for any user of
> cgraph_create_virtual_clone that would like to remove some parameters
> from something which is already a clone.  However, currently there are
> no such users and the new semantics is saner because WPA code will be
> happier using the old indices rather than remapping everything the
> whole time.
> 
> I am still in the process of bootstrapping and testing this patch on
> trunk, I will test it on the 4.9 branch too.  OK if it passes
> everywhere?
> 
> Thanks,
> 
> Martin
> 
> 
> 
> 2014-05-29  Martin Jambor  <mjambor@suse.cz>
> 
> 	PR ipa/61160
> 	* cgraphclones.c (duplicate_thunk_for_node): Removed parameter
> 	args_to_skip, use those from node instead.  Copy args_to_skip and
> 	combined_args_to_skip from node to the new thunk.
> 	(redirect_edge_duplicating_thunks): Removed parameter args_to_skip.
> 	(cgraph_create_virtual_clone): Moved computation of
> 	combined_args_to_skip...
> 	(cgraph_clone_node): ...here, simplify it to bitmap_ior..
> 
> testsuite/
> 	* g++.dg/ipa/pr61160-2.C: New test.
> 	* g++.dg/ipa/pr61160-3.C: Likewise.
> 
> diff --git a/gcc/cgraphclones.c b/gcc/cgraphclones.c
> index 4387b99..91cc13c 100644
> --- a/gcc/cgraphclones.c
> +++ b/gcc/cgraphclones.c
> @@ -301,14 +301,13 @@ set_new_clone_decl_and_node_flags (cgraph_node *new_node)
>     thunk is this_adjusting but we are removing this parameter.  */
>  
>  static cgraph_node *
> -duplicate_thunk_for_node (cgraph_node *thunk, cgraph_node *node,
> -			  bitmap args_to_skip)
> +duplicate_thunk_for_node (cgraph_node *thunk, cgraph_node *node)
>  {
>    cgraph_node *new_thunk, *thunk_of;
>    thunk_of = cgraph_function_or_thunk_node (thunk->callees->callee);
>  
>    if (thunk_of->thunk.thunk_p)
> -    node = duplicate_thunk_for_node (thunk_of, node, args_to_skip);
> +    node = duplicate_thunk_for_node (thunk_of, node);
>  
>    struct cgraph_edge *cs;
>    for (cs = node->callers; cs; cs = cs->next_caller)
> @@ -320,17 +319,18 @@ duplicate_thunk_for_node (cgraph_node *thunk, cgraph_node *node,
>        return cs->caller;
>  
>    tree new_decl;
> -  if (!args_to_skip)
> +  if (!node->clone.args_to_skip)
>      new_decl = copy_node (thunk->decl);
>    else
>      {
>        /* We do not need to duplicate this_adjusting thunks if we have removed
>  	 this.  */
>        if (thunk->thunk.this_adjusting
> -	  && bitmap_bit_p (args_to_skip, 0))
> +	  && bitmap_bit_p (node->clone.args_to_skip, 0))
>  	return node;
>  
> -      new_decl = build_function_decl_skip_args (thunk->decl, args_to_skip,
> +      new_decl = build_function_decl_skip_args (thunk->decl,
> +						node->clone.args_to_skip,
>  						false);
>      }
>    gcc_checking_assert (!DECL_STRUCT_FUNCTION (new_decl));
> @@ -348,6 +348,8 @@ duplicate_thunk_for_node (cgraph_node *thunk, cgraph_node *node,
>    new_thunk->thunk = thunk->thunk;
>    new_thunk->unique_name = in_lto_p;
>    new_thunk->former_clone_of = thunk->decl;
> +  new_thunk->clone.args_to_skip = node->clone.args_to_skip;
> +  new_thunk->clone.combined_args_to_skip = node->clone.combined_args_to_skip;
>  
>    struct cgraph_edge *e = cgraph_create_edge (new_thunk, node, NULL, 0,
>  					      CGRAPH_FREQ_BASE);
> @@ -364,12 +366,11 @@ duplicate_thunk_for_node (cgraph_node *thunk, cgraph_node *node,
>     chain.  */
>  
>  void
> -redirect_edge_duplicating_thunks (struct cgraph_edge *e, struct cgraph_node *n,
> -				  bitmap args_to_skip)
> +redirect_edge_duplicating_thunks (struct cgraph_edge *e, struct cgraph_node *n)
>  {
>    cgraph_node *orig_to = cgraph_function_or_thunk_node (e->callee);
>    if (orig_to->thunk.thunk_p)
> -    n = duplicate_thunk_for_node (orig_to, n, args_to_skip);
> +    n = duplicate_thunk_for_node (orig_to, n);
>  
>    cgraph_redirect_edge_callee (e, n);
>  }
> @@ -422,9 +423,21 @@ cgraph_clone_node (struct cgraph_node *n, tree decl, gcov_type count, int freq,
>    new_node->rtl = n->rtl;
>    new_node->count = count;
>    new_node->frequency = n->frequency;
> -  new_node->clone = n->clone;
> -  new_node->clone.tree_map = NULL;
>    new_node->tp_first_run = n->tp_first_run;
> +
> +  new_node->clone.tree_map = NULL;
> +  new_node->clone.args_to_skip = args_to_skip;
> +  if (!args_to_skip)
> +    new_node->clone.combined_args_to_skip = n->clone.combined_args_to_skip;
> +  else if (n->clone.combined_args_to_skip)
> +    {
> +      new_node->clone.combined_args_to_skip = BITMAP_GGC_ALLOC ();
> +      bitmap_ior (new_node->clone.combined_args_to_skip,
> +		  n->clone.combined_args_to_skip, args_to_skip);
> +    }
> +  else
> +    new_node->clone.combined_args_to_skip = args_to_skip;
> +
>    if (n->count)
>      {
>        if (new_node->count > n->count)
> @@ -449,10 +462,9 @@ cgraph_clone_node (struct cgraph_node *n, tree decl, gcov_type count, int freq,
>        if (!e->callee
>  	  || DECL_BUILT_IN_CLASS (e->callee->decl) != BUILT_IN_NORMAL
>  	  || DECL_FUNCTION_CODE (e->callee->decl) != BUILT_IN_UNREACHABLE)
> -        redirect_edge_duplicating_thunks (e, new_node, args_to_skip);
> +        redirect_edge_duplicating_thunks (e, new_node);
>      }
>  
> -
>    for (e = n->callees;e; e=e->next_callee)
>      cgraph_clone_edge (e, new_node, e->call_stmt, e->lto_stmt_uid,
>  		       count_scale, freq, update_original);
> @@ -561,7 +573,6 @@ cgraph_create_virtual_clone (struct cgraph_node *old_node,
>      DECL_SECTION_NAME (new_node->decl) = NULL;
>    set_new_clone_decl_and_node_flags (new_node);
>    new_node->clone.tree_map = tree_map;
> -  new_node->clone.args_to_skip = args_to_skip;
>  
>    /* Clones of global symbols or symbols with unique names are unique.  */
>    if ((TREE_PUBLIC (old_decl)
> @@ -573,32 +584,7 @@ cgraph_create_virtual_clone (struct cgraph_node *old_node,
>    FOR_EACH_VEC_SAFE_ELT (tree_map, i, map)
>      ipa_maybe_record_reference (new_node, map->new_tree,
>  				IPA_REF_ADDR, NULL);
> -  if (!args_to_skip)
> -    new_node->clone.combined_args_to_skip = old_node->clone.combined_args_to_skip;
> -  else if (old_node->clone.combined_args_to_skip)
> -    {
> -      int newi = 0, oldi = 0;
> -      tree arg;
> -      bitmap new_args_to_skip = BITMAP_GGC_ALLOC ();
> -      struct cgraph_node *orig_node;
> -      for (orig_node = old_node; orig_node->clone_of; orig_node = orig_node->clone_of)
> -        ;
> -      for (arg = DECL_ARGUMENTS (orig_node->decl);
> -	   arg; arg = DECL_CHAIN (arg), oldi++)
> -	{
> -	  if (bitmap_bit_p (old_node->clone.combined_args_to_skip, oldi))
> -	    {
> -	      bitmap_set_bit (new_args_to_skip, oldi);
> -	      continue;
> -	    }
> -	  if (bitmap_bit_p (args_to_skip, newi))
> -	    bitmap_set_bit (new_args_to_skip, oldi);
> -	  newi++;
> -	}
> -      new_node->clone.combined_args_to_skip = new_args_to_skip;
> -    }
> -  else
> -    new_node->clone.combined_args_to_skip = args_to_skip;
> +
>    if (old_node->ipa_transforms_to_apply.exists ())
>      new_node->ipa_transforms_to_apply
>        = old_node->ipa_transforms_to_apply.copy ();
> diff --git a/gcc/testsuite/g++.dg/ipa/pr61160-2.C b/gcc/testsuite/g++.dg/ipa/pr61160-2.C
> new file mode 100644
> index 0000000..1011bd1
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/ipa/pr61160-2.C
> @@ -0,0 +1,43 @@
> +/* { dg-do run } */
> +/* { dg-options "-O3 --param ipa-cp-eval-threshold=1"  } */
> +
> +extern "C" void abort (void);
> +
> +struct CBase {
> +  virtual void BaseFunc () {}
> +};
> +
> +struct MMixin {
> +  virtual void * MixinFunc (int, void *) = 0;
> +};
> +
> +struct CExample: CBase, public MMixin
> +{
> +  int stuff, magic, more_stuff;
> +
> +  CExample ()
> +  {
> +    stuff = 0;
> +    magic = 0xbeef;
> +    more_stuff = 0;
> +  }
> +  void *MixinFunc (int arg, void *arg2)
> +  {
> +    if (arg != 1 || arg2)
> +      return 0;
> +    if (magic != 0xbeef)
> +      abort();
> +    return this;
> +  }
> +};
> +
> +void *test (MMixin & anExample)
> +{
> +  return anExample.MixinFunc (1, 0);
> +}
> +
> +int main ()
> +{
> +  CExample c;
> +  return (test (c) != &c);
> +}
> diff --git a/gcc/testsuite/g++.dg/ipa/pr61160-3.C b/gcc/testsuite/g++.dg/ipa/pr61160-3.C
> new file mode 100644
> index 0000000..8184ec2
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/ipa/pr61160-3.C
> @@ -0,0 +1,37 @@
> +/* { dg-do run } */
> +/* { dg-options "-O3"  } */
> +
> +struct A {
> +  void *p;
> +  A (void *q) : p (q) {}
> +  A (const A &) : p () {}
> +};
> +
> +struct CBase {
> +  virtual void BaseFunc () {}
> +};
> +
> +struct MMixin {
> +  virtual A MixinFunc (int, A) = 0;
> +};
> +
> +struct CExample: CBase, public MMixin
> +{
> +  A MixinFunc (int arg, A arg2)
> +  {
> +    if (arg != 1 || arg2.p)
> +      return 0;
> +    return this;
> +  }
> +};
> +
> +void *test (MMixin & anExample)
> +{
> +  return anExample.MixinFunc (1, (0)).p;
> +}
> +
> +int main ()
> +{
> +  CExample c;
> +  return (test (c) != &c);
> +}