From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ed1-x52c.google.com (mail-ed1-x52c.google.com [IPv6:2a00:1450:4864:20::52c]) by sourceware.org (Postfix) with ESMTPS id B74173857C51 for ; Thu, 4 Nov 2021 07:05:07 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org B74173857C51 Received: by mail-ed1-x52c.google.com with SMTP id c8so1230355ede.13 for ; Thu, 04 Nov 2021 00:05:07 -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:references:in-reply-to:from:date :message-id:subject:to:cc; bh=uNhGRExTXF5GH3yt2OQw7iMTfUwTDjghsNgJjXMHv8U=; b=0ShLk8tmb73rhgYaXGO2rrFjAZZTgBL3mnWw+VjVRxzbBbgZAu6waZmBrCiCnwKiB3 LpNeJXKpBmW1ikjEYiQEY1v8lqLNRnfeUce0lASofp+VM2OcBnnuhlKI4Ae1d4w1dhG/ qTmJpk6NDNFobvRVLP6SlcdObC0c0g0OWh1VJvVcVij6D6UaX0g2COyWLnjynkqtVUN5 n+AMVPO1UK4TJaQwKcbNnR3kjaDDiu6onqqDPgDd80YoOfLFCl1uYuSVsSboazYxsYyL MMuBEx+bF7wvKyhcpYtfgaXrnVZNJDrJL8v0U7ZXWGyap4GWGhL3J2h+4RK5uuNF8jqX SQKg== X-Gm-Message-State: AOAM531P7jJRaQZnL5VU5vT0ohS+f5Ofy8rtnHGAkIGAHC3ATFtycgWf q24IM5sdJQWmKK0mpQdyK3krioi2eFSqYznoUBjKkQ== X-Google-Smtp-Source: ABdhPJxrZSOnjQC37wly2y0edWWCfEn8wAJC44srEhLrU/gwwBt1cXqGjN8GDQQFxGln2DSAinfKKalj1zcYi6bwtJ8= X-Received: by 2002:a05:6402:5190:: with SMTP id q16mr28535822edd.12.1636009506564; Thu, 04 Nov 2021 00:05:06 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: Prathamesh Kulkarni Date: Thu, 4 Nov 2021 12:34:33 +0530 Message-ID: Subject: Re: [aarch64] PR102376 - Emit better diagnostic for arch extensions in target attr To: Martin Sebor Cc: gcc Patches , =?UTF-8?Q?Martin_Li=C5=A1ka?= , richard.sandiford@arm.com Content-Type: multipart/mixed; boundary="0000000000007d226705cff125a9" X-Spam-Status: No, score=-8.9 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: Thu, 04 Nov 2021 07:05:10 -0000 --0000000000007d226705cff125a9 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Thu, 28 Oct 2021 at 21:33, Martin Sebor wrote: > > On 10/28/21 2:59 AM, Prathamesh Kulkarni via Gcc-patches wrote: > > On Fri, 22 Oct 2021 at 14:41, Prathamesh Kulkarni > > wrote: > >> > >> 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 attr= ibute 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 b= e 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/aar= ch64.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 vali= d", 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 va= lid", 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): C= hange 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/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-typ= o > >> 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 ? > > 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 % is not vali= d", token); > >>>> return false; > >>>> } > >>>> > >>>> diff --git a/gcc/testsuite/gcc.target/aarch64/pr102376.c b/gcc/tests= uite/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 '\\+'" } */ > --0000000000007d226705cff125a9 Content-Type: text/plain; charset="US-ASCII"; name="pr102376-5.txt" Content-Disposition: attachment; filename="pr102376-5.txt" Content-Transfer-Encoding: base64 Content-ID: X-Attachment-Id: f_kvklp4vb0 ZGlmZiAtLWdpdCBhL2djYy9jb25maWcvYWFyY2g2NC9hYXJjaDY0LmMgYi9nY2MvY29uZmlnL2Fh cmNoNjQvYWFyY2g2NC5jCmluZGV4IGZkOTI0OWM2MmIzLi5iNDQ5MjQxZjZiZCAxMDA2NDQKLS0t IGEvZ2NjL2NvbmZpZy9hYXJjaDY0L2FhcmNoNjQuYworKysgYi9nY2MvY29uZmlnL2FhcmNoNjQv YWFyY2g2NC5jCkBAIC0xNzU3MSw3ICsxNzU3MSw3IEBAIGFhcmNoNjRfaGFuZGxlX2F0dHJfdHVu ZSAoY29uc3QgY2hhciAqc3RyKQogICAgbW9kaWZpZWQuICAqLwogCiBzdGF0aWMgYm9vbAotYWFy Y2g2NF9oYW5kbGVfYXR0cl9pc2FfZmxhZ3MgKGNoYXIgKnN0cikKK2FhcmNoNjRfaGFuZGxlX2F0 dHJfaXNhX2ZsYWdzIChjb25zdCBjaGFyICpzdHIsIGJvb2wgZW1pdF9kaWFnbm9zdGljID0gdHJ1 ZSkKIHsKICAgZW51bSBhYXJjaDY0X3BhcnNlX29wdF9yZXN1bHQgcGFyc2VfcmVzOwogICB1aW50 NjRfdCBpc2FfZmxhZ3MgPSBhYXJjaDY0X2lzYV9mbGFnczsKQEAgLTE3NTkzLDYgKzE3NTkzLDkg QEAgYWFyY2g2NF9oYW5kbGVfYXR0cl9pc2FfZmxhZ3MgKGNoYXIgKnN0cikKICAgICAgIHJldHVy biB0cnVlOwogICAgIH0KIAorICBpZiAoIWVtaXRfZGlhZ25vc3RpYykKKyAgICByZXR1cm4gZmFs c2U7CisKICAgc3dpdGNoIChwYXJzZV9yZXMpCiAgICAgewogICAgICAgY2FzZSBBQVJDSDY0X1BB UlNFX01JU1NJTkdfQVJHOgpAQCAtMTc4NDQsNyArMTc4NDcsMTQgQEAgYWFyY2g2NF9wcm9jZXNz X3RhcmdldF9hdHRyICh0cmVlIGFyZ3MpCiAgICAgICBudW1fYXR0cnMrKzsKICAgICAgIGlmICgh YWFyY2g2NF9wcm9jZXNzX29uZV90YXJnZXRfYXR0ciAodG9rZW4pKQogCXsKLQkgIGVycm9yICgi cHJhZ21hIG9yIGF0dHJpYnV0ZSAlPHRhcmdldChcIiVzXCIpJT4gaXMgbm90IHZhbGlkIiwgdG9r ZW4pOworCSAgLyogQ2hlY2sgaWYgdG9rZW4gaXMgcG9zc2libHkgYW4gYXJjaCBleHRlbnNpb24g d2l0aG91dAorCSAgICAgbGVhZGluZyAnKycuICAqLworCSAgYXV0byB3aXRoX3BsdXMgPSBzdGQ6 OnN0cmluZyAoIisiKSArIHRva2VuOworCSAgaWYgKGFhcmNoNjRfaGFuZGxlX2F0dHJfaXNhX2Zs YWdzICh3aXRoX3BsdXMuY19zdHIgKCksIGZhbHNlKSkKKwkgICAgZXJyb3IgKCJhcmNoIGV4dGVu c2lvbiAlPCVzJT4gc2hvdWxkIGJlIHByZWZpeGVkIGJ5ICU8KyU+IiwKKwkJICAgdG9rZW4pOwor CSAgZWxzZQorCSAgICBlcnJvciAoInByYWdtYSBvciBhdHRyaWJ1dGUgJTx0YXJnZXQoXCIlc1wi KSU+IGlzIG5vdCB2YWxpZCIsIHRva2VuKTsKIAkgIHJldHVybiBmYWxzZTsKIAl9CiAKZGlmZiAt LWdpdCBhL2djYy90ZXN0c3VpdGUvZ2NjLnRhcmdldC9hYXJjaDY0L3ByMTAyMzc2LmMgYi9nY2Mv dGVzdHN1aXRlL2djYy50YXJnZXQvYWFyY2g2NC9wcjEwMjM3Ni5jCm5ldyBmaWxlIG1vZGUgMTAw NjQ0CmluZGV4IDAwMDAwMDAwMDAwLi5mYzgzMGFkNDc0MgotLS0gL2Rldi9udWxsCisrKyBiL2dj Yy90ZXN0c3VpdGUvZ2NjLnRhcmdldC9hYXJjaDY0L3ByMTAyMzc2LmMKQEAgLTAsMCArMSwzIEBA CisvKiB7IGRnLWRvIGNvbXBpbGUgfSAqLworCit2b2lkIGNhbGN1bGF0ZSh2b2lkKSBfX2F0dHJp YnV0ZV9fICgoX190YXJnZXRfXyAoInN2ZSIpKSk7IC8qIHsgZGctZXJyb3IgImFyY2ggZXh0ZW5z aW9uICdzdmUnIHNob3VsZCBiZSBwcmVmaXhlZCBieSAnXFwrJyIgfSAqLwo= --0000000000007d226705cff125a9--