Do not allow target_clones with alias attr (PR lto/90500).

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).
Related show

Commit Message

Martin Liška May 16, 2019, 11:18 a.m.
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?
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

Comments

Richard Biener May 16, 2019, 11:24 a.m. | #1
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

>

>
Martin Liška May 16, 2019, 11:38 a.m. | #2
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

>>

>>
Richard Biener May 16, 2019, 11:42 a.m. | #3
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

> >>

> >>

>
Martin Liška May 16, 2019, 11:53 a.m. | #4
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

>>>>

>>>>

>>
Richard Biener May 16, 2019, 12:52 p.m. | #5
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

> >>>>

> >>>>

> >>

>
Martin Liška May 16, 2019, 1:08 p.m. | #6
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

>>>>>>

>>>>>>

>>>>

>>
Martin Liška May 17, 2019, 9:01 a.m. | #7
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

Patch

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 } */