| Message ID | e6a74700-b9e6-154d-0a95-feb15ed61b2f@suse.cz |
|---|---|
| State | New |
| Headers | show |
| Series | Do not allow target_clones with alias attr (PR lto/90500). | expand |
On Thu, May 16, 2019 at 1:18 PM Martin Liška <mliska@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? Of course in the testcase the body of the alias target isn't available but that's a general issue, not special to aliases? Richard. > Thanks, > Martin > > gcc/ChangeLog: > > 2019-05-16 Martin Liska <mliska@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 <mliska@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 > >
On 5/16/19 1:24 PM, Richard Biener wrote: > On Thu, May 16, 2019 at 1:18 PM Martin Liška <mliska@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. 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. Martin > > Richard. > >> Thanks, >> Martin >> >> gcc/ChangeLog: >> >> 2019-05-16 Martin Liska <mliska@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 <mliska@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 >> >>
On Thu, May 16, 2019 at 1:38 PM Martin Liška <mliska@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 <mliska@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. > 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 <mliska@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 <mliska@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 > >> > >> >
On 5/16/19 1:42 PM, Richard Biener wrote: > On Thu, May 16, 2019 at 1:38 PM Martin Liška <mliska@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 <mliska@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. 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 <mliska@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 <mliska@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 >>>> >>>> >>
On Thu, May 16, 2019 at 1:53 PM Martin Liška <mliska@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 <mliska@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 <mliska@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 <mliska@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 <mliska@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 > >>>> > >>>> > >> >
On 5/16/19 2:52 PM, Richard Biener wrote: > On Thu, May 16, 2019 at 1:53 PM Martin Liška <mliska@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 <mliska@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 <mliska@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 <mliska@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 <mliska@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 >>>>>> >>>>>> >>>> >>
On 5/16/19 1:42 PM, Richard Biener wrote: > IMHO > a reasonable feature, not sure if a reasonable way to achieve. Just for the record. The use case was not about having an alias to a default clone. They just want to test multi-versioning of some glibc math routines. Martin
diff --git a/gcc/multiple_target.c b/gcc/multiple_target.c index 0a87241b251..fa194d416fe 100644 --- a/gcc/multiple_target.c +++ b/gcc/multiple_target.c @@ -356,7 +356,7 @@ expand_target_clones (struct cgraph_node *node, bool definition) } if (node->definition - && !tree_versionable_function_p (node->decl)) + && (node->alias || !tree_versionable_function_p (node->decl))) { auto_diagnostic_group d; error_at (DECL_SOURCE_LOCATION (node->decl), @@ -365,6 +365,9 @@ expand_target_clones (struct cgraph_node *node, bool definition) if (lookup_attribute ("noclone", DECL_ATTRIBUTES (node->decl))) reason = G_("function %q+F can never be copied " "because it has %<noclone%> attribute"); + else if (node->alias) + reason + = "%<target_clones%> cannot be combined with %<alias%> attribute"; else reason = copy_forbidden (DECL_STRUCT_FUNCTION (node->decl)); if (reason) diff --git a/gcc/testsuite/gcc.target/i386/pr90500-1.c b/gcc/testsuite/gcc.target/i386/pr90500-1.c new file mode 100644 index 00000000000..7ac6a739c05 --- /dev/null +++ b/gcc/testsuite/gcc.target/i386/pr90500-1.c @@ -0,0 +1,8 @@ +/* PR middle-end/84723 */ +/* { dg-do compile } */ +/* { dg-require-ifunc } */ + +__attribute__((target_clones("arch=haswell", "default"))) int __tanh() {} +__typeof(__tanh) tanhf64 __attribute__((alias("__tanh")))/* { dg-error "clones for .target_clones. attribute cannot be created" } */ + /* { dg-message "'target_clones' cannot be combined with 'alias' attribute" "" { target *-*-* } .-1 } */ +__attribute__((__copy__(__tanh))); diff --git a/gcc/testsuite/gcc.target/i386/pr90500-2.c b/gcc/testsuite/gcc.target/i386/pr90500-2.c new file mode 100644 index 00000000000..0fafb8adb21 --- /dev/null +++ b/gcc/testsuite/gcc.target/i386/pr90500-2.c @@ -0,0 +1,7 @@ +/* PR middle-end/84723 */ +/* { dg-do compile } */ +/* { dg-require-ifunc } */ + +__attribute__((target_clones("arch=haswell", "default"))) int __tanh() {} +__typeof(__tanh) tanhf64 __attribute__((alias("__tanh"),target_clones("arch=haswell", "default"))); /* { dg-error "clones for .target_clones. attribute cannot be created" } */ + /* { dg-message "'target_clones' cannot be combined with 'alias' attribute" "" { target *-*-* } .-1 } */