* [aarch64] PR102376 - Emit better diagnostic for arch extensions in target attr
@ 2021-10-18 11:21 Prathamesh Kulkarni
2021-10-19 14:28 ` Richard Sandiford
0 siblings, 1 reply; 12+ messages in thread
From: Prathamesh Kulkarni @ 2021-10-18 11:21 UTC (permalink / raw)
To: gcc Patches, Martin Liška, Richard Sandiford
[-- Attachment #1: Type: text/plain, Size: 696 bytes --]
Hi,
The attached patch emits a more verbose diagnostic for target attribute that
is an architecture extension needing a leading '+'.
For the following test,
void calculate(void) __attribute__ ((__target__ ("sve")));
With patch, the compiler now emits:
102376.c:1:1: error: arch extension ‘sve’ should be prepended with ‘+’
1 | void calculate(void) __attribute__ ((__target__ ("sve")));
| ^~~~
instead of:
102376.c:1:1: error: pragma or attribute ‘target("sve")’ is not valid
1 | void calculate(void) __attribute__ ((__target__ ("sve")));
| ^~~~
(This isn't specific to sve though).
OK to commit after bootstrap+test ?
Thanks,
Prathamesh
[-- Attachment #2: 102376-1.txt --]
[-- Type: text/plain, Size: 828 bytes --]
diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index a9a1800af53..975f7faf968 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -17821,7 +17821,16 @@ aarch64_process_target_attr (tree args)
num_attrs++;
if (!aarch64_process_one_target_attr (token))
{
- error ("pragma or attribute %<target(\"%s\")%> is not valid", token);
+ /* Check if token is possibly an arch extension without
+ leading '+'. */
+ char *str = (char *) xmalloc (strlen (token) + 2);
+ str[0] = '+';
+ strcpy(str + 1, token);
+ if (aarch64_handle_attr_isa_flags (str))
+ error("arch extension %<%s%> should be prepended with %<+%>", token);
+ else
+ error ("pragma or attribute %<target(\"%s\")%> is not valid", token);
+ free (str);
return false;
}
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [aarch64] PR102376 - Emit better diagnostic for arch extensions in target attr 2021-10-18 11:21 [aarch64] PR102376 - Emit better diagnostic for arch extensions in target attr Prathamesh Kulkarni @ 2021-10-19 14:28 ` Richard Sandiford 2021-10-20 7:08 ` Prathamesh Kulkarni 0 siblings, 1 reply; 12+ messages in thread From: Richard Sandiford @ 2021-10-19 14:28 UTC (permalink / raw) To: Prathamesh Kulkarni; +Cc: gcc Patches, Martin Liška Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> writes: > Hi, > The attached patch emits a more verbose diagnostic for target attribute that > is an architecture extension needing a leading '+'. > > For the following test, > void calculate(void) __attribute__ ((__target__ ("sve"))); > > With patch, the compiler now emits: > 102376.c:1:1: error: arch extension ‘sve’ should be prepended with ‘+’ > 1 | void calculate(void) __attribute__ ((__target__ ("sve"))); > | ^~~~ > > instead of: > 102376.c:1:1: error: pragma or attribute ‘target("sve")’ is not valid > 1 | void calculate(void) __attribute__ ((__target__ ("sve"))); > | ^~~~ Nice :-) > (This isn't specific to sve though). > OK to commit after bootstrap+test ? > > Thanks, > Prathamesh > > diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c > index a9a1800af53..975f7faf968 100644 > --- a/gcc/config/aarch64/aarch64.c > +++ b/gcc/config/aarch64/aarch64.c > @@ -17821,7 +17821,16 @@ aarch64_process_target_attr (tree args) > num_attrs++; > if (!aarch64_process_one_target_attr (token)) > { > - error ("pragma or attribute %<target(\"%s\")%> is not valid", token); > + /* Check if token is possibly an arch extension without > + leading '+'. */ > + char *str = (char *) xmalloc (strlen (token) + 2); > + str[0] = '+'; > + strcpy(str + 1, token); I think std::string would be better here, e.g.: auto with_plus = std::string ("+") + token; > + if (aarch64_handle_attr_isa_flags (str)) > + error("arch extension %<%s%> should be prepended with %<+%>", token); Nit: should be a space before the “(”. In principle, a fixit hint would have been nice here, but I don't think we have enough information to provide one. (Just saying for the record.) Thanks, Richard > + else > + error ("pragma or attribute %<target(\"%s\")%> is not valid", token); > + free (str); > return false; > } > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [aarch64] PR102376 - Emit better diagnostic for arch extensions in target attr 2021-10-19 14:28 ` Richard Sandiford @ 2021-10-20 7:08 ` Prathamesh Kulkarni 2021-10-20 9:35 ` Richard Sandiford 0 siblings, 1 reply; 12+ messages in thread From: Prathamesh Kulkarni @ 2021-10-20 7:08 UTC (permalink / raw) To: Prathamesh Kulkarni, gcc Patches, Martin Liška, Richard Sandiford [-- Attachment #1: Type: text/plain, Size: 2369 bytes --] On Tue, 19 Oct 2021 at 19:58, Richard Sandiford <richard.sandiford@arm.com> wrote: > > Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> writes: > > Hi, > > The attached patch emits a more verbose diagnostic for target attribute that > > is an architecture extension needing a leading '+'. > > > > For the following test, > > void calculate(void) __attribute__ ((__target__ ("sve"))); > > > > With patch, the compiler now emits: > > 102376.c:1:1: error: arch extension ‘sve’ should be prepended with ‘+’ > > 1 | void calculate(void) __attribute__ ((__target__ ("sve"))); > > | ^~~~ > > > > instead of: > > 102376.c:1:1: error: pragma or attribute ‘target("sve")’ is not valid > > 1 | void calculate(void) __attribute__ ((__target__ ("sve"))); > > | ^~~~ > > Nice :-) > > > (This isn't specific to sve though). > > OK to commit after bootstrap+test ? > > > > Thanks, > > Prathamesh > > > > diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c > > index a9a1800af53..975f7faf968 100644 > > --- a/gcc/config/aarch64/aarch64.c > > +++ b/gcc/config/aarch64/aarch64.c > > @@ -17821,7 +17821,16 @@ aarch64_process_target_attr (tree args) > > num_attrs++; > > if (!aarch64_process_one_target_attr (token)) > > { > > - error ("pragma or attribute %<target(\"%s\")%> is not valid", token); > > + /* Check if token is possibly an arch extension without > > + leading '+'. */ > > + char *str = (char *) xmalloc (strlen (token) + 2); > > + str[0] = '+'; > > + strcpy(str + 1, token); > > I think std::string would be better here, e.g.: > > auto with_plus = std::string ("+") + token; > > > + if (aarch64_handle_attr_isa_flags (str)) > > + error("arch extension %<%s%> should be prepended with %<+%>", token); > > Nit: should be a space before the “(”. > > In principle, a fixit hint would have been nice here, but I don't think > we have enough information to provide one. (Just saying for the record.) Thanks for the suggestions. Does the attached patch look OK ? Thanks, Prathamesh > > Thanks, > Richard > > > + else > > + error ("pragma or attribute %<target(\"%s\")%> is not valid", token); > > + free (str); > > return false; > > } > > [-- Attachment #2: pr102376-2.txt --] [-- Type: text/plain, Size: 1886 bytes --] [aarch64] PR102376 - Emit better diagnostics for arch extension in target attribute. gcc/ChangeLog: PR target/102376 * config/aarch64/aarch64.c (aarch64_handle_attr_isa_flags): Change str's type to const char *. (aarch64_process_target_attr): Check if token is possibly an arch extension without leading '+' and emit diagnostic accordingly. gcc/testsuite/ChangeLog: PR target/102376 * gcc.target/aarch64/pr102376.c: New test. diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c index a9a1800af53..b72079bc466 100644 --- a/gcc/config/aarch64/aarch64.c +++ b/gcc/config/aarch64/aarch64.c @@ -17548,7 +17548,7 @@ aarch64_handle_attr_tune (const char *str) modified. */ static bool -aarch64_handle_attr_isa_flags (char *str) +aarch64_handle_attr_isa_flags (const char *str) { enum aarch64_parse_opt_result parse_res; uint64_t isa_flags = aarch64_isa_flags; @@ -17821,7 +17821,13 @@ aarch64_process_target_attr (tree args) num_attrs++; if (!aarch64_process_one_target_attr (token)) { - error ("pragma or attribute %<target(\"%s\")%> is not valid", token); + /* Check if token is possibly an arch extension without + leading '+'. */ + auto with_plus = std::string("+") + token; + if (aarch64_handle_attr_isa_flags (with_plus.c_str ())) + error ("arch extension %<%s%> should be prepended with %<+%>", token); + else + error ("pragma or attribute %<target(\"%s\")%> is not valid", token); return false; } diff --git a/gcc/testsuite/gcc.target/aarch64/pr102376.c b/gcc/testsuite/gcc.target/aarch64/pr102376.c new file mode 100644 index 00000000000..efd15f6ca9b --- /dev/null +++ b/gcc/testsuite/gcc.target/aarch64/pr102376.c @@ -0,0 +1,3 @@ +/* { dg-do compile } */ + +void calculate(void) __attribute__ ((__target__ ("sve"))); /* { dg-error "arch extension 'sve' should be prepended with '\\+'" } */ ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [aarch64] PR102376 - Emit better diagnostic for arch extensions in target attr 2021-10-20 7:08 ` Prathamesh Kulkarni @ 2021-10-20 9:35 ` Richard Sandiford 2021-10-22 9:11 ` Prathamesh Kulkarni 0 siblings, 1 reply; 12+ messages in thread From: Richard Sandiford @ 2021-10-20 9:35 UTC (permalink / raw) To: Prathamesh Kulkarni; +Cc: gcc Patches, Martin Liška Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> writes: > On Tue, 19 Oct 2021 at 19:58, Richard Sandiford > <richard.sandiford@arm.com> wrote: >> >> Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> writes: >> > Hi, >> > The attached patch emits a more verbose diagnostic for target attribute that >> > is an architecture extension needing a leading '+'. >> > >> > For the following test, >> > void calculate(void) __attribute__ ((__target__ ("sve"))); >> > >> > With patch, the compiler now emits: >> > 102376.c:1:1: error: arch extension ‘sve’ should be prepended with ‘+’ >> > 1 | void calculate(void) __attribute__ ((__target__ ("sve"))); >> > | ^~~~ >> > >> > instead of: >> > 102376.c:1:1: error: pragma or attribute ‘target("sve")’ is not valid >> > 1 | void calculate(void) __attribute__ ((__target__ ("sve"))); >> > | ^~~~ >> >> Nice :-) >> >> > (This isn't specific to sve though). >> > OK to commit after bootstrap+test ? >> > >> > Thanks, >> > Prathamesh >> > >> > diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c >> > index a9a1800af53..975f7faf968 100644 >> > --- a/gcc/config/aarch64/aarch64.c >> > +++ b/gcc/config/aarch64/aarch64.c >> > @@ -17821,7 +17821,16 @@ aarch64_process_target_attr (tree args) >> > num_attrs++; >> > if (!aarch64_process_one_target_attr (token)) >> > { >> > - error ("pragma or attribute %<target(\"%s\")%> is not valid", token); >> > + /* Check if token is possibly an arch extension without >> > + leading '+'. */ >> > + char *str = (char *) xmalloc (strlen (token) + 2); >> > + str[0] = '+'; >> > + strcpy(str + 1, token); >> >> I think std::string would be better here, e.g.: >> >> auto with_plus = std::string ("+") + token; >> >> > + if (aarch64_handle_attr_isa_flags (str)) >> > + error("arch extension %<%s%> should be prepended with %<+%>", token); >> >> Nit: should be a space before the “(”. >> >> In principle, a fixit hint would have been nice here, but I don't think >> we have enough information to provide one. (Just saying for the record.) > Thanks for the suggestions. > Does the attached patch look OK ? Looks good apart from a couple of formatting nits. > > Thanks, > Prathamesh >> >> Thanks, >> Richard >> >> > + else >> > + error ("pragma or attribute %<target(\"%s\")%> is not valid", token); >> > + free (str); >> > return false; >> > } >> > > > [aarch64] PR102376 - Emit better diagnostics for arch extension in target attribute. > > gcc/ChangeLog: > PR target/102376 > * config/aarch64/aarch64.c (aarch64_handle_attr_isa_flags): Change str's > type to const char *. > (aarch64_process_target_attr): Check if token is possibly an arch extension > without leading '+' and emit diagnostic accordingly. > > gcc/testsuite/ChangeLog: > PR target/102376 > * gcc.target/aarch64/pr102376.c: New test. > diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c > index a9a1800af53..b72079bc466 100644 > --- a/gcc/config/aarch64/aarch64.c > +++ b/gcc/config/aarch64/aarch64.c > @@ -17548,7 +17548,7 @@ aarch64_handle_attr_tune (const char *str) > modified. */ > > static bool > -aarch64_handle_attr_isa_flags (char *str) > +aarch64_handle_attr_isa_flags (const char *str) > { > enum aarch64_parse_opt_result parse_res; > uint64_t isa_flags = aarch64_isa_flags; > @@ -17821,7 +17821,13 @@ aarch64_process_target_attr (tree args) > num_attrs++; > if (!aarch64_process_one_target_attr (token)) > { > - error ("pragma or attribute %<target(\"%s\")%> is not valid", token); > + /* Check if token is possibly an arch extension without > + leading '+'. */ > + auto with_plus = std::string("+") + token; Should be a space before “(”. > + if (aarch64_handle_attr_isa_flags (with_plus.c_str ())) > + error ("arch extension %<%s%> should be prepended with %<+%>", token); Long line, should be: error ("arch extension %<%s%> should be prepended with %<+%>", token); OK with those changes, thanks. Richard > + else > + error ("pragma or attribute %<target(\"%s\")%> is not valid", token); > return false; > } > > diff --git a/gcc/testsuite/gcc.target/aarch64/pr102376.c b/gcc/testsuite/gcc.target/aarch64/pr102376.c > new file mode 100644 > index 00000000000..efd15f6ca9b > --- /dev/null > +++ b/gcc/testsuite/gcc.target/aarch64/pr102376.c > @@ -0,0 +1,3 @@ > +/* { dg-do compile } */ > + > +void calculate(void) __attribute__ ((__target__ ("sve"))); /* { dg-error "arch extension 'sve' should be prepended with '\\+'" } */ ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [aarch64] PR102376 - Emit better diagnostic for arch extensions in target attr 2021-10-20 9:35 ` Richard Sandiford @ 2021-10-22 9:11 ` Prathamesh Kulkarni 2021-10-28 8:59 ` Prathamesh Kulkarni 2021-11-04 8:49 ` Richard Sandiford 0 siblings, 2 replies; 12+ messages in thread From: Prathamesh Kulkarni @ 2021-10-22 9:11 UTC (permalink / raw) To: Prathamesh Kulkarni, gcc Patches, Martin Liška, richard.sandiford [-- Attachment #1: Type: text/plain, Size: 6168 bytes --] On Wed, 20 Oct 2021 at 15:05, Richard Sandiford <richard.sandiford@arm.com> wrote: > > Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> writes: > > On Tue, 19 Oct 2021 at 19:58, Richard Sandiford > > <richard.sandiford@arm.com> wrote: > >> > >> Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> writes: > >> > Hi, > >> > The attached patch emits a more verbose diagnostic for target attribute that > >> > is an architecture extension needing a leading '+'. > >> > > >> > For the following test, > >> > void calculate(void) __attribute__ ((__target__ ("sve"))); > >> > > >> > With patch, the compiler now emits: > >> > 102376.c:1:1: error: arch extension ‘sve’ should be prepended with ‘+’ > >> > 1 | void calculate(void) __attribute__ ((__target__ ("sve"))); > >> > | ^~~~ > >> > > >> > instead of: > >> > 102376.c:1:1: error: pragma or attribute ‘target("sve")’ is not valid > >> > 1 | void calculate(void) __attribute__ ((__target__ ("sve"))); > >> > | ^~~~ > >> > >> Nice :-) > >> > >> > (This isn't specific to sve though). > >> > OK to commit after bootstrap+test ? > >> > > >> > Thanks, > >> > Prathamesh > >> > > >> > diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c > >> > index a9a1800af53..975f7faf968 100644 > >> > --- a/gcc/config/aarch64/aarch64.c > >> > +++ b/gcc/config/aarch64/aarch64.c > >> > @@ -17821,7 +17821,16 @@ aarch64_process_target_attr (tree args) > >> > num_attrs++; > >> > if (!aarch64_process_one_target_attr (token)) > >> > { > >> > - error ("pragma or attribute %<target(\"%s\")%> is not valid", token); > >> > + /* Check if token is possibly an arch extension without > >> > + leading '+'. */ > >> > + char *str = (char *) xmalloc (strlen (token) + 2); > >> > + str[0] = '+'; > >> > + strcpy(str + 1, token); > >> > >> I think std::string would be better here, e.g.: > >> > >> auto with_plus = std::string ("+") + token; > >> > >> > + if (aarch64_handle_attr_isa_flags (str)) > >> > + error("arch extension %<%s%> should be prepended with %<+%>", token); > >> > >> Nit: should be a space before the “(”. > >> > >> In principle, a fixit hint would have been nice here, but I don't think > >> we have enough information to provide one. (Just saying for the record.) > > Thanks for the suggestions. > > Does the attached patch look OK ? > > Looks good apart from a couple of formatting nits. > > > > Thanks, > > Prathamesh > >> > >> Thanks, > >> Richard > >> > >> > + else > >> > + error ("pragma or attribute %<target(\"%s\")%> is not valid", token); > >> > + free (str); > >> > return false; > >> > } > >> > > > > > [aarch64] PR102376 - Emit better diagnostics for arch extension in target attribute. > > > > gcc/ChangeLog: > > PR target/102376 > > * config/aarch64/aarch64.c (aarch64_handle_attr_isa_flags): Change str's > > type to const char *. > > (aarch64_process_target_attr): Check if token is possibly an arch extension > > without leading '+' and emit diagnostic accordingly. > > > > gcc/testsuite/ChangeLog: > > PR target/102376 > > * gcc.target/aarch64/pr102376.c: New test. > > diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c > > index a9a1800af53..b72079bc466 100644 > > --- a/gcc/config/aarch64/aarch64.c > > +++ b/gcc/config/aarch64/aarch64.c > > @@ -17548,7 +17548,7 @@ aarch64_handle_attr_tune (const char *str) > > modified. */ > > > > static bool > > -aarch64_handle_attr_isa_flags (char *str) > > +aarch64_handle_attr_isa_flags (const char *str) > > { > > enum aarch64_parse_opt_result parse_res; > > uint64_t isa_flags = aarch64_isa_flags; > > @@ -17821,7 +17821,13 @@ aarch64_process_target_attr (tree args) > > num_attrs++; > > if (!aarch64_process_one_target_attr (token)) > > { > > - error ("pragma or attribute %<target(\"%s\")%> is not valid", token); > > + /* Check if token is possibly an arch extension without > > + leading '+'. */ > > + auto with_plus = std::string("+") + token; > > Should be a space before “(”. > > > + if (aarch64_handle_attr_isa_flags (with_plus.c_str ())) > > + error ("arch extension %<%s%> should be prepended with %<+%>", token); > > Long line, should be: > > error ("arch extension %<%s%> should be prepended with %<+%>", > token); > > OK with those changes, thanks. Thanks, the patch regressed some target attr tests because it emitted diagnostics twice from aarch64_handle_attr_isa_flags. So for eg, spellcheck_1.c: __attribute__((target ("arch=armv8-a-typo"))) void foo () {} results in: spellcheck_1.c:5:1: error: invalid name ("armv8-a-typo") in ‘target("arch=")’ pragma or attribute 5 | { | ^ spellcheck_1.c:5:1: note: valid arguments are: armv8-a armv8.1-a armv8.2-a armv8.3-a armv8.4-a armv8.5-a armv8.6-a armv8.7-a armv8-r armv9-a spellcheck_1.c:5:1: error: invalid feature modifier arch=armv8-a-typo of value ("+arch=armv8-a-typo") in ‘target()’ pragma or attribute spellcheck_1.c:5:1: error: pragma or attribute ‘target("arch=armv8-a-typo")’ is not valid The patch adds an additional argument to the aarch64_handle_attr_isa_flags, to optionally not emit an error, which works to fix the issue. Does it look OK ? Thanks, Prathamesh > > Richard > > > > + else > > + error ("pragma or attribute %<target(\"%s\")%> is not valid", token); > > return false; > > } > > > > diff --git a/gcc/testsuite/gcc.target/aarch64/pr102376.c b/gcc/testsuite/gcc.target/aarch64/pr102376.c > > new file mode 100644 > > index 00000000000..efd15f6ca9b > > --- /dev/null > > +++ b/gcc/testsuite/gcc.target/aarch64/pr102376.c > > @@ -0,0 +1,3 @@ > > +/* { dg-do compile } */ > > + > > +void calculate(void) __attribute__ ((__target__ ("sve"))); /* { dg-error "arch extension 'sve' should be prepended with '\\+'" } */ [-- Attachment #2: pr102376-4.txt --] [-- Type: text/plain, Size: 1703 bytes --] diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c index a9a1800af53..cf345e05fbc 100644 --- a/gcc/config/aarch64/aarch64.c +++ b/gcc/config/aarch64/aarch64.c @@ -17548,7 +17548,7 @@ aarch64_handle_attr_tune (const char *str) modified. */ static bool -aarch64_handle_attr_isa_flags (char *str) +aarch64_handle_attr_isa_flags (const char *str, bool emit_diagnostic = true) { enum aarch64_parse_opt_result parse_res; uint64_t isa_flags = aarch64_isa_flags; @@ -17570,6 +17570,9 @@ aarch64_handle_attr_isa_flags (char *str) return true; } + if (!emit_diagnostic) + return false; + switch (parse_res) { case AARCH64_PARSE_MISSING_ARG: @@ -17821,7 +17824,14 @@ aarch64_process_target_attr (tree args) num_attrs++; if (!aarch64_process_one_target_attr (token)) { - error ("pragma or attribute %<target(\"%s\")%> is not valid", token); + /* Check if token is possibly an arch extension without + leading '+'. */ + auto with_plus = std::string ("+") + token; + if (aarch64_handle_attr_isa_flags (with_plus.c_str (), false)) + error ("arch extension %<%s%> should be prepended with %<+%>", + token); + else + error ("pragma or attribute %<target(\"%s\")%> is not valid", token); return false; } diff --git a/gcc/testsuite/gcc.target/aarch64/pr102376.c b/gcc/testsuite/gcc.target/aarch64/pr102376.c new file mode 100644 index 00000000000..efd15f6ca9b --- /dev/null +++ b/gcc/testsuite/gcc.target/aarch64/pr102376.c @@ -0,0 +1,3 @@ +/* { dg-do compile } */ + +void calculate(void) __attribute__ ((__target__ ("sve"))); /* { dg-error "arch extension 'sve' should be prepended with '\\+'" } */ ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [aarch64] PR102376 - Emit better diagnostic for arch extensions in target attr 2021-10-22 9:11 ` Prathamesh Kulkarni @ 2021-10-28 8:59 ` Prathamesh Kulkarni 2021-10-28 16:03 ` Martin Sebor 2021-11-04 8:49 ` Richard Sandiford 1 sibling, 1 reply; 12+ messages in thread From: Prathamesh Kulkarni @ 2021-10-28 8:59 UTC (permalink / raw) To: Prathamesh Kulkarni, gcc Patches, Martin Liška, richard.sandiford On Fri, 22 Oct 2021 at 14:41, Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> wrote: > > On Wed, 20 Oct 2021 at 15:05, Richard Sandiford > <richard.sandiford@arm.com> wrote: > > > > Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> writes: > > > On Tue, 19 Oct 2021 at 19:58, Richard Sandiford > > > <richard.sandiford@arm.com> wrote: > > >> > > >> Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> writes: > > >> > Hi, > > >> > The attached patch emits a more verbose diagnostic for target attribute that > > >> > is an architecture extension needing a leading '+'. > > >> > > > >> > For the following test, > > >> > void calculate(void) __attribute__ ((__target__ ("sve"))); > > >> > > > >> > With patch, the compiler now emits: > > >> > 102376.c:1:1: error: arch extension ‘sve’ should be prepended with ‘+’ > > >> > 1 | void calculate(void) __attribute__ ((__target__ ("sve"))); > > >> > | ^~~~ > > >> > > > >> > instead of: > > >> > 102376.c:1:1: error: pragma or attribute ‘target("sve")’ is not valid > > >> > 1 | void calculate(void) __attribute__ ((__target__ ("sve"))); > > >> > | ^~~~ > > >> > > >> Nice :-) > > >> > > >> > (This isn't specific to sve though). > > >> > OK to commit after bootstrap+test ? > > >> > > > >> > Thanks, > > >> > Prathamesh > > >> > > > >> > diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c > > >> > index a9a1800af53..975f7faf968 100644 > > >> > --- a/gcc/config/aarch64/aarch64.c > > >> > +++ b/gcc/config/aarch64/aarch64.c > > >> > @@ -17821,7 +17821,16 @@ aarch64_process_target_attr (tree args) > > >> > num_attrs++; > > >> > if (!aarch64_process_one_target_attr (token)) > > >> > { > > >> > - error ("pragma or attribute %<target(\"%s\")%> is not valid", token); > > >> > + /* Check if token is possibly an arch extension without > > >> > + leading '+'. */ > > >> > + char *str = (char *) xmalloc (strlen (token) + 2); > > >> > + str[0] = '+'; > > >> > + strcpy(str + 1, token); > > >> > > >> I think std::string would be better here, e.g.: > > >> > > >> auto with_plus = std::string ("+") + token; > > >> > > >> > + if (aarch64_handle_attr_isa_flags (str)) > > >> > + error("arch extension %<%s%> should be prepended with %<+%>", token); > > >> > > >> Nit: should be a space before the “(”. > > >> > > >> In principle, a fixit hint would have been nice here, but I don't think > > >> we have enough information to provide one. (Just saying for the record.) > > > Thanks for the suggestions. > > > Does the attached patch look OK ? > > > > Looks good apart from a couple of formatting nits. > > > > > > Thanks, > > > Prathamesh > > >> > > >> Thanks, > > >> Richard > > >> > > >> > + else > > >> > + error ("pragma or attribute %<target(\"%s\")%> is not valid", token); > > >> > + free (str); > > >> > return false; > > >> > } > > >> > > > > > > > [aarch64] PR102376 - Emit better diagnostics for arch extension in target attribute. > > > > > > gcc/ChangeLog: > > > PR target/102376 > > > * config/aarch64/aarch64.c (aarch64_handle_attr_isa_flags): Change str's > > > type to const char *. > > > (aarch64_process_target_attr): Check if token is possibly an arch extension > > > without leading '+' and emit diagnostic accordingly. > > > > > > gcc/testsuite/ChangeLog: > > > PR target/102376 > > > * gcc.target/aarch64/pr102376.c: New test. > > > diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c > > > index a9a1800af53..b72079bc466 100644 > > > --- a/gcc/config/aarch64/aarch64.c > > > +++ b/gcc/config/aarch64/aarch64.c > > > @@ -17548,7 +17548,7 @@ aarch64_handle_attr_tune (const char *str) > > > modified. */ > > > > > > static bool > > > -aarch64_handle_attr_isa_flags (char *str) > > > +aarch64_handle_attr_isa_flags (const char *str) > > > { > > > enum aarch64_parse_opt_result parse_res; > > > uint64_t isa_flags = aarch64_isa_flags; > > > @@ -17821,7 +17821,13 @@ aarch64_process_target_attr (tree args) > > > num_attrs++; > > > if (!aarch64_process_one_target_attr (token)) > > > { > > > - error ("pragma or attribute %<target(\"%s\")%> is not valid", token); > > > + /* Check if token is possibly an arch extension without > > > + leading '+'. */ > > > + auto with_plus = std::string("+") + token; > > > > Should be a space before “(”. > > > > > + if (aarch64_handle_attr_isa_flags (with_plus.c_str ())) > > > + error ("arch extension %<%s%> should be prepended with %<+%>", token); > > > > Long line, should be: > > > > error ("arch extension %<%s%> should be prepended with %<+%>", > > token); > > > > OK with those changes, thanks. > Thanks, the patch regressed some target attr tests because it emitted > diagnostics twice from > aarch64_handle_attr_isa_flags. > So for eg, spellcheck_1.c: > __attribute__((target ("arch=armv8-a-typo"))) void foo () {} > > results in: > spellcheck_1.c:5:1: error: invalid name ("armv8-a-typo") in > ‘target("arch=")’ pragma or attribute > 5 | { > | ^ > spellcheck_1.c:5:1: note: valid arguments are: armv8-a armv8.1-a > armv8.2-a armv8.3-a armv8.4-a armv8.5-a armv8.6-a armv8.7-a armv8-r > armv9-a > spellcheck_1.c:5:1: error: invalid feature modifier arch=armv8-a-typo > of value ("+arch=armv8-a-typo") in ‘target()’ pragma or attribute > spellcheck_1.c:5:1: error: pragma or attribute > ‘target("arch=armv8-a-typo")’ is not valid > > The patch adds an additional argument to the > aarch64_handle_attr_isa_flags, to optionally not emit an error, which > works to fix the issue. > Does it look OK ? ping https://gcc.gnu.org/pipermail/gcc-patches/2021-October/582345.html Thanks, Prathamesh > > Thanks, > Prathamesh > > > > Richard > > > > > > > + else > > > + error ("pragma or attribute %<target(\"%s\")%> is not valid", token); > > > return false; > > > } > > > > > > diff --git a/gcc/testsuite/gcc.target/aarch64/pr102376.c b/gcc/testsuite/gcc.target/aarch64/pr102376.c > > > new file mode 100644 > > > index 00000000000..efd15f6ca9b > > > --- /dev/null > > > +++ b/gcc/testsuite/gcc.target/aarch64/pr102376.c > > > @@ -0,0 +1,3 @@ > > > +/* { dg-do compile } */ > > > + > > > +void calculate(void) __attribute__ ((__target__ ("sve"))); /* { dg-error "arch extension 'sve' should be prepended with '\\+'" } */ ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [aarch64] PR102376 - Emit better diagnostic for arch extensions in target attr 2021-10-28 8:59 ` Prathamesh Kulkarni @ 2021-10-28 16:03 ` Martin Sebor 2021-11-04 7:04 ` Prathamesh Kulkarni 0 siblings, 1 reply; 12+ messages in thread From: Martin Sebor @ 2021-10-28 16:03 UTC (permalink / raw) To: Prathamesh Kulkarni, gcc Patches, Martin Liška, richard.sandiford On 10/28/21 2:59 AM, Prathamesh Kulkarni via Gcc-patches wrote: > On Fri, 22 Oct 2021 at 14:41, Prathamesh Kulkarni > <prathamesh.kulkarni@linaro.org> wrote: >> >> On Wed, 20 Oct 2021 at 15:05, Richard Sandiford >> <richard.sandiford@arm.com> wrote: >>> >>> Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> writes: >>>> On Tue, 19 Oct 2021 at 19:58, Richard Sandiford >>>> <richard.sandiford@arm.com> wrote: >>>>> >>>>> Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> writes: >>>>>> Hi, >>>>>> The attached patch emits a more verbose diagnostic for target attribute that >>>>>> is an architecture extension needing a leading '+'. >>>>>> >>>>>> For the following test, >>>>>> void calculate(void) __attribute__ ((__target__ ("sve"))); >>>>>> >>>>>> With patch, the compiler now emits: >>>>>> 102376.c:1:1: error: arch extension ‘sve’ should be prepended with ‘+’ >>>>>> 1 | void calculate(void) __attribute__ ((__target__ ("sve"))); >>>>>> | ^~~~ >>>>>> >>>>>> instead of: >>>>>> 102376.c:1:1: error: pragma or attribute ‘target("sve")’ is not valid >>>>>> 1 | void calculate(void) __attribute__ ((__target__ ("sve"))); >>>>>> | ^~~~ >>>>> >>>>> Nice :-) >>>>> >>>>>> (This isn't specific to sve though). >>>>>> OK to commit after bootstrap+test ? >>>>>> >>>>>> Thanks, >>>>>> Prathamesh >>>>>> >>>>>> diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c >>>>>> index a9a1800af53..975f7faf968 100644 >>>>>> --- a/gcc/config/aarch64/aarch64.c >>>>>> +++ b/gcc/config/aarch64/aarch64.c >>>>>> @@ -17821,7 +17821,16 @@ aarch64_process_target_attr (tree args) >>>>>> num_attrs++; >>>>>> if (!aarch64_process_one_target_attr (token)) >>>>>> { >>>>>> - error ("pragma or attribute %<target(\"%s\")%> is not valid", token); >>>>>> + /* Check if token is possibly an arch extension without >>>>>> + leading '+'. */ >>>>>> + char *str = (char *) xmalloc (strlen (token) + 2); >>>>>> + str[0] = '+'; >>>>>> + strcpy(str + 1, token); >>>>> >>>>> I think std::string would be better here, e.g.: >>>>> >>>>> auto with_plus = std::string ("+") + token; >>>>> >>>>>> + if (aarch64_handle_attr_isa_flags (str)) >>>>>> + error("arch extension %<%s%> should be prepended with %<+%>", token); >>>>> >>>>> Nit: should be a space before the “(”. >>>>> >>>>> In principle, a fixit hint would have been nice here, but I don't think >>>>> we have enough information to provide one. (Just saying for the record.) >>>> Thanks for the suggestions. >>>> Does the attached patch look OK ? >>> >>> Looks good apart from a couple of formatting nits. >>>> >>>> Thanks, >>>> Prathamesh >>>>> >>>>> Thanks, >>>>> Richard >>>>> >>>>>> + else >>>>>> + error ("pragma or attribute %<target(\"%s\")%> is not valid", token); >>>>>> + free (str); >>>>>> return false; >>>>>> } >>>>>> >>>> >>>> [aarch64] PR102376 - Emit better diagnostics for arch extension in target attribute. >>>> >>>> gcc/ChangeLog: >>>> PR target/102376 >>>> * config/aarch64/aarch64.c (aarch64_handle_attr_isa_flags): Change str's >>>> type to const char *. >>>> (aarch64_process_target_attr): Check if token is possibly an arch extension >>>> without leading '+' and emit diagnostic accordingly. >>>> >>>> gcc/testsuite/ChangeLog: >>>> PR target/102376 >>>> * gcc.target/aarch64/pr102376.c: New test. >>>> diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c >>>> index a9a1800af53..b72079bc466 100644 >>>> --- a/gcc/config/aarch64/aarch64.c >>>> +++ b/gcc/config/aarch64/aarch64.c >>>> @@ -17548,7 +17548,7 @@ aarch64_handle_attr_tune (const char *str) >>>> modified. */ >>>> >>>> static bool >>>> -aarch64_handle_attr_isa_flags (char *str) >>>> +aarch64_handle_attr_isa_flags (const char *str) >>>> { >>>> enum aarch64_parse_opt_result parse_res; >>>> uint64_t isa_flags = aarch64_isa_flags; >>>> @@ -17821,7 +17821,13 @@ aarch64_process_target_attr (tree args) >>>> num_attrs++; >>>> if (!aarch64_process_one_target_attr (token)) >>>> { >>>> - error ("pragma or attribute %<target(\"%s\")%> is not valid", token); >>>> + /* Check if token is possibly an arch extension without >>>> + leading '+'. */ >>>> + auto with_plus = std::string("+") + token; >>> >>> Should be a space before “(”. >>> >>>> + if (aarch64_handle_attr_isa_flags (with_plus.c_str ())) >>>> + error ("arch extension %<%s%> should be prepended with %<+%>", token); >>> >>> Long line, should be: >>> >>> error ("arch extension %<%s%> should be prepended with %<+%>", >>> token); >>> >>> OK with those changes, thanks. >> Thanks, the patch regressed some target attr tests because it emitted >> diagnostics twice from >> aarch64_handle_attr_isa_flags. >> So for eg, spellcheck_1.c: >> __attribute__((target ("arch=armv8-a-typo"))) void foo () {} >> >> results in: >> spellcheck_1.c:5:1: error: invalid name ("armv8-a-typo") in >> ‘target("arch=")’ pragma or attribute >> 5 | { >> | ^ >> spellcheck_1.c:5:1: note: valid arguments are: armv8-a armv8.1-a >> armv8.2-a armv8.3-a armv8.4-a armv8.5-a armv8.6-a armv8.7-a armv8-r >> armv9-a >> spellcheck_1.c:5:1: error: invalid feature modifier arch=armv8-a-typo >> of value ("+arch=armv8-a-typo") in ‘target()’ pragma or attribute >> spellcheck_1.c:5:1: error: pragma or attribute >> ‘target("arch=armv8-a-typo")’ is not valid >> >> The patch adds an additional argument to the >> aarch64_handle_attr_isa_flags, to optionally not emit an error, which >> works to fix the issue. >> Does it look OK ? > ping https://gcc.gnu.org/pipermail/gcc-patches/2021-October/582345.html Just a couple of minor points: + if (aarch64_handle_attr_isa_flags (with_plus.c_str (), false)) + error ("arch extension %<%s%> should be prepended with %<+%>", + token); The phrase is to prepend something or prepend it to something, but usually not to "prepend with." In this context I think either "prefixed by" or "preceded by" would be correct. Also, although there are several other pre-existing uses, the abbreviation arch is not an English word that lends itself to translation. Judging by the German and French .po files, their authors take the trouble of spelling it out, but others such as Spanish or Russian don't, leaving it as is, presumably because they're not sure whether it's supposed to be translated. In the Russian translation it looks especially odd rendered in the latin alphabet in the middle of a sentence in Cyrillic. I think we should follow the German and French translators' lead and spell it out in English as well. "architecture extension %<%s%> should be prefixed by %<+%>" Martin > > Thanks, > Prathamesh >> >> Thanks, >> Prathamesh >>> >>> Richard >>> >>> >>>> + else >>>> + error ("pragma or attribute %<target(\"%s\")%> is not valid", token); >>>> return false; >>>> } >>>> >>>> diff --git a/gcc/testsuite/gcc.target/aarch64/pr102376.c b/gcc/testsuite/gcc.target/aarch64/pr102376.c >>>> new file mode 100644 >>>> index 00000000000..efd15f6ca9b >>>> --- /dev/null >>>> +++ b/gcc/testsuite/gcc.target/aarch64/pr102376.c >>>> @@ -0,0 +1,3 @@ >>>> +/* { dg-do compile } */ >>>> + >>>> +void calculate(void) __attribute__ ((__target__ ("sve"))); /* { dg-error "arch extension 'sve' should be prepended with '\\+'" } */ ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [aarch64] PR102376 - Emit better diagnostic for arch extensions in target attr 2021-10-28 16:03 ` Martin Sebor @ 2021-11-04 7:04 ` Prathamesh Kulkarni 0 siblings, 0 replies; 12+ messages in thread From: Prathamesh Kulkarni @ 2021-11-04 7:04 UTC (permalink / raw) To: Martin Sebor; +Cc: gcc Patches, Martin Liška, richard.sandiford [-- Attachment #1: Type: text/plain, Size: 8441 bytes --] On Thu, 28 Oct 2021 at 21:33, Martin Sebor <msebor@gmail.com> wrote: > > On 10/28/21 2:59 AM, Prathamesh Kulkarni via Gcc-patches wrote: > > On Fri, 22 Oct 2021 at 14:41, Prathamesh Kulkarni > > <prathamesh.kulkarni@linaro.org> wrote: > >> > >> On Wed, 20 Oct 2021 at 15:05, Richard Sandiford > >> <richard.sandiford@arm.com> wrote: > >>> > >>> Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> writes: > >>>> On Tue, 19 Oct 2021 at 19:58, Richard Sandiford > >>>> <richard.sandiford@arm.com> wrote: > >>>>> > >>>>> Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> writes: > >>>>>> Hi, > >>>>>> The attached patch emits a more verbose diagnostic for target attribute that > >>>>>> is an architecture extension needing a leading '+'. > >>>>>> > >>>>>> For the following test, > >>>>>> void calculate(void) __attribute__ ((__target__ ("sve"))); > >>>>>> > >>>>>> With patch, the compiler now emits: > >>>>>> 102376.c:1:1: error: arch extension ‘sve’ should be prepended with ‘+’ > >>>>>> 1 | void calculate(void) __attribute__ ((__target__ ("sve"))); > >>>>>> | ^~~~ > >>>>>> > >>>>>> instead of: > >>>>>> 102376.c:1:1: error: pragma or attribute ‘target("sve")’ is not valid > >>>>>> 1 | void calculate(void) __attribute__ ((__target__ ("sve"))); > >>>>>> | ^~~~ > >>>>> > >>>>> Nice :-) > >>>>> > >>>>>> (This isn't specific to sve though). > >>>>>> OK to commit after bootstrap+test ? > >>>>>> > >>>>>> Thanks, > >>>>>> Prathamesh > >>>>>> > >>>>>> diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c > >>>>>> index a9a1800af53..975f7faf968 100644 > >>>>>> --- a/gcc/config/aarch64/aarch64.c > >>>>>> +++ b/gcc/config/aarch64/aarch64.c > >>>>>> @@ -17821,7 +17821,16 @@ aarch64_process_target_attr (tree args) > >>>>>> num_attrs++; > >>>>>> if (!aarch64_process_one_target_attr (token)) > >>>>>> { > >>>>>> - error ("pragma or attribute %<target(\"%s\")%> is not valid", token); > >>>>>> + /* Check if token is possibly an arch extension without > >>>>>> + leading '+'. */ > >>>>>> + char *str = (char *) xmalloc (strlen (token) + 2); > >>>>>> + str[0] = '+'; > >>>>>> + strcpy(str + 1, token); > >>>>> > >>>>> I think std::string would be better here, e.g.: > >>>>> > >>>>> auto with_plus = std::string ("+") + token; > >>>>> > >>>>>> + if (aarch64_handle_attr_isa_flags (str)) > >>>>>> + error("arch extension %<%s%> should be prepended with %<+%>", token); > >>>>> > >>>>> Nit: should be a space before the “(”. > >>>>> > >>>>> In principle, a fixit hint would have been nice here, but I don't think > >>>>> we have enough information to provide one. (Just saying for the record.) > >>>> Thanks for the suggestions. > >>>> Does the attached patch look OK ? > >>> > >>> Looks good apart from a couple of formatting nits. > >>>> > >>>> Thanks, > >>>> Prathamesh > >>>>> > >>>>> Thanks, > >>>>> Richard > >>>>> > >>>>>> + else > >>>>>> + error ("pragma or attribute %<target(\"%s\")%> is not valid", token); > >>>>>> + free (str); > >>>>>> return false; > >>>>>> } > >>>>>> > >>>> > >>>> [aarch64] PR102376 - Emit better diagnostics for arch extension in target attribute. > >>>> > >>>> gcc/ChangeLog: > >>>> PR target/102376 > >>>> * config/aarch64/aarch64.c (aarch64_handle_attr_isa_flags): Change str's > >>>> type to const char *. > >>>> (aarch64_process_target_attr): Check if token is possibly an arch extension > >>>> without leading '+' and emit diagnostic accordingly. > >>>> > >>>> gcc/testsuite/ChangeLog: > >>>> PR target/102376 > >>>> * gcc.target/aarch64/pr102376.c: New test. > >>>> diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c > >>>> index a9a1800af53..b72079bc466 100644 > >>>> --- a/gcc/config/aarch64/aarch64.c > >>>> +++ b/gcc/config/aarch64/aarch64.c > >>>> @@ -17548,7 +17548,7 @@ aarch64_handle_attr_tune (const char *str) > >>>> modified. */ > >>>> > >>>> static bool > >>>> -aarch64_handle_attr_isa_flags (char *str) > >>>> +aarch64_handle_attr_isa_flags (const char *str) > >>>> { > >>>> enum aarch64_parse_opt_result parse_res; > >>>> uint64_t isa_flags = aarch64_isa_flags; > >>>> @@ -17821,7 +17821,13 @@ aarch64_process_target_attr (tree args) > >>>> num_attrs++; > >>>> if (!aarch64_process_one_target_attr (token)) > >>>> { > >>>> - error ("pragma or attribute %<target(\"%s\")%> is not valid", token); > >>>> + /* Check if token is possibly an arch extension without > >>>> + leading '+'. */ > >>>> + auto with_plus = std::string("+") + token; > >>> > >>> Should be a space before “(”. > >>> > >>>> + if (aarch64_handle_attr_isa_flags (with_plus.c_str ())) > >>>> + error ("arch extension %<%s%> should be prepended with %<+%>", token); > >>> > >>> Long line, should be: > >>> > >>> error ("arch extension %<%s%> should be prepended with %<+%>", > >>> token); > >>> > >>> OK with those changes, thanks. > >> Thanks, the patch regressed some target attr tests because it emitted > >> diagnostics twice from > >> aarch64_handle_attr_isa_flags. > >> So for eg, spellcheck_1.c: > >> __attribute__((target ("arch=armv8-a-typo"))) void foo () {} > >> > >> results in: > >> spellcheck_1.c:5:1: error: invalid name ("armv8-a-typo") in > >> ‘target("arch=")’ pragma or attribute > >> 5 | { > >> | ^ > >> spellcheck_1.c:5:1: note: valid arguments are: armv8-a armv8.1-a > >> armv8.2-a armv8.3-a armv8.4-a armv8.5-a armv8.6-a armv8.7-a armv8-r > >> armv9-a > >> spellcheck_1.c:5:1: error: invalid feature modifier arch=armv8-a-typo > >> of value ("+arch=armv8-a-typo") in ‘target()’ pragma or attribute > >> spellcheck_1.c:5:1: error: pragma or attribute > >> ‘target("arch=armv8-a-typo")’ is not valid > >> > >> The patch adds an additional argument to the > >> aarch64_handle_attr_isa_flags, to optionally not emit an error, which > >> works to fix the issue. > >> Does it look OK ? > > ping https://gcc.gnu.org/pipermail/gcc-patches/2021-October/582345.html > > Just a couple of minor points: > > + if (aarch64_handle_attr_isa_flags (with_plus.c_str (), false)) > + error ("arch extension %<%s%> should be prepended with %<+%>", > + token); > > The phrase is to prepend something or prepend it to something, > but usually not to "prepend with." In this context I think > either "prefixed by" or "preceded by" would be correct. > > Also, although there are several other pre-existing uses, > the abbreviation arch is not an English word that lends itself > to translation. Judging by the German and French .po files, > their authors take the trouble of spelling it out, but others > such as Spanish or Russian don't, leaving it as is, presumably > because they're not sure whether it's supposed to be translated. > In the Russian translation it looks especially odd rendered in > the latin alphabet in the middle of a sentence in Cyrillic. > I think we should follow the German and French translators' > lead and spell it out in English as well. > > "architecture extension %<%s%> should be prefixed by %<+%>" Hi Martin, Thanks for the suggestions and sorry for late response. The attached patch updates the diagnostic to use "prefixed by" instead. Richard, is this patch OK to commit after bootstrap+test ? Thanks, Prathamesh > > Martin > > > > > Thanks, > > Prathamesh > >> > >> Thanks, > >> Prathamesh > >>> > >>> Richard > >>> > >>> > >>>> + else > >>>> + error ("pragma or attribute %<target(\"%s\")%> is not valid", token); > >>>> return false; > >>>> } > >>>> > >>>> diff --git a/gcc/testsuite/gcc.target/aarch64/pr102376.c b/gcc/testsuite/gcc.target/aarch64/pr102376.c > >>>> new file mode 100644 > >>>> index 00000000000..efd15f6ca9b > >>>> --- /dev/null > >>>> +++ b/gcc/testsuite/gcc.target/aarch64/pr102376.c > >>>> @@ -0,0 +1,3 @@ > >>>> +/* { dg-do compile } */ > >>>> + > >>>> +void calculate(void) __attribute__ ((__target__ ("sve"))); /* { dg-error "arch extension 'sve' should be prepended with '\\+'" } */ > [-- Attachment #2: pr102376-5.txt --] [-- Type: text/plain, Size: 1697 bytes --] diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c index fd9249c62b3..b449241f6bd 100644 --- a/gcc/config/aarch64/aarch64.c +++ b/gcc/config/aarch64/aarch64.c @@ -17571,7 +17571,7 @@ aarch64_handle_attr_tune (const char *str) modified. */ static bool -aarch64_handle_attr_isa_flags (char *str) +aarch64_handle_attr_isa_flags (const char *str, bool emit_diagnostic = true) { enum aarch64_parse_opt_result parse_res; uint64_t isa_flags = aarch64_isa_flags; @@ -17593,6 +17593,9 @@ aarch64_handle_attr_isa_flags (char *str) return true; } + if (!emit_diagnostic) + return false; + switch (parse_res) { case AARCH64_PARSE_MISSING_ARG: @@ -17844,7 +17847,14 @@ aarch64_process_target_attr (tree args) num_attrs++; if (!aarch64_process_one_target_attr (token)) { - error ("pragma or attribute %<target(\"%s\")%> is not valid", token); + /* Check if token is possibly an arch extension without + leading '+'. */ + auto with_plus = std::string ("+") + token; + if (aarch64_handle_attr_isa_flags (with_plus.c_str (), false)) + error ("arch extension %<%s%> should be prefixed by %<+%>", + token); + else + error ("pragma or attribute %<target(\"%s\")%> is not valid", token); return false; } diff --git a/gcc/testsuite/gcc.target/aarch64/pr102376.c b/gcc/testsuite/gcc.target/aarch64/pr102376.c new file mode 100644 index 00000000000..fc830ad4742 --- /dev/null +++ b/gcc/testsuite/gcc.target/aarch64/pr102376.c @@ -0,0 +1,3 @@ +/* { dg-do compile } */ + +void calculate(void) __attribute__ ((__target__ ("sve"))); /* { dg-error "arch extension 'sve' should be prefixed by '\\+'" } */ ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [aarch64] PR102376 - Emit better diagnostic for arch extensions in target attr 2021-10-22 9:11 ` Prathamesh Kulkarni 2021-10-28 8:59 ` Prathamesh Kulkarni @ 2021-11-04 8:49 ` Richard Sandiford 2021-11-08 10:33 ` Prathamesh Kulkarni 1 sibling, 1 reply; 12+ messages in thread From: Richard Sandiford @ 2021-11-04 8:49 UTC (permalink / raw) To: Prathamesh Kulkarni; +Cc: gcc Patches, Martin Liška Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> writes: > On Wed, 20 Oct 2021 at 15:05, Richard Sandiford > <richard.sandiford@arm.com> wrote: >> >> Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> writes: >> > On Tue, 19 Oct 2021 at 19:58, Richard Sandiford >> > <richard.sandiford@arm.com> wrote: >> >> >> >> Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> writes: >> >> > Hi, >> >> > The attached patch emits a more verbose diagnostic for target attribute that >> >> > is an architecture extension needing a leading '+'. >> >> > >> >> > For the following test, >> >> > void calculate(void) __attribute__ ((__target__ ("sve"))); >> >> > >> >> > With patch, the compiler now emits: >> >> > 102376.c:1:1: error: arch extension ‘sve’ should be prepended with ‘+’ >> >> > 1 | void calculate(void) __attribute__ ((__target__ ("sve"))); >> >> > | ^~~~ >> >> > >> >> > instead of: >> >> > 102376.c:1:1: error: pragma or attribute ‘target("sve")’ is not valid >> >> > 1 | void calculate(void) __attribute__ ((__target__ ("sve"))); >> >> > | ^~~~ >> >> >> >> Nice :-) >> >> >> >> > (This isn't specific to sve though). >> >> > OK to commit after bootstrap+test ? >> >> > >> >> > Thanks, >> >> > Prathamesh >> >> > >> >> > diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c >> >> > index a9a1800af53..975f7faf968 100644 >> >> > --- a/gcc/config/aarch64/aarch64.c >> >> > +++ b/gcc/config/aarch64/aarch64.c >> >> > @@ -17821,7 +17821,16 @@ aarch64_process_target_attr (tree args) >> >> > num_attrs++; >> >> > if (!aarch64_process_one_target_attr (token)) >> >> > { >> >> > - error ("pragma or attribute %<target(\"%s\")%> is not valid", token); >> >> > + /* Check if token is possibly an arch extension without >> >> > + leading '+'. */ >> >> > + char *str = (char *) xmalloc (strlen (token) + 2); >> >> > + str[0] = '+'; >> >> > + strcpy(str + 1, token); >> >> >> >> I think std::string would be better here, e.g.: >> >> >> >> auto with_plus = std::string ("+") + token; >> >> >> >> > + if (aarch64_handle_attr_isa_flags (str)) >> >> > + error("arch extension %<%s%> should be prepended with %<+%>", token); >> >> >> >> Nit: should be a space before the “(”. >> >> >> >> In principle, a fixit hint would have been nice here, but I don't think >> >> we have enough information to provide one. (Just saying for the record.) >> > Thanks for the suggestions. >> > Does the attached patch look OK ? >> >> Looks good apart from a couple of formatting nits. >> > >> > Thanks, >> > Prathamesh >> >> >> >> Thanks, >> >> Richard >> >> >> >> > + else >> >> > + error ("pragma or attribute %<target(\"%s\")%> is not valid", token); >> >> > + free (str); >> >> > return false; >> >> > } >> >> > >> > >> > [aarch64] PR102376 - Emit better diagnostics for arch extension in target attribute. >> > >> > gcc/ChangeLog: >> > PR target/102376 >> > * config/aarch64/aarch64.c (aarch64_handle_attr_isa_flags): Change str's >> > type to const char *. >> > (aarch64_process_target_attr): Check if token is possibly an arch extension >> > without leading '+' and emit diagnostic accordingly. >> > >> > gcc/testsuite/ChangeLog: >> > PR target/102376 >> > * gcc.target/aarch64/pr102376.c: New test. >> > diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c >> > index a9a1800af53..b72079bc466 100644 >> > --- a/gcc/config/aarch64/aarch64.c >> > +++ b/gcc/config/aarch64/aarch64.c >> > @@ -17548,7 +17548,7 @@ aarch64_handle_attr_tune (const char *str) >> > modified. */ >> > >> > static bool >> > -aarch64_handle_attr_isa_flags (char *str) >> > +aarch64_handle_attr_isa_flags (const char *str) >> > { >> > enum aarch64_parse_opt_result parse_res; >> > uint64_t isa_flags = aarch64_isa_flags; >> > @@ -17821,7 +17821,13 @@ aarch64_process_target_attr (tree args) >> > num_attrs++; >> > if (!aarch64_process_one_target_attr (token)) >> > { >> > - error ("pragma or attribute %<target(\"%s\")%> is not valid", token); >> > + /* Check if token is possibly an arch extension without >> > + leading '+'. */ >> > + auto with_plus = std::string("+") + token; >> >> Should be a space before “(”. >> >> > + if (aarch64_handle_attr_isa_flags (with_plus.c_str ())) >> > + error ("arch extension %<%s%> should be prepended with %<+%>", token); >> >> Long line, should be: >> >> error ("arch extension %<%s%> should be prepended with %<+%>", >> token); >> >> OK with those changes, thanks. > Thanks, the patch regressed some target attr tests because it emitted > diagnostics twice from > aarch64_handle_attr_isa_flags. > So for eg, spellcheck_1.c: > __attribute__((target ("arch=armv8-a-typo"))) void foo () {} > > results in: > spellcheck_1.c:5:1: error: invalid name ("armv8-a-typo") in > ‘target("arch=")’ pragma or attribute > 5 | { > | ^ > spellcheck_1.c:5:1: note: valid arguments are: armv8-a armv8.1-a > armv8.2-a armv8.3-a armv8.4-a armv8.5-a armv8.6-a armv8.7-a armv8-r > armv9-a > spellcheck_1.c:5:1: error: invalid feature modifier arch=armv8-a-typo > of value ("+arch=armv8-a-typo") in ‘target()’ pragma or attribute > spellcheck_1.c:5:1: error: pragma or attribute > ‘target("arch=armv8-a-typo")’ is not valid > > The patch adds an additional argument to the > aarch64_handle_attr_isa_flags, to optionally not emit an error, which > works to fix the issue. > Does it look OK ? I think we should instead call aarch64_parse_arch directly, passing temporary ISA flags instead of &aarch64_isa_flags. That should ensure that the call has no side effects. I agree the new wording (in the later patch) is better, thanks. Richard ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [aarch64] PR102376 - Emit better diagnostic for arch extensions in target attr 2021-11-04 8:49 ` Richard Sandiford @ 2021-11-08 10:33 ` Prathamesh Kulkarni 2021-11-09 14:57 ` Richard Sandiford 0 siblings, 1 reply; 12+ messages in thread From: Prathamesh Kulkarni @ 2021-11-08 10:33 UTC (permalink / raw) To: Prathamesh Kulkarni, gcc Patches, Martin Liška, richard.sandiford [-- Attachment #1: Type: text/plain, Size: 6502 bytes --] On Thu, 4 Nov 2021 at 14:19, Richard Sandiford <richard.sandiford@arm.com> wrote: > > Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> writes: > > On Wed, 20 Oct 2021 at 15:05, Richard Sandiford > > <richard.sandiford@arm.com> wrote: > >> > >> Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> writes: > >> > On Tue, 19 Oct 2021 at 19:58, Richard Sandiford > >> > <richard.sandiford@arm.com> wrote: > >> >> > >> >> Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> writes: > >> >> > Hi, > >> >> > The attached patch emits a more verbose diagnostic for target attribute that > >> >> > is an architecture extension needing a leading '+'. > >> >> > > >> >> > For the following test, > >> >> > void calculate(void) __attribute__ ((__target__ ("sve"))); > >> >> > > >> >> > With patch, the compiler now emits: > >> >> > 102376.c:1:1: error: arch extension ‘sve’ should be prepended with ‘+’ > >> >> > 1 | void calculate(void) __attribute__ ((__target__ ("sve"))); > >> >> > | ^~~~ > >> >> > > >> >> > instead of: > >> >> > 102376.c:1:1: error: pragma or attribute ‘target("sve")’ is not valid > >> >> > 1 | void calculate(void) __attribute__ ((__target__ ("sve"))); > >> >> > | ^~~~ > >> >> > >> >> Nice :-) > >> >> > >> >> > (This isn't specific to sve though). > >> >> > OK to commit after bootstrap+test ? > >> >> > > >> >> > Thanks, > >> >> > Prathamesh > >> >> > > >> >> > diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c > >> >> > index a9a1800af53..975f7faf968 100644 > >> >> > --- a/gcc/config/aarch64/aarch64.c > >> >> > +++ b/gcc/config/aarch64/aarch64.c > >> >> > @@ -17821,7 +17821,16 @@ aarch64_process_target_attr (tree args) > >> >> > num_attrs++; > >> >> > if (!aarch64_process_one_target_attr (token)) > >> >> > { > >> >> > - error ("pragma or attribute %<target(\"%s\")%> is not valid", token); > >> >> > + /* Check if token is possibly an arch extension without > >> >> > + leading '+'. */ > >> >> > + char *str = (char *) xmalloc (strlen (token) + 2); > >> >> > + str[0] = '+'; > >> >> > + strcpy(str + 1, token); > >> >> > >> >> I think std::string would be better here, e.g.: > >> >> > >> >> auto with_plus = std::string ("+") + token; > >> >> > >> >> > + if (aarch64_handle_attr_isa_flags (str)) > >> >> > + error("arch extension %<%s%> should be prepended with %<+%>", token); > >> >> > >> >> Nit: should be a space before the “(”. > >> >> > >> >> In principle, a fixit hint would have been nice here, but I don't think > >> >> we have enough information to provide one. (Just saying for the record.) > >> > Thanks for the suggestions. > >> > Does the attached patch look OK ? > >> > >> Looks good apart from a couple of formatting nits. > >> > > >> > Thanks, > >> > Prathamesh > >> >> > >> >> Thanks, > >> >> Richard > >> >> > >> >> > + else > >> >> > + error ("pragma or attribute %<target(\"%s\")%> is not valid", token); > >> >> > + free (str); > >> >> > return false; > >> >> > } > >> >> > > >> > > >> > [aarch64] PR102376 - Emit better diagnostics for arch extension in target attribute. > >> > > >> > gcc/ChangeLog: > >> > PR target/102376 > >> > * config/aarch64/aarch64.c (aarch64_handle_attr_isa_flags): Change str's > >> > type to const char *. > >> > (aarch64_process_target_attr): Check if token is possibly an arch extension > >> > without leading '+' and emit diagnostic accordingly. > >> > > >> > gcc/testsuite/ChangeLog: > >> > PR target/102376 > >> > * gcc.target/aarch64/pr102376.c: New test. > >> > diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c > >> > index a9a1800af53..b72079bc466 100644 > >> > --- a/gcc/config/aarch64/aarch64.c > >> > +++ b/gcc/config/aarch64/aarch64.c > >> > @@ -17548,7 +17548,7 @@ aarch64_handle_attr_tune (const char *str) > >> > modified. */ > >> > > >> > static bool > >> > -aarch64_handle_attr_isa_flags (char *str) > >> > +aarch64_handle_attr_isa_flags (const char *str) > >> > { > >> > enum aarch64_parse_opt_result parse_res; > >> > uint64_t isa_flags = aarch64_isa_flags; > >> > @@ -17821,7 +17821,13 @@ aarch64_process_target_attr (tree args) > >> > num_attrs++; > >> > if (!aarch64_process_one_target_attr (token)) > >> > { > >> > - error ("pragma or attribute %<target(\"%s\")%> is not valid", token); > >> > + /* Check if token is possibly an arch extension without > >> > + leading '+'. */ > >> > + auto with_plus = std::string("+") + token; > >> > >> Should be a space before “(”. > >> > >> > + if (aarch64_handle_attr_isa_flags (with_plus.c_str ())) > >> > + error ("arch extension %<%s%> should be prepended with %<+%>", token); > >> > >> Long line, should be: > >> > >> error ("arch extension %<%s%> should be prepended with %<+%>", > >> token); > >> > >> OK with those changes, thanks. > > Thanks, the patch regressed some target attr tests because it emitted > > diagnostics twice from > > aarch64_handle_attr_isa_flags. > > So for eg, spellcheck_1.c: > > __attribute__((target ("arch=armv8-a-typo"))) void foo () {} > > > > results in: > > spellcheck_1.c:5:1: error: invalid name ("armv8-a-typo") in > > ‘target("arch=")’ pragma or attribute > > 5 | { > > | ^ > > spellcheck_1.c:5:1: note: valid arguments are: armv8-a armv8.1-a > > armv8.2-a armv8.3-a armv8.4-a armv8.5-a armv8.6-a armv8.7-a armv8-r > > armv9-a > > spellcheck_1.c:5:1: error: invalid feature modifier arch=armv8-a-typo > > of value ("+arch=armv8-a-typo") in ‘target()’ pragma or attribute > > spellcheck_1.c:5:1: error: pragma or attribute > > ‘target("arch=armv8-a-typo")’ is not valid > > > > The patch adds an additional argument to the > > aarch64_handle_attr_isa_flags, to optionally not emit an error, which > > works to fix the issue. > > Does it look OK ? > > I think we should instead call aarch64_parse_arch directly, passing > temporary ISA flags instead of &aarch64_isa_flags. That should ensure > that the call has no side effects. > > I agree the new wording (in the later patch) is better, thanks. Thanks for the suggestions, does the attached patch look OK ? Thanks, Prathamesh > > Richard [-- Attachment #2: pr102376-6.txt --] [-- Type: text/plain, Size: 1292 bytes --] diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c index fd9249c62b3..218a7e06f68 100644 --- a/gcc/config/aarch64/aarch64.c +++ b/gcc/config/aarch64/aarch64.c @@ -17844,7 +17844,18 @@ aarch64_process_target_attr (tree args) num_attrs++; if (!aarch64_process_one_target_attr (token)) { - error ("pragma or attribute %<target(\"%s\")%> is not valid", token); + /* Check if token is possibly an arch extension without + leading '+'. */ + uint64_t isa_temp = 0; + auto with_plus = std::string ("+") + token; + enum aarch64_parse_opt_result ext_res + = aarch64_parse_extension (with_plus.c_str (), &isa_temp, nullptr); + + if (ext_res == AARCH64_PARSE_OK) + error ("arch extension %<%s%> should be prefixed by %<+%>", + token); + else + error ("pragma or attribute %<target(\"%s\")%> is not valid", token); return false; } diff --git a/gcc/testsuite/gcc.target/aarch64/pr102376.c b/gcc/testsuite/gcc.target/aarch64/pr102376.c new file mode 100644 index 00000000000..fc830ad4742 --- /dev/null +++ b/gcc/testsuite/gcc.target/aarch64/pr102376.c @@ -0,0 +1,3 @@ +/* { dg-do compile } */ + +void calculate(void) __attribute__ ((__target__ ("sve"))); /* { dg-error "arch extension 'sve' should be prefixed by '\\+'" } */ ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [aarch64] PR102376 - Emit better diagnostic for arch extensions in target attr 2021-11-08 10:33 ` Prathamesh Kulkarni @ 2021-11-09 14:57 ` Richard Sandiford 2021-11-11 9:15 ` Prathamesh Kulkarni 0 siblings, 1 reply; 12+ messages in thread From: Richard Sandiford @ 2021-11-09 14:57 UTC (permalink / raw) To: Prathamesh Kulkarni; +Cc: gcc Patches, Martin Liška Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> writes: > On Thu, 4 Nov 2021 at 14:19, Richard Sandiford > <richard.sandiford@arm.com> wrote: >> >> Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> writes: >> > On Wed, 20 Oct 2021 at 15:05, Richard Sandiford >> > <richard.sandiford@arm.com> wrote: >> >> >> >> Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> writes: >> >> > On Tue, 19 Oct 2021 at 19:58, Richard Sandiford >> >> > <richard.sandiford@arm.com> wrote: >> >> >> >> >> >> Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> writes: >> >> >> > Hi, >> >> >> > The attached patch emits a more verbose diagnostic for target attribute that >> >> >> > is an architecture extension needing a leading '+'. >> >> >> > >> >> >> > For the following test, >> >> >> > void calculate(void) __attribute__ ((__target__ ("sve"))); >> >> >> > >> >> >> > With patch, the compiler now emits: >> >> >> > 102376.c:1:1: error: arch extension ‘sve’ should be prepended with ‘+’ >> >> >> > 1 | void calculate(void) __attribute__ ((__target__ ("sve"))); >> >> >> > | ^~~~ >> >> >> > >> >> >> > instead of: >> >> >> > 102376.c:1:1: error: pragma or attribute ‘target("sve")’ is not valid >> >> >> > 1 | void calculate(void) __attribute__ ((__target__ ("sve"))); >> >> >> > | ^~~~ >> >> >> >> >> >> Nice :-) >> >> >> >> >> >> > (This isn't specific to sve though). >> >> >> > OK to commit after bootstrap+test ? >> >> >> > >> >> >> > Thanks, >> >> >> > Prathamesh >> >> >> > >> >> >> > diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c >> >> >> > index a9a1800af53..975f7faf968 100644 >> >> >> > --- a/gcc/config/aarch64/aarch64.c >> >> >> > +++ b/gcc/config/aarch64/aarch64.c >> >> >> > @@ -17821,7 +17821,16 @@ aarch64_process_target_attr (tree args) >> >> >> > num_attrs++; >> >> >> > if (!aarch64_process_one_target_attr (token)) >> >> >> > { >> >> >> > - error ("pragma or attribute %<target(\"%s\")%> is not valid", token); >> >> >> > + /* Check if token is possibly an arch extension without >> >> >> > + leading '+'. */ >> >> >> > + char *str = (char *) xmalloc (strlen (token) + 2); >> >> >> > + str[0] = '+'; >> >> >> > + strcpy(str + 1, token); >> >> >> >> >> >> I think std::string would be better here, e.g.: >> >> >> >> >> >> auto with_plus = std::string ("+") + token; >> >> >> >> >> >> > + if (aarch64_handle_attr_isa_flags (str)) >> >> >> > + error("arch extension %<%s%> should be prepended with %<+%>", token); >> >> >> >> >> >> Nit: should be a space before the “(”. >> >> >> >> >> >> In principle, a fixit hint would have been nice here, but I don't think >> >> >> we have enough information to provide one. (Just saying for the record.) >> >> > Thanks for the suggestions. >> >> > Does the attached patch look OK ? >> >> >> >> Looks good apart from a couple of formatting nits. >> >> > >> >> > Thanks, >> >> > Prathamesh >> >> >> >> >> >> Thanks, >> >> >> Richard >> >> >> >> >> >> > + else >> >> >> > + error ("pragma or attribute %<target(\"%s\")%> is not valid", token); >> >> >> > + free (str); >> >> >> > return false; >> >> >> > } >> >> >> > >> >> > >> >> > [aarch64] PR102376 - Emit better diagnostics for arch extension in target attribute. >> >> > >> >> > gcc/ChangeLog: >> >> > PR target/102376 >> >> > * config/aarch64/aarch64.c (aarch64_handle_attr_isa_flags): Change str's >> >> > type to const char *. >> >> > (aarch64_process_target_attr): Check if token is possibly an arch extension >> >> > without leading '+' and emit diagnostic accordingly. >> >> > >> >> > gcc/testsuite/ChangeLog: >> >> > PR target/102376 >> >> > * gcc.target/aarch64/pr102376.c: New test. >> >> > diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c >> >> > index a9a1800af53..b72079bc466 100644 >> >> > --- a/gcc/config/aarch64/aarch64.c >> >> > +++ b/gcc/config/aarch64/aarch64.c >> >> > @@ -17548,7 +17548,7 @@ aarch64_handle_attr_tune (const char *str) >> >> > modified. */ >> >> > >> >> > static bool >> >> > -aarch64_handle_attr_isa_flags (char *str) >> >> > +aarch64_handle_attr_isa_flags (const char *str) >> >> > { >> >> > enum aarch64_parse_opt_result parse_res; >> >> > uint64_t isa_flags = aarch64_isa_flags; >> >> > @@ -17821,7 +17821,13 @@ aarch64_process_target_attr (tree args) >> >> > num_attrs++; >> >> > if (!aarch64_process_one_target_attr (token)) >> >> > { >> >> > - error ("pragma or attribute %<target(\"%s\")%> is not valid", token); >> >> > + /* Check if token is possibly an arch extension without >> >> > + leading '+'. */ >> >> > + auto with_plus = std::string("+") + token; >> >> >> >> Should be a space before “(”. >> >> >> >> > + if (aarch64_handle_attr_isa_flags (with_plus.c_str ())) >> >> > + error ("arch extension %<%s%> should be prepended with %<+%>", token); >> >> >> >> Long line, should be: >> >> >> >> error ("arch extension %<%s%> should be prepended with %<+%>", >> >> token); >> >> >> >> OK with those changes, thanks. >> > Thanks, the patch regressed some target attr tests because it emitted >> > diagnostics twice from >> > aarch64_handle_attr_isa_flags. >> > So for eg, spellcheck_1.c: >> > __attribute__((target ("arch=armv8-a-typo"))) void foo () {} >> > >> > results in: >> > spellcheck_1.c:5:1: error: invalid name ("armv8-a-typo") in >> > ‘target("arch=")’ pragma or attribute >> > 5 | { >> > | ^ >> > spellcheck_1.c:5:1: note: valid arguments are: armv8-a armv8.1-a >> > armv8.2-a armv8.3-a armv8.4-a armv8.5-a armv8.6-a armv8.7-a armv8-r >> > armv9-a >> > spellcheck_1.c:5:1: error: invalid feature modifier arch=armv8-a-typo >> > of value ("+arch=armv8-a-typo") in ‘target()’ pragma or attribute >> > spellcheck_1.c:5:1: error: pragma or attribute >> > ‘target("arch=armv8-a-typo")’ is not valid >> > >> > The patch adds an additional argument to the >> > aarch64_handle_attr_isa_flags, to optionally not emit an error, which >> > works to fix the issue. >> > Does it look OK ? >> >> I think we should instead call aarch64_parse_arch directly, passing >> temporary ISA flags instead of &aarch64_isa_flags. That should ensure >> that the call has no side effects. >> >> I agree the new wording (in the later patch) is better, thanks. > Thanks for the suggestions, does the attached patch look OK ? Please remember to say how you tested patches. OK assuming it passed bootstrap & regression-test on aarch64-linux-gnu. Thanks, Richard > > Thanks, > Prathamesh >> >> Richard > > diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c > index fd9249c62b3..218a7e06f68 100644 > --- a/gcc/config/aarch64/aarch64.c > +++ b/gcc/config/aarch64/aarch64.c > @@ -17844,7 +17844,18 @@ aarch64_process_target_attr (tree args) > num_attrs++; > if (!aarch64_process_one_target_attr (token)) > { > - error ("pragma or attribute %<target(\"%s\")%> is not valid", token); > + /* Check if token is possibly an arch extension without > + leading '+'. */ > + uint64_t isa_temp = 0; > + auto with_plus = std::string ("+") + token; > + enum aarch64_parse_opt_result ext_res > + = aarch64_parse_extension (with_plus.c_str (), &isa_temp, nullptr); > + > + if (ext_res == AARCH64_PARSE_OK) > + error ("arch extension %<%s%> should be prefixed by %<+%>", > + token); > + else > + error ("pragma or attribute %<target(\"%s\")%> is not valid", token); > return false; > } > > diff --git a/gcc/testsuite/gcc.target/aarch64/pr102376.c b/gcc/testsuite/gcc.target/aarch64/pr102376.c > new file mode 100644 > index 00000000000..fc830ad4742 > --- /dev/null > +++ b/gcc/testsuite/gcc.target/aarch64/pr102376.c > @@ -0,0 +1,3 @@ > +/* { dg-do compile } */ > + > +void calculate(void) __attribute__ ((__target__ ("sve"))); /* { dg-error "arch extension 'sve' should be prefixed by '\\+'" } */ ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [aarch64] PR102376 - Emit better diagnostic for arch extensions in target attr 2021-11-09 14:57 ` Richard Sandiford @ 2021-11-11 9:15 ` Prathamesh Kulkarni 0 siblings, 0 replies; 12+ messages in thread From: Prathamesh Kulkarni @ 2021-11-11 9:15 UTC (permalink / raw) To: Prathamesh Kulkarni, gcc Patches, Martin Liška, richard.sandiford On Tue, 9 Nov 2021 at 20:27, Richard Sandiford <richard.sandiford@arm.com> wrote: > > Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> writes: > > On Thu, 4 Nov 2021 at 14:19, Richard Sandiford > > <richard.sandiford@arm.com> wrote: > >> > >> Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> writes: > >> > On Wed, 20 Oct 2021 at 15:05, Richard Sandiford > >> > <richard.sandiford@arm.com> wrote: > >> >> > >> >> Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> writes: > >> >> > On Tue, 19 Oct 2021 at 19:58, Richard Sandiford > >> >> > <richard.sandiford@arm.com> wrote: > >> >> >> > >> >> >> Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> writes: > >> >> >> > Hi, > >> >> >> > The attached patch emits a more verbose diagnostic for target attribute that > >> >> >> > is an architecture extension needing a leading '+'. > >> >> >> > > >> >> >> > For the following test, > >> >> >> > void calculate(void) __attribute__ ((__target__ ("sve"))); > >> >> >> > > >> >> >> > With patch, the compiler now emits: > >> >> >> > 102376.c:1:1: error: arch extension ‘sve’ should be prepended with ‘+’ > >> >> >> > 1 | void calculate(void) __attribute__ ((__target__ ("sve"))); > >> >> >> > | ^~~~ > >> >> >> > > >> >> >> > instead of: > >> >> >> > 102376.c:1:1: error: pragma or attribute ‘target("sve")’ is not valid > >> >> >> > 1 | void calculate(void) __attribute__ ((__target__ ("sve"))); > >> >> >> > | ^~~~ > >> >> >> > >> >> >> Nice :-) > >> >> >> > >> >> >> > (This isn't specific to sve though). > >> >> >> > OK to commit after bootstrap+test ? > >> >> >> > > >> >> >> > Thanks, > >> >> >> > Prathamesh > >> >> >> > > >> >> >> > diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c > >> >> >> > index a9a1800af53..975f7faf968 100644 > >> >> >> > --- a/gcc/config/aarch64/aarch64.c > >> >> >> > +++ b/gcc/config/aarch64/aarch64.c > >> >> >> > @@ -17821,7 +17821,16 @@ aarch64_process_target_attr (tree args) > >> >> >> > num_attrs++; > >> >> >> > if (!aarch64_process_one_target_attr (token)) > >> >> >> > { > >> >> >> > - error ("pragma or attribute %<target(\"%s\")%> is not valid", token); > >> >> >> > + /* Check if token is possibly an arch extension without > >> >> >> > + leading '+'. */ > >> >> >> > + char *str = (char *) xmalloc (strlen (token) + 2); > >> >> >> > + str[0] = '+'; > >> >> >> > + strcpy(str + 1, token); > >> >> >> > >> >> >> I think std::string would be better here, e.g.: > >> >> >> > >> >> >> auto with_plus = std::string ("+") + token; > >> >> >> > >> >> >> > + if (aarch64_handle_attr_isa_flags (str)) > >> >> >> > + error("arch extension %<%s%> should be prepended with %<+%>", token); > >> >> >> > >> >> >> Nit: should be a space before the “(”. > >> >> >> > >> >> >> In principle, a fixit hint would have been nice here, but I don't think > >> >> >> we have enough information to provide one. (Just saying for the record.) > >> >> > Thanks for the suggestions. > >> >> > Does the attached patch look OK ? > >> >> > >> >> Looks good apart from a couple of formatting nits. > >> >> > > >> >> > Thanks, > >> >> > Prathamesh > >> >> >> > >> >> >> Thanks, > >> >> >> Richard > >> >> >> > >> >> >> > + else > >> >> >> > + error ("pragma or attribute %<target(\"%s\")%> is not valid", token); > >> >> >> > + free (str); > >> >> >> > return false; > >> >> >> > } > >> >> >> > > >> >> > > >> >> > [aarch64] PR102376 - Emit better diagnostics for arch extension in target attribute. > >> >> > > >> >> > gcc/ChangeLog: > >> >> > PR target/102376 > >> >> > * config/aarch64/aarch64.c (aarch64_handle_attr_isa_flags): Change str's > >> >> > type to const char *. > >> >> > (aarch64_process_target_attr): Check if token is possibly an arch extension > >> >> > without leading '+' and emit diagnostic accordingly. > >> >> > > >> >> > gcc/testsuite/ChangeLog: > >> >> > PR target/102376 > >> >> > * gcc.target/aarch64/pr102376.c: New test. > >> >> > diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c > >> >> > index a9a1800af53..b72079bc466 100644 > >> >> > --- a/gcc/config/aarch64/aarch64.c > >> >> > +++ b/gcc/config/aarch64/aarch64.c > >> >> > @@ -17548,7 +17548,7 @@ aarch64_handle_attr_tune (const char *str) > >> >> > modified. */ > >> >> > > >> >> > static bool > >> >> > -aarch64_handle_attr_isa_flags (char *str) > >> >> > +aarch64_handle_attr_isa_flags (const char *str) > >> >> > { > >> >> > enum aarch64_parse_opt_result parse_res; > >> >> > uint64_t isa_flags = aarch64_isa_flags; > >> >> > @@ -17821,7 +17821,13 @@ aarch64_process_target_attr (tree args) > >> >> > num_attrs++; > >> >> > if (!aarch64_process_one_target_attr (token)) > >> >> > { > >> >> > - error ("pragma or attribute %<target(\"%s\")%> is not valid", token); > >> >> > + /* Check if token is possibly an arch extension without > >> >> > + leading '+'. */ > >> >> > + auto with_plus = std::string("+") + token; > >> >> > >> >> Should be a space before “(”. > >> >> > >> >> > + if (aarch64_handle_attr_isa_flags (with_plus.c_str ())) > >> >> > + error ("arch extension %<%s%> should be prepended with %<+%>", token); > >> >> > >> >> Long line, should be: > >> >> > >> >> error ("arch extension %<%s%> should be prepended with %<+%>", > >> >> token); > >> >> > >> >> OK with those changes, thanks. > >> > Thanks, the patch regressed some target attr tests because it emitted > >> > diagnostics twice from > >> > aarch64_handle_attr_isa_flags. > >> > So for eg, spellcheck_1.c: > >> > __attribute__((target ("arch=armv8-a-typo"))) void foo () {} > >> > > >> > results in: > >> > spellcheck_1.c:5:1: error: invalid name ("armv8-a-typo") in > >> > ‘target("arch=")’ pragma or attribute > >> > 5 | { > >> > | ^ > >> > spellcheck_1.c:5:1: note: valid arguments are: armv8-a armv8.1-a > >> > armv8.2-a armv8.3-a armv8.4-a armv8.5-a armv8.6-a armv8.7-a armv8-r > >> > armv9-a > >> > spellcheck_1.c:5:1: error: invalid feature modifier arch=armv8-a-typo > >> > of value ("+arch=armv8-a-typo") in ‘target()’ pragma or attribute > >> > spellcheck_1.c:5:1: error: pragma or attribute > >> > ‘target("arch=armv8-a-typo")’ is not valid > >> > > >> > The patch adds an additional argument to the > >> > aarch64_handle_attr_isa_flags, to optionally not emit an error, which > >> > works to fix the issue. > >> > Does it look OK ? > >> > >> I think we should instead call aarch64_parse_arch directly, passing > >> temporary ISA flags instead of &aarch64_isa_flags. That should ensure > >> that the call has no side effects. > >> > >> I agree the new wording (in the later patch) is better, thanks. > > Thanks for the suggestions, does the attached patch look OK ? > > Please remember to say how you tested patches. Right, sorry will do henceforth. > > OK assuming it passed bootstrap & regression-test on aarch64-linux-gnu. Thanks, committed as 145be5efaf5674a7d25c723dc5684392a6276834 after bootstrap+test on aarch64-linux-gnu. Thanks, Prathamesh > > Thanks, > Richard > > > > > Thanks, > > Prathamesh > >> > >> Richard > > > > diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c > > index fd9249c62b3..218a7e06f68 100644 > > --- a/gcc/config/aarch64/aarch64.c > > +++ b/gcc/config/aarch64/aarch64.c > > @@ -17844,7 +17844,18 @@ aarch64_process_target_attr (tree args) > > num_attrs++; > > if (!aarch64_process_one_target_attr (token)) > > { > > - error ("pragma or attribute %<target(\"%s\")%> is not valid", token); > > + /* Check if token is possibly an arch extension without > > + leading '+'. */ > > + uint64_t isa_temp = 0; > > + auto with_plus = std::string ("+") + token; > > + enum aarch64_parse_opt_result ext_res > > + = aarch64_parse_extension (with_plus.c_str (), &isa_temp, nullptr); > > + > > + if (ext_res == AARCH64_PARSE_OK) > > + error ("arch extension %<%s%> should be prefixed by %<+%>", > > + token); > > + else > > + error ("pragma or attribute %<target(\"%s\")%> is not valid", token); > > return false; > > } > > > > diff --git a/gcc/testsuite/gcc.target/aarch64/pr102376.c b/gcc/testsuite/gcc.target/aarch64/pr102376.c > > new file mode 100644 > > index 00000000000..fc830ad4742 > > --- /dev/null > > +++ b/gcc/testsuite/gcc.target/aarch64/pr102376.c > > @@ -0,0 +1,3 @@ > > +/* { dg-do compile } */ > > + > > +void calculate(void) __attribute__ ((__target__ ("sve"))); /* { dg-error "arch extension 'sve' should be prefixed by '\\+'" } */ ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2021-11-11 9:16 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-10-18 11:21 [aarch64] PR102376 - Emit better diagnostic for arch extensions in target attr Prathamesh Kulkarni 2021-10-19 14:28 ` Richard Sandiford 2021-10-20 7:08 ` Prathamesh Kulkarni 2021-10-20 9:35 ` Richard Sandiford 2021-10-22 9:11 ` Prathamesh Kulkarni 2021-10-28 8:59 ` Prathamesh Kulkarni 2021-10-28 16:03 ` Martin Sebor 2021-11-04 7:04 ` Prathamesh Kulkarni 2021-11-04 8:49 ` Richard Sandiford 2021-11-08 10:33 ` Prathamesh Kulkarni 2021-11-09 14:57 ` Richard Sandiford 2021-11-11 9:15 ` Prathamesh Kulkarni
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).