Re: User directed Function Multiversioning via Function Overloading (issue5752064)

[Jan Hubicka <hubicka@ucw.cz>]

Hi,
sorry for jumping in late, for too long I did not had chnce to look at my TODO.
I have two comments...
> Index: gcc/cgraphbuild.c
> ===================================================================
> --- gcc/cgraphbuild.c	(revision 192623)
> +++ gcc/cgraphbuild.c	(working copy)
> @@ -34,6 +34,7 @@ along with GCC; see the file COPYING3.  If not see
>  #include "ipa-utils.h"
>  #include "except.h"
>  #include "ipa-inline.h"
> +#include "target.h"
>  
>  /* Context of record_reference.  */
>  struct record_reference_ctx
> @@ -317,8 +318,23 @@ build_cgraph_edges (void)
>  							 bb);
>  	      decl = gimple_call_fndecl (stmt);
>  	      if (decl)
> -		cgraph_create_edge (node, cgraph_get_create_node (decl),
> -				    stmt, bb->count, freq);
> +		{
> +		  struct cgraph_node *callee = cgraph_get_create_node (decl);
> +	          /* If a call to a multiversioned function dispatcher is
> +		     found, generate the body to dispatch the right function
> +		     at run-time.  */
> +		  if (callee->dispatcher_function)
> +		    {
> +		      tree resolver_decl;
> +		      gcc_assert (callee->function_version
> +				  && callee->function_version->next);
> +		      gcc_assert (targetm.generate_version_dispatcher_body);
> +		      resolver_decl
> +			 = targetm.generate_version_dispatcher_body (callee);
> +		      gcc_assert (resolver_decl != NULL_TREE);
> +		    }
> +		  cgraph_create_edge (node, callee, stmt, bb->count, freq);
> +	        }
I do not really think resolver generation belongs here + I would preffer
build_cgraph_edges to really just build the edges.
> Index: gcc/cgraph.c
> ===================================================================
> --- gcc/cgraph.c	(revision 192623)
> +++ gcc/cgraph.c	(working copy)
> @@ -1277,6 +1277,16 @@ cgraph_mark_address_taken_node (struct cgraph_node
>    node->symbol.address_taken = 1;
>    node = cgraph_function_or_thunk_node (node, NULL);
>    node->symbol.address_taken = 1;
> +  /* If the address of a multiversioned function dispatcher is taken,
> +     generate the body to dispatch the right function at run-time.  This
> +     is needed as the address can be used to do an indirect call.  */
> +  if (node->dispatcher_function)
> +    {
> +      gcc_assert (node->function_version
> +		  && node->function_version->next);
> +      gcc_assert (targetm.generate_version_dispatcher_body);
> +      targetm.generate_version_dispatcher_body (node);
> +    }

Similarly here.  I also think this way you will miss aliases of the multiversioned
functions.

I am not sure why the multiversioning is tied with the cgraph build and the
datastructure is put into cgraph_node itself.  It seems to me that your
dispatchers are in a way related to thunks - i.e. they are inserted into
callgraph and once they become reachable their body needs to be produced.  I
think generate_version_dispatcher_body should thus probably be done from
cgraph_analyze_function. (to make the function to be seen by analyze_function
you will need to make it to be finalized at the time you set
dispatcher_function flag.

I would also put the dispatcher datastructure into on-side hash by node->uid.
(i.e. these are rare and thus the datastructure should be small)
symbol table is critical for WPA stage memory use and I plan to remove as much
as possible from the nodes in near future. For this reason I would preffer
to not add too much of stuff that is not going to be used by majority of nodes.

Honza