On Mon, Apr 27, 2020 at 6:54 PM Richard Sandiford
<richard.sandif...@arm.com> wrote:
>
> Christophe Lyon <christophe.l...@linaro.org> writes:
> >> This PR was caused by mismatched expectations between
> >> vectorizable_comparison and SLP. We had a "<" comparison
> >> between two booleans that were leaves of the SLP tree, so
> >> vectorizable_comparison fell back on:
> >>
> >> /* Invariant comparison. */
> >> if (!vectype)
> >> {
> >> vectype = get_vectype_for_scalar_type (vinfo, TREE_TYPE (rhs1),
> >> slp_node);
> >> if (maybe_ne (TYPE_VECTOR_SUBPARTS (vectype), nunits))
> >> return false;
> >> }
> >>
> >> rhs1 and rhs2 were *unsigned* boolean types, so we got back a vector
> >> of unsigned integers. This in itself was OK, and meant that "<"
> >> worked as expected without the need for the boolean fix-ups:
> >>
> >> /* Boolean values may have another representation in vectors
> >> and therefore we prefer bit operations over comparison for
> >> them (which also works for scalar masks). We store opcodes
> >> to use in bitop1 and bitop2. Statement is vectorized as
> >> BITOP2 (rhs1 BITOP1 rhs2) or
> >> rhs1 BITOP2 (BITOP1 rhs2)
> >> depending on bitop1 and bitop2 arity. */
> >> bool swap_p = false;
> >> if (VECTOR_BOOLEAN_TYPE_P (vectype))
> >> {
> >>
> >> However, vectorizable_comparison then used vect_get_slp_defs to get
> >> the actual operands. The request went to vect_get_constant_vectors,
> >> which also has logic to calculate the vector type. The problem was
> >> that this type was different from the one chosen above:
> >>
> >> if (VECT_SCALAR_BOOLEAN_TYPE_P (TREE_TYPE (op))
> >> && vect_mask_constant_operand_p (stmt_vinfo))
> >> vector_type = truth_type_for (stmt_vectype);
> >> else
> >> vector_type = get_vectype_for_scalar_type (vinfo, TREE_TYPE (op),
> >> op_node);
> >>
> >> So the function gave back a vector of mask types, which here are vectors
> >> of *signed* booleans. This meant that "<" gave:
> >>
> >> true (-1) < false (0)
> >>
> >> and so the boolean fixup above was needed after all.
> >>
> >> Fixed by making vectorizable_comparison also pick a mask type in this case.
> >>
> >> Tested on aarch64-linux-gnu (with and without SVE) and x86_64-linux-gnu.
> >> Approved by Richard in the PR.
> >>
> >> Richard
> >>
> >>
> >> 2020-04-23 Richard Sandiford <richard.sandif...@arm.com>
> >>
> >> gcc/
> >> PR tree-optimization/94727
> >> * tree-vect-stmts.c (vectorizable_comparison): Use mask_type when
> >> comparing invariant scalar booleans.
> >>
> >> gcc/testsuite/
> >> PR tree-optimization/94727
> >> * gcc.dg/vect/pr94727.c: New test.
> >
> > Hi Richard,
> >
> > The new testcase fails at execution on arm (and s390 according to
> > gcc-testresults).
> >
> > Can you check?
>
> Thanks for the heads-up. Looks like we need the same fix when
> handling comparisons embedded in a VEC_COND_EXPR.
>
> Here too, the problem is that vect_get_constant_vectors will
> calculate its own vector type, using truth_type_for on the
> STMT_VINFO_VECTYPE, and the vectoriable_* routines need to be
> consistent with that.
>
> Tested on arm-linux-gnueabihf and x86_64-linux-gnu. OK to install?
OK.
> Richard
>
>
> 2020-04-27 Richard Sandiford <richard.sandif...@arm.com>
>
> gcc/
> PR tree-optimization/94727
> * tree-vect-stmts.c (vect_is_simple_cond): If both comparison
> operands are invariant booleans, use the mask type associated with the
> STMT_VINFO_VECTYPE. Use !slp_node instead of !vectype to exclude SLP.
> (vectorizable_condition): Pass vectype unconditionally to
> vect_is_simple_cond.
> ---
> gcc/tree-vect-stmts.c | 27 ++++++++++++++++-----------
> 1 file changed, 16 insertions(+), 11 deletions(-)
>
> diff --git a/gcc/tree-vect-stmts.c b/gcc/tree-vect-stmts.c
> index 88a1e2c51d2..1984787bac4 100644
> --- a/gcc/tree-vect-stmts.c
> +++ b/gcc/tree-vect-stmts.c
> @@ -9942,16 +9942,21 @@ vect_is_simple_cond (tree cond, vec_info *vinfo,
> slp_tree slp_node,
> if (! *comp_vectype)
> {
> tree scalar_type = TREE_TYPE (lhs);
> - /* If we can widen the comparison to match vectype do so. */
> - if (INTEGRAL_TYPE_P (scalar_type)
> - && vectype
> - && tree_int_cst_lt (TYPE_SIZE (scalar_type),
> - TYPE_SIZE (TREE_TYPE (vectype))))
> - scalar_type = build_nonstandard_integer_type
> - (tree_to_uhwi (TYPE_SIZE (TREE_TYPE (vectype))),
> - TYPE_UNSIGNED (scalar_type));
> - *comp_vectype = get_vectype_for_scalar_type (vinfo, scalar_type,
> - slp_node);
> + if (VECT_SCALAR_BOOLEAN_TYPE_P (scalar_type))
> + *comp_vectype = truth_type_for (vectype);
> + else
> + {
> + /* If we can widen the comparison to match vectype do so. */
> + if (INTEGRAL_TYPE_P (scalar_type)
> + && !slp_node
> + && tree_int_cst_lt (TYPE_SIZE (scalar_type),
> + TYPE_SIZE (TREE_TYPE (vectype))))
> + scalar_type = build_nonstandard_integer_type
> + (tree_to_uhwi (TYPE_SIZE (TREE_TYPE (vectype))),
> + TYPE_UNSIGNED (scalar_type));
> + *comp_vectype = get_vectype_for_scalar_type (vinfo, scalar_type,
> + slp_node);
> + }
> }
>
> return true;
> @@ -10066,7 +10071,7 @@ vectorizable_condition (stmt_vec_info stmt_info,
> gimple_stmt_iterator *gsi,
> else_clause = gimple_assign_rhs3 (stmt);
>
> if (!vect_is_simple_cond (cond_expr, stmt_info->vinfo, slp_node,
> - &comp_vectype, &dts[0], slp_node ? NULL : vectype)
> + &comp_vectype, &dts[0], vectype)
> || !comp_vectype)
> return false;
>
> --
> 2.17.1
>
