On Fri, 13 Nov 2020, Joel Hutton wrote: > Tests are still running, but I believe I've addressed all the comments. > > > 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. > > Done. > > > 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. > > Fixed. > > > > gcc/ChangeLog: > > > > > > 2020-11-12 Joel Hutton <joel.hut...@arm.com> > > > > > > * expr.c (expand_expr_real_2): add widen_add,widen_subtract cases > > > > Not that I personally care about this stuff (would love to see changelogs > > go away :-)) but some nits: > > > > Each description is supposed to start with a capital letter and end with > > a full stop (even if it's not a complete sentence). Same for the rest > > Fixed. > > > > * optabs-tree.c (optab_for_tree_code): optabs for widening > > > adds,subtracts > > > > The line limit for changelogs is 80 characters. The entry should say > > what changed, so ?Handle ?? or ?Add case for ?? or something. > > Fixed. > > > > * tree-vect-patterns.c (vect_recog_widen_add_pattern): New recog > > > ptatern > > > > typo: pattern > > Fixed. > > > > Add widening add, subtract patterns to tree-vect-patterns. > > > Add aarch64 tests for patterns. > > > > > > fix sad > > > > Would be good to expand on this for the final commit message. > > 'fix sad' was accidentally included when I squashed two commits. I've made > all the commit messages more descriptive. > > > > + > > > + case VEC_WIDEN_SUB_HI_EXPR: > > > + return (TYPE_UNSIGNED (type) > > > + ? vec_widen_usubl_hi_optab : vec_widen_ssubl_hi_optab); > > > + > > > + > > > > Nits: excess blank line at the end and excess space before the ?:?s. > > Fixed. > > > > +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") > > > > Looks like the current code groups signed stuff together and > > unsigned stuff together, so would be good to follow that. > > Fixed. > > > Same comments as the previous patch about having a "+nosve" pragma > > and about the scan-assembler-times lines. Same for the sub test. > > Fixed. > > > 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. > > Fixed. Signed and unsigned variants because they correspond to signed and > unsigned instructions, (uaddl/uaddl2, saddl/saddl2). > > > The new functions should have comments before them. Can probably > > just use the vect_recog_widen_mult_pattern comment as a template. > > Fixed. > > > > + 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. > > Fixed. > > > > --- 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_EXPR, > > > + &plus_oprnd0, &plus_oprnd1) > > > + || vect_reassociating_reduction_p (vinfo, stmt_vinfo, > > > WIDEN_ADD_EXPR, > > > + &plus_oprnd0, &plus_oprnd1))) > > > return NULL; > > > > > > tree sum_type = 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 the > > negative of the minimum signed value due to the range of the widening > > MINUS_EXPR.) */ > > vect_unpromoted_value unprom_abs; > > plus_oprnd0 = vect_look_through_possible_promotion (vinfo, plus_oprnd0, > > &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. > > Fixed.
LGTM. Thanks, Richard. > > gcc/ChangeLog: > > 2020-11-13 Joel Hutton <joel.hut...@arm.com> > > * 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. > * optabs.def (OPTAB_D): Define vectorized widen add, subtracts. > * 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. > (vect_recog_widen_sub_pattern): New recog pattern. > (vect_recog_average_pattern): Update widened add code. > (vect_recog_average_pattern): Update widened add code. > * tree-vect-stmts.c (vectorizable_conversion): Add case for widened add, > subtract. > (supportable_widening_operation): Add case for widened add, subtract. > * tree.def > (WIDEN_PLUS_EXPR): New tree code. > (WIDEN_MINUS_EXPR): New tree code. > (VEC_WIDEN_ADD_HI_EXPR): New tree code. > (VEC_WIDEN_PLUS_LO_EXPR): New tree code. > (VEC_WIDEN_MINUS_HI_EXPR): New tree code. > (VEC_WIDEN_MINUS_LO_EXPR): New tree code. > > gcc/testsuite/ChangeLog: > > 2020-11-13 Joel Hutton <joel.hut...@arm.com> > > * gcc.target/aarch64/vect-widen-add.c: New test. > * gcc.target/aarch64/vect-widen-sub.c: New test. > -- Richard Biener <rguent...@suse.de> SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany; GF: Felix Imend
