On Thu, 28 Oct 2021 at 21:33, Martin Sebor <mse...@gmail.com> wrote:
>
> On 10/28/21 2:59 AM, Prathamesh Kulkarni via Gcc-patches wrote:
> > On Fri, 22 Oct 2021 at 14:41, Prathamesh Kulkarni
> > <prathamesh.kulka...@linaro.org> wrote:
> >>
> >> 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 ?
> > 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 %<+%>"
Hi Martin,
Thanks for the suggestions and sorry for late response.
The attached patch updates the diagnostic to use "prefixed by" instead.
Richard, is this patch OK to commit after bootstrap+test ?
Thanks,
Prathamesh
>
> 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 '\\+'" } */
>
diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index fd9249c62b3..b449241f6bd 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -17571,7 +17571,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, bool emit_diagnostic = true)
{
enum aarch64_parse_opt_result parse_res;
uint64_t isa_flags = aarch64_isa_flags;
@@ -17593,6 +17593,9 @@ aarch64_handle_attr_isa_flags (char *str)
return true;
}
+ if (!emit_diagnostic)
+ return false;
+
switch (parse_res)
{
case AARCH64_PARSE_MISSING_ARG:
@@ -17844,7 +17847,14 @@ 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;
+ if (aarch64_handle_attr_isa_flags (with_plus.c_str (), false))
+ 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 '\\+'" } */
