[2/2,PR,63814] Do not re-create expanded artificial thunks

Hi,

when debugging PR 63814 I noticed that when cgraph_node::create_clone
was using redirect_edge_duplicating_thunks to redirect two edges to a
thunk of a clone, two thunks were created, one for each edge.  The
reason is that even though duplicate_thunk_for_node attempts to locate
an already created thunk, it does so by looking for a caller with
thunk.thunk_p set and the previously created one does not have it set
because (on i686) expand_thunk has expanded the thunk to gimple and
cleared the flag.

This patch fixes the issue by marking such expanded thunks with yet
another flag and then uses the flag to identify such expanded thunks.
Bootstrapped and tested on x86_64-linux and i686-linux.  Honza, do you
think this is a good approach?  Is the patch OK for trunk?

Thanks,

Martin


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

	* cgraph.h (cgraph_thunk_info): Converted thunk_p to a bit-field.
	Added new flag expanded_thunk_p.
	* cgraphunit.c (expand_thunk): Set expanded_thunk_p when appropriate.
	* cgraphclones.c (duplicate_thunk_for_node): Also re-use an expanded
	thunk if available.

Ping.

Thx,

Martin


On Fri, Nov 21, 2014 at 08:18:12PM +0100, Martin Jambor wrote:
> Hi,
> 
> when debugging PR 63814 I noticed that when cgraph_node::create_clone
> was using redirect_edge_duplicating_thunks to redirect two edges to a
> thunk of a clone, two thunks were created, one for each edge.  The
> reason is that even though duplicate_thunk_for_node attempts to locate
> an already created thunk, it does so by looking for a caller with
> thunk.thunk_p set and the previously created one does not have it set
> because (on i686) expand_thunk has expanded the thunk to gimple and
> cleared the flag.
> 
> This patch fixes the issue by marking such expanded thunks with yet
> another flag and then uses the flag to identify such expanded thunks.
> Bootstrapped and tested on x86_64-linux and i686-linux.  Honza, do you
> think this is a good approach?  Is the patch OK for trunk?
> 
> Thanks,
> 
> Martin
> 
> 
> 2014-11-21  Martin Jambor  <mjambor@suse.cz>
> 
> 	* cgraph.h (cgraph_thunk_info): Converted thunk_p to a bit-field.
> 	Added new flag expanded_thunk_p.
> 	* cgraphunit.c (expand_thunk): Set expanded_thunk_p when appropriate.
> 	* cgraphclones.c (duplicate_thunk_for_node): Also re-use an expanded
> 	thunk if available.
> 
> Index: src/gcc/cgraph.h
> ===================================================================
> --- src.orig/gcc/cgraph.h
> +++ src/gcc/cgraph.h
> @@ -552,7 +552,9 @@ struct GTY(()) cgraph_thunk_info {
>    bool virtual_offset_p;
>    bool add_pointer_bounds_args;
>    /* Set to true when alias node is thunk.  */
> -  bool thunk_p;
> +  unsigned thunk_p : 1;
> +  /* Set when this is an already expanded thunk.  */
> +  unsigned expanded_thunk_p : 1;
>  };
>  
>  /* Information about the function collected locally.
> Index: src/gcc/cgraphclones.c
> ===================================================================
> --- src.orig/gcc/cgraphclones.c
> +++ src/gcc/cgraphclones.c
> @@ -311,7 +311,7 @@ duplicate_thunk_for_node (cgraph_node *t
>  
>    cgraph_edge *cs;
>    for (cs = node->callers; cs; cs = cs->next_caller)
> -    if (cs->caller->thunk.thunk_p
> +    if ((cs->caller->thunk.thunk_p || cs->caller->thunk.expanded_thunk_p)
>  	&& cs->caller->thunk.this_adjusting == thunk->thunk.this_adjusting
>  	&& cs->caller->thunk.fixed_offset == thunk->thunk.fixed_offset
>  	&& cs->caller->thunk.virtual_offset_p == thunk->thunk.virtual_offset_p
> Index: src/gcc/cgraphunit.c
> ===================================================================
> --- src.orig/gcc/cgraphunit.c
> +++ src/gcc/cgraphunit.c
> @@ -1504,6 +1504,7 @@ cgraph_node::expand_thunk (bool output_a
>        set_cfun (NULL);
>        TREE_ASM_WRITTEN (thunk_fndecl) = 1;
>        thunk.thunk_p = false;
> +      thunk.expanded_thunk_p = true;
>        analyzed = false;
>      }
>    else
> @@ -1686,6 +1687,7 @@ cgraph_node::expand_thunk (bool output_a
>        /* Since we want to emit the thunk, we explicitly mark its name as
>  	 referenced.  */
>        thunk.thunk_p = false;
> +      thunk.expanded_thunk_p = true;
>        lowered = true;
>        bitmap_obstack_release (NULL);
>      }

> On Fri, Nov 21, 2014 at 08:18:12PM +0100, Martin Jambor wrote:
> > Hi,
> > 
> > when debugging PR 63814 I noticed that when cgraph_node::create_clone
> > was using redirect_edge_duplicating_thunks to redirect two edges to a
> > thunk of a clone, two thunks were created, one for each edge.  The
> > reason is that even though duplicate_thunk_for_node attempts to locate
> > an already created thunk, it does so by looking for a caller with
> > thunk.thunk_p set and the previously created one does not have it set
> > because (on i686) expand_thunk has expanded the thunk to gimple and
> > cleared the flag.
> > 
> > This patch fixes the issue by marking such expanded thunks with yet
> > another flag and then uses the flag to identify such expanded thunks.
> > Bootstrapped and tested on x86_64-linux and i686-linux.  Honza, do you
> > think this is a good approach?  Is the patch OK for trunk?

Hmm, I was sort of hoping to drop the whole .thunk structure into summary and
do not keep it for functions with Gimple body, so this change will imply the
need to keep this datastructure for thunks that are already expanded.
moreover we do not stream the flag, so not all expanded thunks are handled,
only those that was introduced during WPA.

It may be cleaner to simply donot expand thunks proactively before all
the clonning is finished?

Honza
> > 
> > Thanks,
> > 
> > Martin
> > 
> > 
> > 2014-11-21  Martin Jambor  <mjambor@suse.cz>
> > 
> > 	* cgraph.h (cgraph_thunk_info): Converted thunk_p to a bit-field.
> > 	Added new flag expanded_thunk_p.
> > 	* cgraphunit.c (expand_thunk): Set expanded_thunk_p when appropriate.
> > 	* cgraphclones.c (duplicate_thunk_for_node): Also re-use an expanded
> > 	thunk if available.
> > 
> > Index: src/gcc/cgraph.h
> > ===================================================================
> > --- src.orig/gcc/cgraph.h
> > +++ src/gcc/cgraph.h
> > @@ -552,7 +552,9 @@ struct GTY(()) cgraph_thunk_info {
> >    bool virtual_offset_p;
> >    bool add_pointer_bounds_args;
> >    /* Set to true when alias node is thunk.  */
> > -  bool thunk_p;
> > +  unsigned thunk_p : 1;
> > +  /* Set when this is an already expanded thunk.  */
> > +  unsigned expanded_thunk_p : 1;
> >  };
> >  
> >  /* Information about the function collected locally.
> > Index: src/gcc/cgraphclones.c
> > ===================================================================
> > --- src.orig/gcc/cgraphclones.c
> > +++ src/gcc/cgraphclones.c
> > @@ -311,7 +311,7 @@ duplicate_thunk_for_node (cgraph_node *t
> >  
> >    cgraph_edge *cs;
> >    for (cs = node->callers; cs; cs = cs->next_caller)
> > -    if (cs->caller->thunk.thunk_p
> > +    if ((cs->caller->thunk.thunk_p || cs->caller->thunk.expanded_thunk_p)
> >  	&& cs->caller->thunk.this_adjusting == thunk->thunk.this_adjusting
> >  	&& cs->caller->thunk.fixed_offset == thunk->thunk.fixed_offset
> >  	&& cs->caller->thunk.virtual_offset_p == thunk->thunk.virtual_offset_p
> > Index: src/gcc/cgraphunit.c
> > ===================================================================
> > --- src.orig/gcc/cgraphunit.c
> > +++ src/gcc/cgraphunit.c
> > @@ -1504,6 +1504,7 @@ cgraph_node::expand_thunk (bool output_a
> >        set_cfun (NULL);
> >        TREE_ASM_WRITTEN (thunk_fndecl) = 1;
> >        thunk.thunk_p = false;
> > +      thunk.expanded_thunk_p = true;
> >        analyzed = false;
> >      }
> >    else
> > @@ -1686,6 +1687,7 @@ cgraph_node::expand_thunk (bool output_a
> >        /* Since we want to emit the thunk, we explicitly mark its name as
> >  	 referenced.  */
> >        thunk.thunk_p = false;
> > +      thunk.expanded_thunk_p = true;
> >        lowered = true;
> >        bitmap_obstack_release (NULL);
> >      }