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?
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.
Meanwhile your patch is OK I think.
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
> >>>>
> >>>>
> >>
>
