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 0F0083858026 for ; Tue, 17 Nov 2020 13:40:09 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 0F0083858026 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 B757E101E; Tue, 17 Nov 2020 05:40:08 -0800 (PST) Received: from localhost (e121540-lin.manchester.arm.com [10.32.98.126]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 264F63F719; Tue, 17 Nov 2020 05:40:08 -0800 (PST) From: Richard Sandiford To: Richard Biener Mail-Followup-To: Richard Biener , Joel Hutton , Joel Hutton via Gcc-patches , richard.sandiford@arm.com Cc: Joel Hutton , Joel Hutton via Gcc-patches Subject: Re: [2/3][vect] Add widening add, subtract vect patterns References: Date: Tue, 17 Nov 2020 13:40:06 +0000 In-Reply-To: (Richard Biener's message of "Mon, 16 Nov 2020 15:04:06 +0100 (CET)") 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=-6.7 required=5.0 tests=BAYES_00, KAM_DMARC_STATUS, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) 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: Tue, 17 Nov 2020 13:40:10 -0000 Richard Biener writes: > On Fri, 13 Nov 2020, Joel Hutton wrote: > >> Tests are still running, but I believe I've addressed all the comments. >>=20 >> > Like Richard said, the new patterns need to be documented in md.texi >> > and the new tree codes need to be documented in generic.texi. >>=20 >> Done. >>=20 >> > While we're using tree codes, I think we need to make the naming >> > consistent with other tree codes: WIDEN_PLUS_EXPR instead of >> > WIDEN_ADD_EXPR and WIDEN_MINUS_EXPR instead of WIDEN_SUB_EXPR. >> > Same idea for the VEC_* codes. >>=20 >> Fixed. >>=20 >> > > gcc/ChangeLog: >> > > >> > > 2020-11-12 Joel Hutton >> > > >> > > * expr.c (expand_expr_real_2): add widen_add,widen_subtract = cases >> >=20 >> > Not that I personally care about this stuff (would love to see changel= ogs >> > go away :-)) but some nits: >> >=20 >> > Each description is supposed to start with a capital letter and end wi= th >> > a full stop (even if it's not a complete sentence). Same for the rest >>=20 >> Fixed. >>=20 >> > > * optabs-tree.c (optab_for_tree_code): optabs for widening a= dds,subtracts >> >=20 >> > The line limit for changelogs is 80 characters. The entry should say >> > what changed, so ?Handle ?? or ?Add case for ?? or something. >>=20 >> Fixed. >>=20 >> > > * tree-vect-patterns.c (vect_recog_widen_add_pattern): New r= ecog ptatern >> >=20 >> > typo: pattern >>=20 >> Fixed. >>=20 >> > > Add widening add, subtract patterns to tree-vect-patterns. >> > > Add aarch64 tests for patterns. >> > > >> > > fix sad >> >=20 >> > Would be good to expand on this for the final commit message. >>=20 >> 'fix sad' was accidentally included when I squashed two commits. I've ma= de all the commit messages more descriptive. >>=20 >> > > + >> > > + case VEC_WIDEN_SUB_HI_EXPR: >> > > + return (TYPE_UNSIGNED (type) >> > > + ? vec_widen_usubl_hi_optab : vec_widen_ssubl_hi_optab); >> > > + >> > > + >> >=20 >> > Nits: excess blank line at the end and excess space before the ?:?s. >>=20 >> Fixed. >>=20 >> > > +OPTAB_D (vec_widen_usubl_lo_optab, "vec_widen_usubl_lo_$a") >> > > +OPTAB_D (vec_widen_uaddl_hi_optab, "vec_widen_uaddl_hi_$a") >> > > +OPTAB_D (vec_widen_uaddl_lo_optab, "vec_widen_uaddl_lo_$a") >> > > OPTAB_D (vec_widen_sshiftl_hi_optab, "vec_widen_sshiftl_hi_$a") >> > > OPTAB_D (vec_widen_sshiftl_lo_optab, "vec_widen_sshiftl_lo_$a") >> > > OPTAB_D (vec_widen_umult_even_optab, "vec_widen_umult_even_$a") >> >=20 >> > Looks like the current code groups signed stuff together and >> > unsigned stuff together, so would be good to follow that. >>=20 >> Fixed. >>=20 >> > Same comments as the previous patch about having a "+nosve" pragma >> > and about the scan-assembler-times lines. Same for the sub test. >>=20 >> Fixed. >>=20 >> > I am missing documentation in md.texi for the new patterns. In >> > particular I wonder why you need singed and unsigned variants >> > for the add/subtract patterns. >>=20 >> Fixed. Signed and unsigned variants because they correspond to signed and >> unsigned instructions, (uaddl/uaddl2, saddl/saddl2). >>=20 >> > The new functions should have comments before them. Can probably >> > just use the vect_recog_widen_mult_pattern comment as a template. >>=20 >> Fixed. >>=20 >> > > + case VEC_WIDEN_SUB_HI_EXPR: >> > > + case VEC_WIDEN_SUB_LO_EXPR: >> > > + case VEC_WIDEN_ADD_HI_EXPR: >> > > + case VEC_WIDEN_ADD_LO_EXPR: >> > > + return false; >> > > + >> > >> > I think these should get the same validity checking as >> > VEC_WIDEN_MULT_HI_EXPR etc. >>=20 >> Fixed. >>=20 >> > > --- a/gcc/tree-vect-patterns.c >> > > +++ b/gcc/tree-vect-patterns.c >> > > @@ -1086,8 +1086,10 @@ vect_recog_sad_pattern (vec_info *vinfo, >> > > of the above pattern. */ >> > > >> > > tree plus_oprnd0, plus_oprnd1; >> > > - if (!vect_reassociating_reduction_p (vinfo, stmt_vinfo, PLUS_EXPR, >> > > - &plus_oprnd0, &plus_oprnd1)) >> > > + if (!(vect_reassociating_reduction_p (vinfo, stmt_vinfo, PLUS_EXP= R, >> > > + &plus_oprnd0, &plus_oprnd1) >> > > + || vect_reassociating_reduction_p (vinfo, stmt_vinfo, WIDEN_AD= D_EXPR, >> > > + &plus_oprnd0, &plus_oprnd1))) >> > > return NULL; >> > > >> > > tree sum_type =3D gimple_expr_type (last_stmt); >> > >> > I think we should make: >> > >> > /* Any non-truncating sequence of conversions is OK here, since >> > with a successful match, the result of the ABS(U) is known to fit >> > within the nonnegative range of the result type. (It cannot be t= he >> > negative of the minimum signed value due to the range of the wide= ning >> > MINUS_EXPR.) */ >> > vect_unpromoted_value unprom_abs; >> > plus_oprnd0 =3D vect_look_through_possible_promotion (vinfo, plus_op= rnd0, >> > &unprom_abs); >> > >> > specific to the PLUS_EXPR case. If we look through promotions on >> > the operands of a WIDEN_ADD_EXPR, we could potentially have a mixture >> > of signednesses involved, one on the operands of the WIDEN_ADD_EXPR >> > and one on its inputs. >>=20 >> Fixed. > > LGTM. Same here FWIW, although: > +/* Try to detect addition on widened inputs, converting SUB_EXPR > + to WIDEN_MINUS_EXPR. See vect_recog_widen_op_pattern for details. */ > +static gimple * > +vect_recog_widen_minus_pattern (vec_info *vinfo, stmt_vec_info last_stmt= _info, > + tree *type_out) > +{ > + return vect_recog_widen_op_pattern (vinfo, last_stmt_info, type_out, > + MINUS_EXPR, WIDEN_MINUS_EXPR, false, > + "vect_recog_widen_minus_pattern"); > +} s/addition/subtraction/ and s/SUB_EXPR/MINUS_EXPR/ in the comment. Just to be sure, on the changelog: >> * expr.c (expand_expr_real_2): Add widen_add,widen_subtract cases. >> * optabs-tree.c (optab_for_tree_code): Add case for widening optabs. >> adds, subtracts. Continuation lines should be indented by a tab only, not a tab and two spaces. (Although I agree the above looks nicer than the =E2=80=9Ccorrect= =E2=80=9D way.) >> * optabs.def (OPTAB_D): Define vectorized widen add, subtracts. Should be indented by a tab rather than 8 spaces. >> * tree-cfg.c (verify_gimple_assign_binary): Add case for widening adds, >> subtracts. >> * tree-inline.c (estimate_operator_cost): Add case for widening adds, >> subtracts. >> * tree-vect-generic.c (expand_vector_operations_1): Add case for >> widening adds, subtracts tree-vect-patterns.c >> * (vect_recog_widen_add_pattern): New recog pattern. Mis-positioned tree-vect-patterns.c, should be * tree-vect-generic.c (expand_vector_operations_1): Add case for widening adds, subtracts. * tree-vect-patterns.c (vect_recog_widen_add_pattern): New recog pattern. =E2=80=A6 No need for another review around over that though, just go ahead and apply the patch with those changes. Thanks, Richard