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

[Richard Sandiford <richard.sandiford@arm.com>]

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.

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