On Mon, Oct 26, 2015 at 6:50 PM, Jeff Law <law@redhat.com> wrote:
> 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?
Right. That was just the way to keep the node, that I didn't clean up.
Replaced with "new_node->force_output = true;"

>
>> +
>> +
>> +  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?
Yes. This is not necessary. However that way we'll have the following
code in dispatcher:
        cmpl    $6, __cpu_model+4(%rip)
        sete    %al
        movzbl  %al, %eax
        testl   %eax, %eax
        jle     .L16
        movl    $foo.target_clone.1, %eax
I think it is very hard to read and debug such...

While now we have:

        cmpl    $6, __cpu_model+4(%rip)
        sete    %al
        movzbl  %al, %eax
        testl   %eax, %eax
        jle     .L16
        movl    $foo.arch_slm, %eax

and it is clear that we are jumping to SLM code here.
I'd like to keep target in names.

Thanks,
Evgeny
>
>
> Jeff