| Message ID | 80b9b4d6-2641-d726-2925-03053b5f231f@suse.cz |
|---|---|
| State | New |
| Headers | show |
| Series | Handle multiple 'default' in target attribute (PR middle-end/89970). | expand |
On Thu, Apr 11, 2019 at 10:37 AM Martin Liška <mliska@suse.cz> wrote: > > Hi. > > The patch is catching duplicate 'default' values in target attribute. > > Patch can bootstrap on x86_64-linux-gnu and survives regression tests. > > Ready to be installed? I wonder if it isn't better to ignore duplicate "default"s (given you needed to adjust a testcase even)? Richard. > Thanks, > Martin > > gcc/ChangeLog: > > 2019-04-10 Martin Liska <mliska@suse.cz> > > PR middle-end/89970 > * multiple_target.c (create_dispatcher_calls): Wrap ifunc > in error message. > (separate_attrs): Handle multiple 'default's. > (expand_target_clones): Rework error handling code. > > gcc/testsuite/ChangeLog: > > 2019-04-10 Martin Liska <mliska@suse.cz> > > PR middle-end/89970 > * gcc.target/i386/mvc15.c: New test. > * gcc.target/i386/mvc3.c: Quote target in error pattern. > * gcc.target/i386/mvc4.c: Remove duplicit 'default'. > --- > gcc/multiple_target.c | 38 ++++++++++++++++++--------- > gcc/testsuite/gcc.target/i386/mvc15.c | 11 ++++++++ > gcc/testsuite/gcc.target/i386/mvc3.c | 2 +- > gcc/testsuite/gcc.target/i386/mvc4.c | 2 +- > 4 files changed, 38 insertions(+), 15 deletions(-) > create mode 100644 gcc/testsuite/gcc.target/i386/mvc15.c > >
On 4/11/19 11:57 AM, Richard Biener wrote: > On Thu, Apr 11, 2019 at 10:37 AM Martin Liška <mliska@suse.cz> wrote: >> >> Hi. >> >> The patch is catching duplicate 'default' values in target attribute. >> >> Patch can bootstrap on x86_64-linux-gnu and survives regression tests. >> >> Ready to be installed? > > I wonder if it isn't better to ignore duplicate "default"s (given you needed to > adjust a testcase even)? Well, possibly yes. But I would prefer to have a more strict checking. Martin > > Richard. > >> Thanks, >> Martin >> >> gcc/ChangeLog: >> >> 2019-04-10 Martin Liska <mliska@suse.cz> >> >> PR middle-end/89970 >> * multiple_target.c (create_dispatcher_calls): Wrap ifunc >> in error message. >> (separate_attrs): Handle multiple 'default's. >> (expand_target_clones): Rework error handling code. >> >> gcc/testsuite/ChangeLog: >> >> 2019-04-10 Martin Liska <mliska@suse.cz> >> >> PR middle-end/89970 >> * gcc.target/i386/mvc15.c: New test. >> * gcc.target/i386/mvc3.c: Quote target in error pattern. >> * gcc.target/i386/mvc4.c: Remove duplicit 'default'. >> --- >> gcc/multiple_target.c | 38 ++++++++++++++++++--------- >> gcc/testsuite/gcc.target/i386/mvc15.c | 11 ++++++++ >> gcc/testsuite/gcc.target/i386/mvc3.c | 2 +- >> gcc/testsuite/gcc.target/i386/mvc4.c | 2 +- >> 4 files changed, 38 insertions(+), 15 deletions(-) >> create mode 100644 gcc/testsuite/gcc.target/i386/mvc15.c >> >>
On Thu, Apr 11, 2019 at 12:44:40PM +0200, Martin Liška wrote: > On 4/11/19 11:57 AM, Richard Biener wrote: > > On Thu, Apr 11, 2019 at 10:37 AM Martin Liška <mliska@suse.cz> wrote: > >> > >> Hi. > >> > >> The patch is catching duplicate 'default' values in target attribute. > >> > >> Patch can bootstrap on x86_64-linux-gnu and survives regression tests. > >> > >> Ready to be installed? > > > > I wonder if it isn't better to ignore duplicate "default"s (given you needed to > > adjust a testcase even)? > > Well, possibly yes. But I would prefer to have a more strict checking. I agree, default, default is not really useful and rejecting it is fine. Having a testcase covering that is of course desirable. Jakub
On Thu, Apr 11, 2019 at 12:47 PM Jakub Jelinek <jakub@redhat.com> wrote: > > On Thu, Apr 11, 2019 at 12:44:40PM +0200, Martin Liška wrote: > > On 4/11/19 11:57 AM, Richard Biener wrote: > > > On Thu, Apr 11, 2019 at 10:37 AM Martin Liška <mliska@suse.cz> wrote: > > >> > > >> Hi. > > >> > > >> The patch is catching duplicate 'default' values in target attribute. > > >> > > >> Patch can bootstrap on x86_64-linux-gnu and survives regression tests. > > >> > > >> Ready to be installed? > > > > > > I wonder if it isn't better to ignore duplicate "default"s (given you needed to > > > adjust a testcase even)? > > > > Well, possibly yes. But I would prefer to have a more strict checking. > > I agree, default, default is not really useful and rejecting it is fine. > Having a testcase covering that is of course desirable. There is one in the patch. That said, I was worried to go down the same route as that volatile asm thing and the patch needs to be on branches as well? Anyhow, patch is OK for trunk. Richard. > Jakub
On Thu, Apr 11, 2019 at 01:10:00PM +0200, Richard Biener wrote: > On Thu, Apr 11, 2019 at 12:47 PM Jakub Jelinek <jakub@redhat.com> wrote: > > > > On Thu, Apr 11, 2019 at 12:44:40PM +0200, Martin Liška wrote: > > > On 4/11/19 11:57 AM, Richard Biener wrote: > > > > On Thu, Apr 11, 2019 at 10:37 AM Martin Liška <mliska@suse.cz> wrote: > > > >> > > > >> Hi. > > > >> > > > >> The patch is catching duplicate 'default' values in target attribute. > > > >> > > > >> Patch can bootstrap on x86_64-linux-gnu and survives regression tests. > > > >> > > > >> Ready to be installed? > > > > > > > > I wonder if it isn't better to ignore duplicate "default"s (given you needed to > > > > adjust a testcase even)? > > > > > > Well, possibly yes. But I would prefer to have a more strict checking. > > > > I agree, default, default is not really useful and rejecting it is fine. > > Having a testcase covering that is of course desirable. > > There is one in the patch. > > That said, I was worried to go down the same route as that volatile asm > thing and the patch needs to be on branches as well? On branches I'd indeed ignore second and following default if that is what we've done before. Jakub
On 4/11/19 1:15 PM, Jakub Jelinek wrote: > On Thu, Apr 11, 2019 at 01:10:00PM +0200, Richard Biener wrote: >> On Thu, Apr 11, 2019 at 12:47 PM Jakub Jelinek <jakub@redhat.com> wrote: >>> >>> On Thu, Apr 11, 2019 at 12:44:40PM +0200, Martin Liška wrote: >>>> On 4/11/19 11:57 AM, Richard Biener wrote: >>>>> On Thu, Apr 11, 2019 at 10:37 AM Martin Liška <mliska@suse.cz> wrote: >>>>>> >>>>>> Hi. >>>>>> >>>>>> The patch is catching duplicate 'default' values in target attribute. >>>>>> >>>>>> Patch can bootstrap on x86_64-linux-gnu and survives regression tests. >>>>>> >>>>>> Ready to be installed? >>>>> >>>>> I wonder if it isn't better to ignore duplicate "default"s (given you needed to >>>>> adjust a testcase even)? >>>> >>>> Well, possibly yes. But I would prefer to have a more strict checking. >>> >>> I agree, default, default is not really useful and rejecting it is fine. >>> Having a testcase covering that is of course desirable. >> >> There is one in the patch. >> >> That said, I was worried to go down the same route as that volatile asm >> thing and the patch needs to be on branches as well? > > On branches I'd indeed ignore second and following default if that is what > we've done before. > > Jakub > Well, I'm probably no planning to backport that. Martin
diff --git a/gcc/multiple_target.c b/gcc/multiple_target.c index fd983cc4ad9..99dc9da44f5 100644 --- a/gcc/multiple_target.c +++ b/gcc/multiple_target.c @@ -73,7 +73,7 @@ create_dispatcher_calls (struct cgraph_node *node) if (!targetm.has_ifunc_p ()) { error_at (DECL_SOURCE_LOCATION (node->decl), - "the call requires ifunc, which is not" + "the call requires %<ifunc%>, which is not" " supported by this target"); return; } @@ -235,8 +235,10 @@ get_attr_str (tree arglist, char *attr_str) } /* Return number of attributes separated by comma and put them into ARGS. - If there is no DEFAULT attribute return -1. If there is an empty - string in attribute return -2. */ + If there is no DEFAULT attribute return -1. + If there is an empty string in attribute return -2. + If there are multiple DEFAULT attributes return -3. + */ static int separate_attrs (char *attr_str, char **attrs, int attrnum) @@ -256,6 +258,8 @@ separate_attrs (char *attr_str, char **attrs, int attrnum) } if (default_count == 0) return -1; + else if (default_count > 1) + return -3; else if (i + default_count < attrnum) return -2; @@ -347,8 +351,7 @@ expand_target_clones (struct cgraph_node *node, bool definition) if (attr_len == -1) { warning_at (DECL_SOURCE_LOCATION (node->decl), - 0, - "single %<target_clones%> attribute is ignored"); + 0, "single %<target_clones%> attribute is ignored"); return false; } @@ -374,21 +377,30 @@ expand_target_clones (struct cgraph_node *node, bool definition) char **attrs = XNEWVEC (char *, attrnum); attrnum = separate_attrs (attr_str, attrs, attrnum); - if (attrnum == -1) + switch (attrnum) { + case -1: error_at (DECL_SOURCE_LOCATION (node->decl), - "default target was not set"); - XDELETEVEC (attrs); - XDELETEVEC (attr_str); - return false; - } - else if (attrnum == -2) - { + "%<default%> target was not set"); + break; + case -2: error_at (DECL_SOURCE_LOCATION (node->decl), "an empty string cannot be in %<target_clones%> attribute"); + break; + case -3: + error_at (DECL_SOURCE_LOCATION (node->decl), + "multiple %<default%> targets were set"); + break; + default: + break; + } + + if (attrnum < 0) + { XDELETEVEC (attrs); XDELETEVEC (attr_str); return false; + } cgraph_function_version_info *decl1_v = NULL; diff --git a/gcc/testsuite/gcc.target/i386/mvc15.c b/gcc/testsuite/gcc.target/i386/mvc15.c new file mode 100644 index 00000000000..955aa1e9fe8 --- /dev/null +++ b/gcc/testsuite/gcc.target/i386/mvc15.c @@ -0,0 +1,11 @@ +/* { dg-do compile } */ +/* { dg-require-ifunc "" } */ + +__attribute__((target_clones("default", "default"))) +int foo (); /* { dg-error "multiple 'default' targets were set" } */ + +int +bar () +{ + return foo(); +} diff --git a/gcc/testsuite/gcc.target/i386/mvc3.c b/gcc/testsuite/gcc.target/i386/mvc3.c index 1c7755fabbe..f14fc02694c 100644 --- a/gcc/testsuite/gcc.target/i386/mvc3.c +++ b/gcc/testsuite/gcc.target/i386/mvc3.c @@ -2,7 +2,7 @@ /* { dg-require-ifunc "" } */ __attribute__((target_clones("avx","arch=slm","arch=core-avx2"))) -int foo (); /* { dg-error "default target was not set" } */ +int foo (); /* { dg-error "'default' target was not set" } */ int bar () diff --git a/gcc/testsuite/gcc.target/i386/mvc4.c b/gcc/testsuite/gcc.target/i386/mvc4.c index 91293c34cca..1b9b0b4f174 100644 --- a/gcc/testsuite/gcc.target/i386/mvc4.c +++ b/gcc/testsuite/gcc.target/i386/mvc4.c @@ -1,7 +1,7 @@ /* { dg-do run } */ /* { dg-require-ifunc "" } */ -__attribute__((target_clones("default","avx","default"))) +__attribute__((target_clones("default","avx"))) int foo () {