Message ID | CAAgBjMmAWeRqHtWtzuvBtFpd5btXY7R71uEGAZYt4ydVuWqStQ@mail.gmail.com |
---|---|
State | Accepted |
Headers |
Return-Path: <gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org> X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 285CC385800D for <patchwork@sourceware.org>; Mon, 18 Oct 2021 11:22:15 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 285CC385800D DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1634556135; bh=U88QgWT7qtKMhwpxRdDiLj+HWpjPTAell7gDs/t0qvA=; h=Date:Subject:To:List-Id:List-Unsubscribe:List-Archive:List-Post: List-Help:List-Subscribe:From:Reply-To:From; b=f3lvqIx9RJ56UQhkVtpydpojDLtuf5B+WnPzp2iyXc96zYcRrvsCLB+GXIBc8XP5U +H4sGcJ13sr0ooqH4TufuL6Y4H0VPc8ML3D/bYkdnobra6aQmaJWuVq0mbGntLOrjp caCfC4+sMFsufAP3Vj1d9JLsVJBeVuS2amr+L4PQ= X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from mail-ed1-x52f.google.com (mail-ed1-x52f.google.com [IPv6:2a00:1450:4864:20::52f]) by sourceware.org (Postfix) with ESMTPS id 64CF63858402 for <gcc-patches@gcc.gnu.org>; Mon, 18 Oct 2021 11:21:41 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 64CF63858402 Received: by mail-ed1-x52f.google.com with SMTP id g10so69803750edj.1 for <gcc-patches@gcc.gnu.org>; Mon, 18 Oct 2021 04:21:41 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:from:date:message-id:subject:to; bh=U88QgWT7qtKMhwpxRdDiLj+HWpjPTAell7gDs/t0qvA=; b=r2M68uW5WRWio8T+crofUVeq+aDqZMX2cNaL52uGILTHkvonltivsFlskebWWSNjL0 SpmwqCTx+cd7U7a5ObgvcCCAT1kQMJTOpz584c7ymlZlVlspeZ2fyj0E/BCaMIXeWOS/ Rti5L9/MdT51Us/mP4heLqBgNVB2jOCrpF4aFcMykTyv7t+MyhANKgDP06lPQraJOdfj 6EE9WnibjPSCMsrk3il7a9Zsse/H67GsZhh/QmzKNZ/UymYJohBc0H3xfwxuV6jqhmdu EJ+5XVhzlG6KkPX3iwVg2qMaDkg4GxEzhBvPgZA5EmFvuQ8BWiwey0hEZmp0csh3DU/J WQFQ== X-Gm-Message-State: AOAM530YejB7byHe+6GqwlpslABLlF9vrGE8FTes5uBfJdNq2AJnH5v5 KeyEoRfS2fAcATkxiVIntZX+UbbQ7mCEotT7HrCegJwaZT5g1g== X-Google-Smtp-Source: ABdhPJwSSBW75Gia0O8nNtYYzAWn0mbU7Bmz+DdaBi9RRDuOJ6CrTFMvvZBrOZUpc4g4UDjc8XY2iO8mps564Ka0ZaI= X-Received: by 2002:a17:906:a08d:: with SMTP id q13mr28490249ejy.465.1634556100129; Mon, 18 Oct 2021 04:21:40 -0700 (PDT) MIME-Version: 1.0 Date: Mon, 18 Oct 2021 16:51:03 +0530 Message-ID: <CAAgBjMmAWeRqHtWtzuvBtFpd5btXY7R71uEGAZYt4ydVuWqStQ@mail.gmail.com> Subject: [aarch64] PR102376 - Emit better diagnostic for arch extensions in target attr To: gcc Patches <gcc-patches@gcc.gnu.org>, =?utf-8?q?Martin_Li=C5=A1ka?= <mliska@suse.cz>, Richard Sandiford <richard.sandiford@arm.com> Content-Type: multipart/mixed; boundary="000000000000b713c905ce9ebfe9" X-Spam-Status: No, score=-8.5 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list <gcc-patches.gcc.gnu.org> List-Unsubscribe: <https://gcc.gnu.org/mailman/options/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=unsubscribe> List-Archive: <https://gcc.gnu.org/pipermail/gcc-patches/> List-Post: <mailto:gcc-patches@gcc.gnu.org> List-Help: <mailto:gcc-patches-request@gcc.gnu.org?subject=help> List-Subscribe: <https://gcc.gnu.org/mailman/listinfo/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=subscribe> From: Prathamesh Kulkarni via Gcc-patches <gcc-patches@gcc.gnu.org> Reply-To: Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> Errors-To: gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org Sender: "Gcc-patches" <gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org> |
Series |
[aarch64] PR102376 - Emit better diagnostic for arch extensions in target attr
|
|
Commit Message
Prathamesh Kulkarni
Oct. 18, 2021, 11:21 a.m. UTC
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"))); | ^~~~ (This isn't specific to sve though). OK to commit after bootstrap+test ? Thanks, Prathamesh
Comments
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, Richard > + else > + error ("pragma or attribute %<target(\"%s\")%> is not valid", token); > + free (str); > return false; > } >
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 ? 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; + if (aarch64_handle_attr_isa_flags (with_plus.c_str ())) + error ("arch extension %<%s%> should be prepended with %<+%>", 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..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 '\\+'" } */
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 '\\+'" } */
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 ? 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 a9a1800af53..cf345e05fbc 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, bool emit_diagnostic = true) { enum aarch64_parse_opt_result parse_res; uint64_t isa_flags = aarch64_isa_flags; @@ -17570,6 +17570,9 @@ aarch64_handle_attr_isa_flags (char *str) return true; } + if (!emit_diagnostic) + return false; + switch (parse_res) { case AARCH64_PARSE_MISSING_ARG: @@ -17821,7 +17824,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 prepended with %<+%>", + 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..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 '\\+'" } */
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 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 '\\+'" } */
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 '\\+'" } */
On Thu, 28 Oct 2021 at 21:33, Martin Sebor <msebor@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.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 %<+%>" 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 '\\+'" } */
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. Richard
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 ? 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 '\\+'" } */
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 '\\+'" } */
On Tue, 9 Nov 2021 at 20:27, Richard Sandiford <richard.sandiford@arm.com> wrote: > > 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. Right, sorry will do henceforth. > > OK assuming it passed bootstrap & regression-test on aarch64-linux-gnu. Thanks, committed as 145be5efaf5674a7d25c723dc5684392a6276834 after bootstrap+test on aarch64-linux-gnu. Thanks, Prathamesh > > 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 '\\+'" } */
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); + if (aarch64_handle_attr_isa_flags (str)) + error("arch extension %<%s%> should be prepended with %<+%>", token); + else + error ("pragma or attribute %<target(\"%s\")%> is not valid", token); + free (str); return false; }