[aarch64] PR102376 - Emit better diagnostic for arch extensions in target attr

Richard Sandiford richard.sandiford@arm.com
Tue Nov 9 14:57:37 GMT 2021


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


More information about the Gcc-patches mailing list