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