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

[Jason Merrill <jason@redhat.com>]

On 08/24/2012 08:34 PM, Sriraman Tallam wrote:
> +	  /* For function versions, their parms and types match
> +	     but they are not duplicates.  Record function versions
> +	     as and when they are found.  */
> +	  if (TREE_CODE (fn) == FUNCTION_DECL
> +	      && TREE_CODE (method) == FUNCTION_DECL
> +	      && (DECL_FUNCTION_SPECIFIC_TARGET (fn)
> +		  || DECL_FUNCTION_SPECIFIC_TARGET (method))
> +	      && targetm.target_option.function_versions (fn, method))
> + 	    {
> +	      targetm.set_version_assembler_name (fn);
> +	      targetm.set_version_assembler_name (method);
> +	      continue;
> +	    }

This seems like an odd place to be setting assembler names; better to 
just have the existing mangle_decl_assembler_name hook add the 
appropriate suffix when it's called normally.

> +	 Also, mark this function as needed if it is marked inline but
> +	 is a multi-versioned function.  */

Why?  If it's used, it should be marked needed though the normal process.

> +	    error_at (location_of (DECL_NAME (OVL_CURRENT (fn))),
> +		      "Call to multiversioned function %<%D(%A)%> with"
> +		      " no default version", DECL_NAME (OVL_CURRENT (fn)),
> +		      build_tree_list_vec (*args));

location_of just returns input_location if you ask it for the location 
of an identifier, so you might as well use error with no explicit 
location.  And why not print candidates->fn instead of pasting the 
name/args?  Also, lowercase "call".

> +    {
> +      tree dispatcher_decl = NULL;
> +      struct cgraph_node *node = cgraph_get_node (fn);
> +      if (node != NULL)
> +        dispatcher_decl = cgraph_get_node (fn)->version_dispatcher_decl;
> +      if (dispatcher_decl == NULL)
> +	{
> +	  error_at (input_location, "Call to multiversioned function"
> +		    " without a default is not allowed");
> +	  return NULL;
> +	}
> +      retrofit_lang_decl (dispatcher_decl);
> +      gcc_assert (dispatcher_decl != NULL);
> +      fn = dispatcher_decl;
> +    }

Let's move this logic into a separate function that returns the 
dispatcher function.

> +      /* Both functions must be marked versioned.  */
> +      gcc_assert (DECL_FUNCTION_VERSIONED (cand1->fn)
> +		  && DECL_FUNCTION_VERSIONED (cand2->fn));

Why can't you compare a versioned function and a non-versioned one?

The code in joust should go further down in the function, before the 
handling of two declarations of the same function.

> +  /* For multiversioned functions, aggregate all the versions here for
> +     generating the dispatcher body later if necessary.  */
> +
> +  if (TREE_CODE (candidates->fn) == FUNCTION_DECL
> +      && DECL_FUNCTION_VERSIONED (candidates->fn))
> +    {
> +      VEC (tree, heap) *fn_ver_vec = NULL;
> +      struct z_candidate *ver = candidates;
> +      fn_ver_vec = VEC_alloc (tree, heap, 2);
> +      for (;ver; ver = ver->next)
> +        VEC_safe_push (tree, heap, fn_ver_vec, ver->fn);
> +      gcc_assert (targetm.get_function_versions_dispatcher);
> +      targetm.get_function_versions_dispatcher (fn_ver_vec);
> +      VEC_free (tree, heap, fn_ver_vec);
> +    }

This seems to assume that all the functions in the list of candidates 
are versioned, but there might be unrelated functions from different 
namespaces.  Also, doing this every time someone calls a versioned 
function seems like the wrong place; I would think it would be better to 
build up a list of versions as you seed declarations, and then use that 
list to define the dispatcher at EOF if it's needed.

> +      if (TREE_CODE (decl) == FUNCTION_DECL
> +          && DECL_FUNCTION_VERSIONED (decl)
> +	  && DECL_ASSEMBLER_NAME_SET_P (decl))
> +	write_source_name (DECL_ASSEMBLER_NAME (decl));
> +      else
> +	write_source_name (DECL_NAME (decl));

Again, I think it's better to handle the suffix via 
mangle_decl_assembler_name.

Jason