Richard Biener <[email protected]> writes:
> 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 <[email protected]>
>> > >
>> > > * 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.
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 “correct” 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.
…
No need for another review around over that though, just go ahead and
apply the patch with those changes.
Thanks,
Richard
