On 5/16/19 2:52 PM, Richard Biener wrote:
> On Thu, May 16, 2019 at 1:53 PM Martin Liška <mli...@suse.cz> wrote:
>>
>> On 5/16/19 1:42 PM, Richard Biener wrote:
>>> On Thu, May 16, 2019 at 1:38 PM Martin Liška <mli...@suse.cz> wrote:
>>>>
>>>> On 5/16/19 1:24 PM, Richard Biener wrote:
>>>>> On Thu, May 16, 2019 at 1:18 PM Martin Liška <mli...@suse.cz> wrote:
>>>>>>
>>>>>> Hi.
>>>>>>
>>>>>> We should not allow target_clones being combined with alias attribute.
>>>>>>
>>>>>> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
>>>>>>
>>>>>> Ready to be installed?
>>>>>
>>>>> So that's because an alias cannot be turned into a dispatcher and still
>>>>> be an alias, correct? So a way around this would be to turn the
>>>>> alias into the dispatcher and clone the alias target, leaving the
>>>>> plain alias target as default variant not going through the dispatcher?
>>>>
>>>> We do allow having an alias to a target clone symbol:
>>>>
>>>> __attribute__((target_clones("arch=haswell", "default"))) int __tanh() {}
>>>> __typeof(__tanh) tanhf64 __attribute__((alias("__tanh")));
>>>>
>>>> Having that tanhf64 points to the resolver, which I believe is correct.
>>>
>>> In this case yes. I think the case in the PR wants to have an alias
>>> to the default variant instead and that's not possible so it tries to
>>> do the cloning on an alias (basically tell cloning to use an alternate
>>> symbol name for the resolver, leaving the default in place). IMHO
>>> a reasonable feature, not sure if a reasonable way to achieve.
>>
>> I see. Agree with you that it can be handy. On the other hand, one can use
>> target
>> attribute:
>>
>> __attribute__((target("avx","arch=core-avx2")))
>> int
>> bar ()
>> {
>> return 2;
>> }
>>
>> __attribute__((target("default")))
>> int
>> bar ()
>> {
>> return 2;
>> }
>>
>> int barrr () __attribute__((alias("_Z3barv")));
>>
>> Which directly identifies a concrete implementation.
>
> Hmm. You reference the dispatcher here via barrr?
> Who knows the mangling of the default version?
Yep, that's a bit cumbersome, but we've been using that for quite some time :)
>
> I guess having
>
> __attribute__((target("avx" (bar_avx),"default" (bar_default))))
> int bar() { ... }
>
> would be nice, creating external symbols for the individual clones?
>
> But maybe
>
> int bar() { ... }
> __attribute__((target("avx"))) int bar();
> __attribute__((target("default"), alias("bar_default"))) int bar();
>
> could already work for this.
I prefer the later one. I'll maybe implement that in the future.
>
> Meanwhile your patch is OK I think.
Thanks,
Martin
>
> Richard.
>
>>
>> Martin
>>
>>>
>>>> The PR is about an alias that has itself target_clone attribute, which
>>>> does not make sense.
>>>>
>>>>>
>>>>> Of course in the testcase the body of the alias target isn't available
>>>>> but that's a general issue, not special to aliases?
>>>>
>>>> We do the target_clone expansion just for node->definition which is true
>>>> for node->alias == true symbols.
>>>
>>> I see. I guess it should be done for node->analyzed only, but yes,
>>> w/o considering aliases or thunks all definitions have bodies.
>>>
>>> Richard.
>>>
>>>> Martin
>>>>
>>>>>
>>>>> Richard.
>>>>>
>>>>>> Thanks,
>>>>>> Martin
>>>>>>
>>>>>> gcc/ChangeLog:
>>>>>>
>>>>>> 2019-05-16 Martin Liska <mli...@suse.cz>
>>>>>>
>>>>>> PR lto/90500
>>>>>> * multiple_target.c (expand_target_clones): Do not allow
>>>>>> target_clones being used with a symbol that is an alias.
>>>>>>
>>>>>> gcc/testsuite/ChangeLog:
>>>>>>
>>>>>> 2019-05-16 Martin Liska <mli...@suse.cz>
>>>>>>
>>>>>> PR lto/90500
>>>>>> * gcc.target/i386/pr90500-1.c: New test.
>>>>>> * gcc.target/i386/pr90500-2.c: New test.
>>>>>> ---
>>>>>> gcc/multiple_target.c | 5 ++++-
>>>>>> gcc/testsuite/gcc.target/i386/pr90500-1.c | 8 ++++++++
>>>>>> gcc/testsuite/gcc.target/i386/pr90500-2.c | 7 +++++++
>>>>>> 3 files changed, 19 insertions(+), 1 deletion(-)
>>>>>> create mode 100644 gcc/testsuite/gcc.target/i386/pr90500-1.c
>>>>>> create mode 100644 gcc/testsuite/gcc.target/i386/pr90500-2.c
>>>>>>
>>>>>>
>>>>
>>
