On Tue, 9 Nov 2021 at 20:27, Richard Sandiford
<richard.sandif...@arm.com> wrote:
>
> Prathamesh Kulkarni <prathamesh.kulka...@linaro.org> writes:
> > On Thu, 4 Nov 2021 at 14:19, Richard Sandiford
> > <richard.sandif...@arm.com> wrote:
> >>
> >> Prathamesh Kulkarni <prathamesh.kulka...@linaro.org> writes:
> >> > 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 ?
> >>
> >> 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 '\\+'" } */
