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

[Martin Sebor <msebor@gmail.com>]

On 10/28/21 2:59 AM, Prathamesh Kulkarni via Gcc-patches wrote:
> On Fri, 22 Oct 2021 at 14:41, Prathamesh Kulkarni
> <prathamesh.kulkarni@linaro.org> wrote:
>>
>> 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 ?
> ping https://gcc.gnu.org/pipermail/gcc-patches/2021-October/582345.html

Just a couple of minor points:

+	  if (aarch64_handle_attr_isa_flags (with_plus.c_str (), false))
+	    error ("arch extension %<%s%> should be prepended with %<+%>",
+		   token);

The phrase is to prepend something or prepend it to something,
but usually not to "prepend with."  In this context I think
either "prefixed by" or "preceded by" would be correct.

Also, although there are several other pre-existing uses,
the abbreviation arch is not an English word that lends itself
to translation.  Judging by the German and French .po files,
their authors take the trouble of spelling it out, but others
such as Spanish or Russian don't, leaving it as is, presumably
because they're not sure whether it's supposed to be translated.
In the Russian translation it looks especially odd rendered in
the latin alphabet in the middle of a sentence in Cyrillic.
I think we should follow the German and French translators'
lead and spell it out in English as well.

   "architecture extension %<%s%> should be prefixed by %<+%>"

Martin

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