On Thu, 23 Apr 2020 at 16:48, Richard Sandiford
<[email protected]> wrote:
>
> 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 <[email protected]>
>
> 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
Christophe
> ---
> gcc/testsuite/gcc.dg/vect/pr94727.c | 24 ++++++++++++++++++++++++
> gcc/tree-vect-stmts.c | 7 +++++--
> 2 files changed, 29 insertions(+), 2 deletions(-)
> create mode 100644 gcc/testsuite/gcc.dg/vect/pr94727.c
>
> diff --git a/gcc/tree-vect-stmts.c b/gcc/tree-vect-stmts.c
> index 7f3a9fb5fb3..88a1e2c51d2 100644
> --- a/gcc/tree-vect-stmts.c
> +++ b/gcc/tree-vect-stmts.c
> @@ -10566,8 +10566,11 @@ vectorizable_comparison (stmt_vec_info stmt_info,
> gimple_stmt_iterator *gsi,
> /* Invariant comparison. */
> if (!vectype)
> {
> - vectype = get_vectype_for_scalar_type (vinfo, TREE_TYPE (rhs1),
> - slp_node);
> + if (VECT_SCALAR_BOOLEAN_TYPE_P (TREE_TYPE (rhs1)))
> + vectype = mask_type;
> + else
> + vectype = get_vectype_for_scalar_type (vinfo, TREE_TYPE (rhs1),
> + slp_node);
> if (!vectype || maybe_ne (TYPE_VECTOR_SUBPARTS (vectype), nunits))
> return false;
> }
> diff --git a/gcc/testsuite/gcc.dg/vect/pr94727.c
> b/gcc/testsuite/gcc.dg/vect/pr94727.c
> new file mode 100644
> index 00000000000..38408711345
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/vect/pr94727.c
> @@ -0,0 +1,24 @@
> +/* { dg-additional-options "-O3" } */
> +
> +unsigned char a[16][32];
> +long b[16][32];
> +unsigned long c;
> +_Bool d;
> +
> +void __attribute__((noipa))
> +foo (void)
> +{
> + for (int j = 0; j < 8; j++)
> + for (int i = 0; i < 17; ++i)
> + b[j][i] = (a[j][i] < c) > d;
> +}
> +
> +int
> +main (void)
> +{
> + c = 1;
> + foo ();
> + if (!b[0][0])
> + __builtin_abort ();
> + return 0;
> +}
