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 '\\+'" } */