On Fri, 22 Oct 2021 at 14:41, Prathamesh Kulkarni
<[email protected]> wrote:
>
> On Wed, 20 Oct 2021 at 15:05, Richard Sandiford
> <[email protected]> wrote:
> >
> > Prathamesh Kulkarni <[email protected]> writes:
> > > On Tue, 19 Oct 2021 at 19:58, Richard Sandiford
> > > <[email protected]> wrote:
> > >>
> > >> Prathamesh Kulkarni <[email protected]> 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 '\\+'" } */
