From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by sourceware.org (Postfix) with ESMTP id 64D2A3858C3A for ; Thu, 4 Nov 2021 08:49:18 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 64D2A3858C3A Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 09CB31FB; Thu, 4 Nov 2021 01:49:18 -0700 (PDT) Received: from localhost (unknown [10.32.98.88]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 68DAF3F719; Thu, 4 Nov 2021 01:49:17 -0700 (PDT) From: Richard Sandiford To: Prathamesh Kulkarni Mail-Followup-To: Prathamesh Kulkarni ,gcc Patches , Martin =?utf-8?Q?Li=C5=A1ka?= , richard.sandiford@arm.com Cc: gcc Patches , Martin =?utf-8?Q?Li=C5=A1ka?= Subject: Re: [aarch64] PR102376 - Emit better diagnostic for arch extensions in target attr References: Date: Thu, 04 Nov 2021 08:49:16 +0000 In-Reply-To: (Prathamesh Kulkarni's message of "Fri, 22 Oct 2021 14:41:58 +0530") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-12.4 required=5.0 tests=BAYES_00, GIT_PATCH_0, KAM_DMARC_STATUS, KAM_SHORT, 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 04 Nov 2021 08:49:20 -0000 Prathamesh Kulkarni writes: > On Wed, 20 Oct 2021 at 15:05, Richard Sandiford > wrote: >> >> Prathamesh Kulkarni writes: >> > On Tue, 19 Oct 2021 at 19:58, Richard Sandiford >> > wrote: >> >> >> >> Prathamesh Kulkarni writes: >> >> > Hi, >> >> > The attached patch emits a more verbose diagnostic for target attri= bute 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 =E2=80=98sve=E2=80=99 should be= prepended with =E2=80=98+=E2=80=99 >> >> > 1 | void calculate(void) __attribute__ ((__target__ ("sve"))); >> >> > | ^~~~ >> >> > >> >> > instead of: >> >> > 102376.c:1:1: error: pragma or attribute =E2=80=98target("sve")=E2= =80=99 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/aarc= h64.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 % is not valid= ", token); >> >> > + /* Check if token is possibly an arch extension without >> >> > + leading '+'. */ >> >> > + char *str =3D (char *) xmalloc (strlen (token) + 2); >> >> > + str[0] =3D '+'; >> >> > + strcpy(str + 1, token); >> >> >> >> I think std::string would be better here, e.g.: >> >> >> >> auto with_plus =3D 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 =E2=80=9C(=E2=80=9D. >> >> >> >> In principle, a fixit hint would have been nice here, but I don't thi= nk >> >> we have enough information to provide one. (Just saying for the reco= rd.) >> > 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 % is not val= id", token); >> >> > + free (str); >> >> > return false; >> >> > } >> >> > >> > >> > [aarch64] PR102376 - Emit better diagnostics for arch extension in tar= get attribute. >> > >> > gcc/ChangeLog: >> > PR target/102376 >> > * config/aarch64/aarch64.c (aarch64_handle_attr_isa_flags): Chan= ge str's >> > type to const char *. >> > (aarch64_process_target_attr): Check if token is possibly an arc= h 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 =3D 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 % is not valid", = token); >> > + /* Check if token is possibly an arch extension without >> > + leading '+'. */ >> > + auto with_plus =3D std::string("+") + token; >> >> Should be a space before =E2=80=9C(=E2=80=9D. >> >> > + 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=3Darmv8-a-typo"))) void foo () {} > > results in: > spellcheck_1.c:5:1: error: invalid name ("armv8-a-typo") in > =E2=80=98target("arch=3D")=E2=80=99 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=3Darmv8-a-typo > of value ("+arch=3Darmv8-a-typo") in =E2=80=98target()=E2=80=99 pragma or= attribute > spellcheck_1.c:5:1: error: pragma or attribute > =E2=80=98target("arch=3Darmv8-a-typo")=E2=80=99 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