Re: [PATCH] New attribute to create target clones

[Jeff Law <law at redhat dot com>]

On 10/12/2015 05:35 PM, Evgeny Stupachenko wrote:
> Hi All,
>
> Here is a new version of patch (attached).
> Bootstrap and make check are in progress (all new tests passed).
>
> New test case g++.dg/ext/mvc4.C fails with ICE, when options lower
> than "-mavx" are passed.
> However it has the same behavior if "target_clones" attribute is
> replaced by 2 corresponding "target" attributes.
> I've filed PR67946 on this:
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67946
>
> Thanks,
> Evgeny
>
> ChangeLog:
>
> 2015-10-13  Evgeny Stupachenko<evstupac@gmail.com>
> gcc/
>          * Makefile.in (OBJS): Add multiple_target.o.
>          * attrib.c (make_attribute): Moved from config/i386/i386.c
>          * config/i386/i386.c (make_attribute): Deleted.
>          * multiple_target.c (make_attribute): New.
>          (create_dispatcher_calls): Ditto.
>          (get_attr_len): Ditto.
>          (get_attr_str): Ditto.
>          (is_valid_asm_symbol): Ditto.
>          (create_new_asm_name): Ditto.
>          (create_target_clone): Ditto.
>          (expand_target_clones): Ditto.
>          (ipa_target_clone): Ditto.
>          (ipa_dispatcher_calls): Ditto.
>          * passes.def (pass_target_clone): Two new ipa passes.
>          * tree-pass.h (make_pass_target_clone): Ditto.
>
> gcc/c-family
>          * c-common.c (handle_target_clones_attribute): New.
>          * (c_common_attribute_table): Add handle_target_clones_attribute.
>          * (handle_always_inline_attribute): Add check on target_clones
>          attribute.
>          * (handle_target_attribute): Ditto.
>
> gcc/testsuite
>          * gcc.dg/mvc1.c: New test for multiple targets cloning.
>          * gcc.dg/mvc2.c: Ditto.
>          * gcc.dg/mvc3.c: Ditto.
>          * gcc.dg/mvc4.c: Ditto.
>          * gcc.dg/mvc5.c: Ditto.
>          * gcc.dg/mvc6.c: Ditto.
>          * gcc.dg/mvc7.c: Ditto.
>          * g++.dg/ext/mvc1.C: Ditto.
>          * g++.dg/ext/mvc2.C: Ditto.
>          * g++.dg/ext/mvc3.C: Ditto.
>          * g++.dg/ext/mvc4.C: Ditto.
>
> gcc/doc
>          * doc/extend.texi (target_clones): New attribute description.

> diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
> index 23e6a76..f9d28d1 100644
> --- a/gcc/doc/extend.texi
> +++ b/gcc/doc/extend.texi
> @@ -3066,6 +3066,19 @@ This function attribute make a stack protection of the function if
>   flags @option{fstack-protector} or @option{fstack-protector-strong}
>   or @option{fstack-protector-explicit} are set.
>
> +@item target_clones (@var{options})
> +@cindex @code{target_clones} function attribute
> +The @code{target_clones} attribute is used to specify that a function is to
> +be cloned into multiple versions compiled with different target options
> +than specified on the command line.  The supported options and restrictions
> +are the same as for @code{target}.
"as for @code{target}" -> "as for the @code{target} attribute."

I think that makes it a tiny bit clearer.




> +
> +/*  Creates target clone of NODE.  */
> +
> +static cgraph_node *
> +create_target_clone (cgraph_node *node, bool definition)
> +{
> +  cgraph_node *new_node;
> +  if (definition)
> +    {
> +      new_node = node->create_version_clone_with_body (vNULL, NULL,
> +    						       NULL, false,
> +						       NULL, NULL,
> +						       "target_clone");
> +      new_node->externally_visible = node->externally_visible;
> +      new_node->address_taken = node->address_taken;
> +      new_node->thunk = node->thunk;
> +      new_node->alias = node->alias;
> +      new_node->weakref = node->weakref;
> +      new_node->cpp_implicit_alias = node->cpp_implicit_alias;
> +      new_node->local.local = node->local.local;
So do we need to explicitly clear TREE_PUBLIC here?  It also seems like 
copying externally_visible, address_taken and possibly some of those 
other fields is wrong.  The clone is going to be local to the CU, it 
doesn't inherit those properties from the original -- only the 
dispatcher needs to inherit those properties, right?

> +
> +
> +  for (i = 0; i < attrnum; i++)
> +    {
> +      char *attr = attrs[i];
> +      cgraph_node *new_node = create_target_clone (node, defenition);
> +      char *new_asm_name =
> +	  XNEWVEC (char, strlen (old_asm_name) + strlen (attr) + 2);
> +      create_new_asm_name (old_asm_name, attr, new_asm_name);
I thought after discussions with Jan that this wasn't going to be 
necessary as cloning should create a suitable name for us?


Jeff