From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ed1-x532.google.com (mail-ed1-x532.google.com [IPv6:2a00:1450:4864:20::532]) by sourceware.org (Postfix) with ESMTPS id 71B14385842A for ; Thu, 11 Nov 2021 09:16:37 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 71B14385842A Received: by mail-ed1-x532.google.com with SMTP id t18so2171945edd.8 for ; Thu, 11 Nov 2021 01:16:37 -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:content-transfer-encoding; bh=ojjJAdL8h6LSBYi+fraWNhiNsgdJszEzHJzJeERlsZY=; b=TIVE3vXu36Ks0dueVe9j4gLipf87wq6DdHJGib75q+eEduhyx70BboE6Byc5DIhmU3 IdFPyZsiawy5xY/9ar1qi4SQdeuRmh9CDkVJb2tp0lIgHEvjCe2J60A1zVJYP3BV6pdv XB33236F5V0cMyb/jdhu77KHImApyZxHP0gzgI+c6iYnxVHC73alv4GjYADfvsDPMuHW wKJPHYhNsRGm9G019LP4GuMz6C7Q8S6DEilGwh9FlPXCknICU3jPNQcpI12ob2DKtP6y IFg7jD0AfeuwRDiYUJeMRAeUyrQ3adPv0Y3ROd+DMqAqgHUzPwR0t5cfj6vR+dKH7ree Pl8A== X-Gm-Message-State: AOAM530sxFos5yvfZujUO9kYOhfAcRKhq7lsyYuqs0sZphiw2zHaCldd Yjiarak6Q/FtQDl22gusDUwVG5Yj3CfxtUxaULsNaA== X-Google-Smtp-Source: ABdhPJwZqvWk3NK62/CjJ7c4JnxJ4oFrUKEf5fYluTr08lTakBhvP1bAjXLs5ujLFkThBX+ntEiowHlPP3bazOFNXXU= X-Received: by 2002:a17:906:2ada:: with SMTP id m26mr1969465eje.571.1636622195910; Thu, 11 Nov 2021 01:16:35 -0800 (PST) MIME-Version: 1.0 References: In-Reply-To: From: Prathamesh Kulkarni Date: Thu, 11 Nov 2021 14:45:59 +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: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-8.8 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, 11 Nov 2021 09:16:39 -0000 On Tue, 9 Nov 2021 at 20:27, Richard Sandiford wrote: > > Prathamesh Kulkarni writes: > > 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 = 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 =E2=80=98sve=E2=80=99 shou= ld 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= /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 arg= s) > >> >> >> > 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 withou= t > >> >> >> > + 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 wit= h %<+%>", 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 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 % is no= t valid", token); > >> >> >> > + free (str); > >> >> >> > return false; > >> >> >> > } > >> >> >> > > >> >> > > >> >> > [aarch64] PR102376 - Emit better diagnostics for arch extension i= n 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 a= n 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/aa= rch64.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 val= id", 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 emitte= d > >> > 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-t= ypo > >> > of value ("+arch=3Darmv8-a-typo") in =E2=80=98target()=E2=80=99 prag= ma 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, whic= h > >> > 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 % is not valid", t= oken); > > + /* Check if token is possibly an arch extension without > > + leading '+'. */ > > + uint64_t isa_temp =3D 0; > > + auto with_plus =3D std::string ("+") + token; > > + enum aarch64_parse_opt_result ext_res > > + =3D aarch64_parse_extension (with_plus.c_str (), &isa_temp, n= ullptr); > > + > > + if (ext_res =3D=3D AARCH64_PARSE_OK) > > + error ("arch extension %<%s%> should be prefixed by %<+%>", > > + token); > > + else > > + error ("pragma or attribute % is not valid",= token); > > return false; > > } > > > > diff --git a/gcc/testsuite/gcc.target/aarch64/pr102376.c b/gcc/testsuit= e/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-err= or "arch extension 'sve' should be prefixed by '\\+'" } */