From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: 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 98C6D3858401 for ; Mon, 8 Nov 2021 10:34:13 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 98C6D3858401 Received: by mail-ed1-x52f.google.com with SMTP id r12so60467197edt.6 for ; Mon, 08 Nov 2021 02:34:13 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to; bh=pbF8u8nIU4x61I0GMcdygx9zpd4G/Ukdlbq3MiIityY=; b=lKMdGKrwML3AEkQGtxckTCM6QRMQQ96KrQsTHdQ/IXI8KFfbRTRPcqqeeu7Tzl3Soq hpZycvcGuulbZZtH/XPWguoSpbtdYUuiFCzg4r8AkTz7VrtO5asxO1Ih+MYp17rbFv80 XHWcfAwCTB/vWoGjsM547JPX7PNFgMKS3hcbBxAVR7RIdL/Hg+f07OlUUimACCYK2T/y i7PvkOzHfrtwkYKaIod+AljT+TWjx+pUPT8VodHLBBtirTiOQcCCFlLNxRqLhH+f94ea oV/hxUv8xuxp2ij7HKxjpWqcJ3gKEd83JJvGfLUwjX4DPHefbicMw5O1+5axLL51IVdu cDDA== X-Gm-Message-State: AOAM531S6ugAPvmnd2EqUpeAykVI/0tX1OPxVt9JS642cJjvpfWrt3UD 8Vcp7lzoEq8lgms7ne/rpss7buye+MX1y/u4K+Fmlg== X-Google-Smtp-Source: ABdhPJymoeK8uPv8JZb8SmC4lfuvk6vhnqHstE7Rs9PlG6UL5oip4ywsIoV63hlHOPZoZ+JcTWRHSYo5GywW9km5sto= X-Received: by 2002:a17:907:a088:: with SMTP id hu8mr1882953ejc.234.1636367652435; Mon, 08 Nov 2021 02:34:12 -0800 (PST) MIME-Version: 1.0 References: In-Reply-To: From: Prathamesh Kulkarni Date: Mon, 8 Nov 2021 16:03:37 +0530 Message-ID: Subject: Re: [aarch64] PR102376 - Emit better diagnostic for arch extensions in target attr To: Prathamesh Kulkarni , gcc Patches , =?UTF-8?Q?Martin_Li=C5=A1ka?= , richard.sandiford@arm.com Content-Type: multipart/mixed; boundary="000000000000a5955405d04488c2" X-Spam-Status: No, score=-8.7 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 08 Nov 2021 10:34:16 -0000 --000000000000a5955405d04488c2 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Thu, 4 Nov 2021 at 14:19, Richard Sandiford wrote: > > 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 att= ribute 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/aa= rch64.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 val= id", 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 t= hink > >> >> we have enough information to provide one. (Just saying for the re= cord.) > >> > 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 v= alid", token); > >> >> > + free (str); > >> >> > return false; > >> >> > } > >> >> > > >> > > >> > [aarch64] PR102376 - Emit better diagnostics for arch extension in t= arget attribute. > >> > > >> > gcc/ChangeLog: > >> > PR target/102376 > >> > * config/aarch64/aarch64.c (aarch64_handle_attr_isa_flags): Ch= ange str's > >> > type to const char *. > >> > (aarch64_process_target_attr): Check if token is possibly an a= rch 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/aarch= 64.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. Thanks for the suggestions, does the attached patch look OK ? Thanks, Prathamesh > > Richard --000000000000a5955405d04488c2 Content-Type: text/plain; charset="US-ASCII"; name="pr102376-6.txt" Content-Disposition: attachment; filename="pr102376-6.txt" Content-Transfer-Encoding: base64 Content-ID: X-Attachment-Id: f_kvqiwqbv0 ZGlmZiAtLWdpdCBhL2djYy9jb25maWcvYWFyY2g2NC9hYXJjaDY0LmMgYi9nY2MvY29uZmlnL2Fh cmNoNjQvYWFyY2g2NC5jCmluZGV4IGZkOTI0OWM2MmIzLi4yMThhN2UwNmY2OCAxMDA2NDQKLS0t IGEvZ2NjL2NvbmZpZy9hYXJjaDY0L2FhcmNoNjQuYworKysgYi9nY2MvY29uZmlnL2FhcmNoNjQv YWFyY2g2NC5jCkBAIC0xNzg0NCw3ICsxNzg0NCwxOCBAQCBhYXJjaDY0X3Byb2Nlc3NfdGFyZ2V0 X2F0dHIgKHRyZWUgYXJncykKICAgICAgIG51bV9hdHRycysrOwogICAgICAgaWYgKCFhYXJjaDY0 X3Byb2Nlc3Nfb25lX3RhcmdldF9hdHRyICh0b2tlbikpCiAJewotCSAgZXJyb3IgKCJwcmFnbWEg b3IgYXR0cmlidXRlICU8dGFyZ2V0KFwiJXNcIiklPiBpcyBub3QgdmFsaWQiLCB0b2tlbik7CisJ ICAvKiBDaGVjayBpZiB0b2tlbiBpcyBwb3NzaWJseSBhbiBhcmNoIGV4dGVuc2lvbiB3aXRob3V0 CisJICAgICBsZWFkaW5nICcrJy4gICovCisJICB1aW50NjRfdCBpc2FfdGVtcCA9IDA7CisJICBh dXRvIHdpdGhfcGx1cyA9IHN0ZDo6c3RyaW5nICgiKyIpICsgdG9rZW47CisJICBlbnVtIGFhcmNo NjRfcGFyc2Vfb3B0X3Jlc3VsdCBleHRfcmVzCisJICAgID0gYWFyY2g2NF9wYXJzZV9leHRlbnNp b24gKHdpdGhfcGx1cy5jX3N0ciAoKSwgJmlzYV90ZW1wLCBudWxscHRyKTsKKworCSAgaWYgKGV4 dF9yZXMgPT0gQUFSQ0g2NF9QQVJTRV9PSykKKwkgICAgZXJyb3IgKCJhcmNoIGV4dGVuc2lvbiAl PCVzJT4gc2hvdWxkIGJlIHByZWZpeGVkIGJ5ICU8KyU+IiwKKwkJICAgdG9rZW4pOworCSAgZWxz ZQorCSAgICBlcnJvciAoInByYWdtYSBvciBhdHRyaWJ1dGUgJTx0YXJnZXQoXCIlc1wiKSU+IGlz IG5vdCB2YWxpZCIsIHRva2VuKTsKIAkgIHJldHVybiBmYWxzZTsKIAl9CiAKZGlmZiAtLWdpdCBh L2djYy90ZXN0c3VpdGUvZ2NjLnRhcmdldC9hYXJjaDY0L3ByMTAyMzc2LmMgYi9nY2MvdGVzdHN1 aXRlL2djYy50YXJnZXQvYWFyY2g2NC9wcjEwMjM3Ni5jCm5ldyBmaWxlIG1vZGUgMTAwNjQ0Cmlu ZGV4IDAwMDAwMDAwMDAwLi5mYzgzMGFkNDc0MgotLS0gL2Rldi9udWxsCisrKyBiL2djYy90ZXN0 c3VpdGUvZ2NjLnRhcmdldC9hYXJjaDY0L3ByMTAyMzc2LmMKQEAgLTAsMCArMSwzIEBACisvKiB7 IGRnLWRvIGNvbXBpbGUgfSAqLworCit2b2lkIGNhbGN1bGF0ZSh2b2lkKSBfX2F0dHJpYnV0ZV9f ICgoX190YXJnZXRfXyAoInN2ZSIpKSk7IC8qIHsgZGctZXJyb3IgImFyY2ggZXh0ZW5zaW9uICdz dmUnIHNob3VsZCBiZSBwcmVmaXhlZCBieSAnXFwrJyIgfSAqLwo= --000000000000a5955405d04488c2--