On Fri, 22 Oct 2021 at 14:41, Prathamesh Kulkarni <prathamesh.kulka...@linaro.org> wrote: > > On Wed, 20 Oct 2021 at 15:05, Richard Sandiford > <richard.sandif...@arm.com> wrote: > > > > Prathamesh Kulkarni <prathamesh.kulka...@linaro.org> writes: > > > On Tue, 19 Oct 2021 at 19:58, Richard Sandiford > > > <richard.sandif...@arm.com> wrote: > > >> > > >> Prathamesh Kulkarni <prathamesh.kulka...@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 '\\+'" } */