Re: [2/3][vect] Add widening add, subtract vect patterns

[Richard Biener <rguenther@suse.de>]

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.hutton@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.hutton@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.hutton@arm.com>
> 
>         * gcc.target/aarch64/vect-widen-add.c: New test.
>         * gcc.target/aarch64/vect-widen-sub.c: New test.
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Felix Imend