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 9AAC53858D1E for ; Mon, 24 Apr 2023 13:01:21 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 9AAC53858D1E Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=arm.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=arm.com 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 EA78BD75; Mon, 24 Apr 2023 06:02:04 -0700 (PDT) Received: from localhost (e121540-lin.manchester.arm.com [10.32.110.72]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 5526A3F64C; Mon, 24 Apr 2023 06:01:20 -0700 (PDT) From: Richard Sandiford To: Richard Biener Mail-Followup-To: Richard Biener ,"Andre Vieira \(lists\)" , Richard Biener , "gcc-patches\@gcc.gnu.org" , richard.sandiford@arm.com Cc: "Andre Vieira \(lists\)" , Richard Biener , "gcc-patches\@gcc.gnu.org" Subject: Re: [ping][vect-patterns] Refactor widen_plus/widen_minus as internal_fns References: <51ce8969-3130-452e-092e-f9d91eff2dad@arm.com> Date: Mon, 24 Apr 2023 14:01:19 +0100 In-Reply-To: (Richard Biener's message of "Mon, 24 Apr 2023 13:57:33 +0200") 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=-24.5 required=5.0 tests=BAYES_00,KAM_DMARC_NONE,KAM_DMARC_STATUS,KAM_LAZY_DOMAIN_SECURITY,SPF_HELO_NONE,SPF_NONE,TXREP,T_SCC_BODY_TEXT_LINE autolearn=no autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: Richard Biener writes: > On Thu, Apr 20, 2023 at 3:24=E2=80=AFPM Andre Vieira (lists) via Gcc-patc= hes > wrote: >> >> Rebased all three patches and made some small changes to the second one: >> - removed sub and abd optabs from commutative_optab_p, I suspect this >> was a copy paste mistake, >> - removed what I believe to be a superfluous switch case in vectorizable >> conversion, the one that was here: >> + if (code.is_fn_code ()) >> + { >> + internal_fn ifn =3D as_internal_fn (code.as_fn_code ()); >> + int ecf_flags =3D internal_fn_flags (ifn); >> + gcc_assert (ecf_flags & ECF_MULTI); >> + >> + switch (code.as_fn_code ()) >> + { >> + case CFN_VEC_WIDEN_PLUS: >> + break; >> + case CFN_VEC_WIDEN_MINUS: >> + break; >> + case CFN_LAST: >> + default: >> + return false; >> + } >> + >> + internal_fn lo, hi; >> + lookup_multi_internal_fn (ifn, &lo, &hi); >> + *code1 =3D as_combined_fn (lo); >> + *code2 =3D as_combined_fn (hi); >> + optab1 =3D lookup_multi_ifn_optab (lo, !TYPE_UNSIGNED (vectype)); >> + optab2 =3D lookup_multi_ifn_optab (hi, !TYPE_UNSIGNED (vectype)); >> } >> >> I don't think we need to check they are a specfic fn code, as we look-up >> optabs and if they succeed then surely we can vectorize? >> >> OK for trunk? > > In the first patch I see some uses of safe_as_tree_code like > > + if (ch.is_tree_code ()) > + return op1 =3D=3D NULL_TREE ? gimple_build_assign (lhs, > ch.safe_as_tree_code (), > + op0) : > + gimple_build_assign (lhs, ch.safe_as_tree_c= ode (), > + op0, op1); > + else > + { > + internal_fn fn =3D as_internal_fn (ch.safe_as_fn_code ()); > + gimple* stmt; > > where the context actually requires a valid tree code. Please change tho= se > to force to tree code / ifn code. Just use explicit casts here and the o= ther > places that are similar. Before the as_internal_fn just put a > gcc_assert (ch.is_internal_fn ()). Also, doesn't the above ?: simplify to the "else" arm? Null trailing arguments would be ignored for unary operators. I wasn't sure what to make of the op0 handling: > +/* Build a GIMPLE_ASSIGN or GIMPLE_CALL with the tree_code, > + or internal_fn contained in ch, respectively. */ > +gimple * > +vect_gimple_build (tree lhs, code_helper ch, tree op0, tree op1) > +{ > + if (op0 =3D=3D NULL_TREE) > + return NULL; Can that happen, and if so, does returning null make sense? Maybe an assert would be safer. Thanks, Richard