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

[Diego Novillo <dnovillo@google.com>]

On 2012-10-12 18:19 , Sriraman Tallam wrote:

> When the front-end sees more than one decl for "foo", it calls a target hook to
> determine if they are versions. To prevent duplicate definition errors with other
>  versions of "foo", "decls_match" function in cp/decl.c is made to return false
>  when 2 decls have are deemed versions by the target. This will make all function
> versions of "foo" to be added to the overload list of "foo".

So, this means that this can only work for C++, right?  Or could the 
same trickery be done some other way in other FEs?

I see no handling of different FEs.  If the user tries to use these 
attributes from languages other than C++, we should emit a diagnostic.

> +@deftypefn {Target Hook} tree TARGET_GET_FUNCTION_VERSIONS_DISPATCHER (void *@var{arglist})
> +This hook is used to get the dispatcher function for a set of function
> +versions.  The dispatcher function is called to invoke the rignt function

s/rignt/right/

> +version at run-time. @var{arglist} is the vector of function versions
> +that should be considered for dispatch.
> +@end deftypefn
> +
> +@deftypefn {Target Hook} tree TARGET_GENERATE_VERSION_DISPATCHER_BODY (void *@var{arg})
> +This hook is used to generate the dispatcher logic to invoke the right
> +function version at runtime for a given set of function versions.

s/runtime/run-time/

> +@hook TARGET_GET_FUNCTION_VERSIONS_DISPATCHER
> +This hook is used to get the dispatcher function for a set of function
> +versions.  The dispatcher function is called to invoke the rignt function

s/rignt/right/

> +version at run-time. @var{arglist} is the vector of function versions
> +that should be considered for dispatch.
> +@end deftypefn
> +
> +@hook TARGET_GENERATE_VERSION_DISPATCHER_BODY
> +This hook is used to generate the dispatcher logic to invoke the right
> +function version at runtime for a given set of function versions.

s/runtime/run-time/

> @@ -288,7 +289,6 @@ mark_store (gimple stmt, tree t, void *data)
>       }
>    return false;
>  }
> -
>  /* Create cgraph edges for function calls.
>     Also look for functions and variables having addresses taken.  */

Don't remove vertical white space, please.

> +		{
> +		  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.next);

What if callee is the last version in the list?  Not sure what you are 
trying to check here.


> @@ -8601,9 +8601,22 @@ handle_target_attribute (tree *node, tree name, tr
>        warning (OPT_Wattributes, "%qE attribute ignored", name);
>        *no_add_attrs = true;
>      }
> -  else if (! targetm.target_option.valid_attribute_p (*node, name, args,
> -						      flags))
> -    *no_add_attrs = true;
> +  else
> +    {
> +      /* When a target attribute is invalid, it may also be because the
> +	 target for the compilation unit and the attribute match.  For
> +         instance, target attribute "xxx" is invalid when -mxxx is used.
> +         When used with multiversioning, removing the attribute will lead
> +         to duplicate definitions if a default version is provided.
> +	 So, generate a warning here and remove the attribute.  */
> +      if (!targetm.target_option.valid_attribute_p (*node, name, args, flags))
> +	{
> +	  warning (OPT_Wattributes,
> +		   "Invalid target attribute in function %qE, ignored.",
> +		   *node);
> +	  *no_add_attrs = true;

If you do this, isn't the compiler going to generate two warning 
messages?  One for the invalid target attribute, the second for the 
duplicate definition.

> @@ -228,6 +228,26 @@ struct GTY(()) cgraph_node {
>    struct cgraph_node *prev_sibling_clone;
>    struct cgraph_node *clones;
>    struct cgraph_node *clone_of;
> +
> +  /* Function Multiversioning info.  */
> +  struct {
> +    /* Chains all the semantically identical function versions.  The
> +       first function in this chain is the default function.  */
> +    struct cgraph_node *prev;
> +    /* If this node is a dispatcher for function versions, this points
> +       to the default function version, the first function in the chain.  */
> +    struct cgraph_node *next;

Why not a VEC of function decls?  Seems easier to manage and less size 
overhead.


> @@ -3516,8 +3522,8 @@ struct GTY(()) tree_function_decl {
>    unsigned looping_const_or_pure_flag : 1;
>    unsigned has_debug_args_flag : 1;
>    unsigned tm_clone_flag : 1;
> -
> -  /* 1 bit left */
> +  unsigned versioned_function : 1;
> +  /* No bits left.  */

You ate the last bit!  How rude ;)

> @@ -8132,6 +8176,38 @@ joust (struct z_candidate *cand1, struct z_candida
>        && (IS_TYPE_OR_DECL_P (cand1->fn)))
>      return 1;
>
> +  /* For Candidates of a multi-versioned function,  make the version with

s/Candidates/candidates/

> +  old_current_function_decl = current_function_decl;
> +  push_cfun (DECL_STRUCT_FUNCTION (function_decl));
> +  current_function_decl = function_decl;

push_cfun will set current_function_decl for you.  No need to keep track 
of old_current_function_decl.

> +  enum feature_priority
> +  {
> +    P_ZERO = 0,
> +    P_MMX,
> +    P_SSE,
> +    P_SSE2,
> +    P_SSE3,
> +    P_SSSE3,
> +    P_PROC_SSSE3,
> +    P_SSE4_a,
> +    P_PROC_SSE4_a,
> +    P_SSE4_1,
> +    P_SSE4_2,
> +    P_PROC_SSE4_2,
> +    P_POPCNT,
> +    P_AVX,
> +    P_AVX2,
> +    P_FMA,
> +    P_PROC_FMA
> +  };

There's no need to have this list dynamically defined, right?

> +	}
> +    }
> +
> +  /* Process feature name.  */
> +  tok_str =  (char *) xmalloc (strlen (attrs_str) + 1);

XNEWVEC(char, strlen (attrs_str) + 1);

> +  /* Atleast one more version other than the default.  */

s/Atleast/At least/

> +  num_versions = VEC_length (tree, fndecls);
> +  gcc_assert (num_versions >= 2);
> +
> +  function_version_info = (struct _function_version_info *)
> +    xmalloc ((num_versions - 1) * sizeof (struct _function_version_info));

Better use VEC() here.

> +
> +  /* The first version in the vector is the default decl.  */
> +  default_decl = VEC_index (tree, fndecls, 0);
> +
> +  old_current_function_decl = current_function_decl;
> +  push_cfun (DECL_STRUCT_FUNCTION (dispatch_decl));
> +  current_function_decl = dispatch_decl;

No need to set current_function_decl.

> +
> +  gseq = bb_seq (*empty_bb);
> +  /* Function version dispatch is via IFUNC.  IFUNC resolvers fire before
> +     constructors, so explicity call __builtin_cpu_init here.  */
> +  ifunc_cpu_init_stmt = gimple_build_call_vec (
> +                     ix86_builtins [(int) IX86_BUILTIN_CPU_INIT], NULL);
> +  gimple_seq_add_stmt (&gseq, ifunc_cpu_init_stmt);
> +  gimple_set_bb (ifunc_cpu_init_stmt, *empty_bb);
> +  set_bb_seq (*empty_bb, gseq);
> +
> +  pop_cfun ();
> +  current_function_decl = old_current_function_decl;

Likewise here.

> +/* This function returns true if fn1 and fn2 are versions of the same function.
> +   Returns false if only one of the function decls has the target attribute
> +   set or if the targets of the function decls are different.  This assumes
> +   the fn1 and fn2 have the same signature.  */

Mention the arguments in capitals.

> +  for (i = 0; i < strlen (str); i++)
> +    if (str[i] == ',')
> +      argnum++;
> +
> +  attr_str = (char *)xmalloc (strlen (str) + 1);

XNEWVEC()

> +  strcpy (attr_str, str);
> +
> +  /* Replace "=,-" with "_".  */
> +  for (i = 0; i < strlen (attr_str); i++)
> +    if (attr_str[i] == '=' || attr_str[i]== '-')
> +      attr_str[i] = '_';
> +
> +  if (argnum == 1)
> +    return attr_str;
> +
> +  args = (char **)xmalloc (argnum * sizeof (char *));

VEC()?

> +  if (DECL_DECLARED_INLINE_P (decl)
> +      && lookup_attribute ("gnu_inline",
> +			   DECL_ATTRIBUTES (decl)))
> +    error_at (DECL_SOURCE_LOCATION (decl),
> +	      "Function versions cannot be marked as gnu_inline,"
> +	      " bodies have to be generated\n");

No newline at the end of the error message.

> +  sprintf (assembler_name, "%s.%s", orig_name, attr_str);
> +  if (dump_file)
> +    fprintf (stderr, "Assembler name set to %s for function version %s\n",
> +	     assembler_name, IDENTIFIER_POINTER (id));

This dumps to stderr instead of dump_file.  Also, use the new dumping 
facility?

> +/* Return a new name by appending SUFFIX to the DECL name.  If
> +   make_unique is true, append the full path name.  */

Full path name of what?

> +
> +static char *
> +make_name (tree decl, const char *suffix, bool make_unique)
> +{
> +  char *global_var_name;
> +  int name_len;
> +  const char *name;
> +  const char *unique_name = NULL;
> +
> +  name = IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (decl));
> +
> +  /* Get a unique name that can be used globally without any chances
> +     of collision at link time.  */
> +  if (make_unique)
> +    unique_name = IDENTIFIER_POINTER (get_file_function_name ("\0"));
> +
> +  name_len = strlen (name) + strlen (suffix) + 2;
> +
> +  if (make_unique)
> +    name_len += strlen (unique_name) + 1;
> +  global_var_name = (char *) xmalloc (name_len);

XNEWVEC.



Diego.