Jakub Jelinek via Gcc-patches <gcc-patches@gcc.gnu.org> writes:
> Hi!
>
> The following testcase ICEs, because after the veclower pass which is the
> last point which lowers unsupported vector operations to supported ones
> (or scalars) match.pd simplifies a supported vector operation into
> unsupported one (vec << 1 >> 1 into vec & { ~1, ... }).
> Guarding each match.pd pattern that could do it manually would be IMHO a
> nightmare and it could affect hundreds of them, so this patch instead
> performs the verification and punting in the infrastructure (of course only
> in PROP_gimple_lvec state which is set after the vector lowering).
> The function attempts to match expand_vector_operations_1 (except I haven't
> added VEC_PERM_EXPR, VEC_COND_EXPR and COND_EXPR cases there, so those
> aren't checked and can be added later if we find it a problem).
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
>
> 2021-02-02 Jakub Jelinek <ja...@redhat.com>
>
> PR tree-optimization/98287
> * gimple-match.h (check_gimple_lvec): New declaration.
> * gimple-match-head.c (check_gimple_lvec): New function.
> (maybe_push_res_to_seq): Use it.
> * gimple-fold.c: Include tree-pass.h.
> (replace_stmt_with_simplification): Use it.
>
> * gcc.dg/pr98287.c: New test.
>
> --- gcc/gimple-match.h.jj 2021-01-04 10:25:38.456238093 +0100
> +++ gcc/gimple-match.h 2021-02-01 13:19:30.393860766 +0100
> @@ -335,6 +335,7 @@ extern tree (*mprts_hook) (gimple_match_
>
> bool gimple_simplify (gimple *, gimple_match_op *, gimple_seq *,
> tree (*)(tree), tree (*)(tree));
> +bool check_gimple_lvec (gimple_match_op *);
> tree maybe_push_res_to_seq (gimple_match_op *, gimple_seq *,
> tree res = NULL_TREE);
> void maybe_build_generic_op (gimple_match_op *);
> --- gcc/gimple-match-head.c.jj 2021-01-16 19:46:53.511672837 +0100
> +++ gcc/gimple-match-head.c 2021-02-01 13:18:58.676225427 +0100
> @@ -549,6 +549,109 @@ build_call_internal (internal_fn fn, gim
> res_op->op_or_null (4));
> }
>
> +/* In PROP_gimple_lvec mode, perform extra checking if RES_OP and return
> + true if RES_OP is not appropriate - would require vector operations that
> + would need to be lowered before expansion. */
> +
> +bool
> +check_gimple_lvec (gimple_match_op *res_op)
Guess this is personal preference, but the return value seems inverted.
I'd have expected true if something is OK and false if something isn't.
> +{
> + enum tree_code code = res_op->code;
> + enum gimple_rhs_class rhs_class = get_gimple_rhs_class (code);
> + if (rhs_class != GIMPLE_UNARY_RHS && rhs_class != GIMPLE_BINARY_RHS)
> + return false;
> +
> + tree rhs1 = res_op->op_or_null (0);
> + tree rhs2 = res_op->op_or_null (1);
> + tree type = res_op->type;
> +
> + if (!VECTOR_TYPE_P (type) || !VECTOR_TYPE_P (TREE_TYPE (rhs1)))
> + return false;
> +
> + /* A scalar operation pretending to be a vector one. */
> + if (VECTOR_BOOLEAN_TYPE_P (type)
> + && !VECTOR_MODE_P (TYPE_MODE (type))
> + && TYPE_MODE (type) != BLKmode
> + && (TREE_CODE_CLASS (code) != tcc_comparison
> + || (VECTOR_BOOLEAN_TYPE_P (TREE_TYPE (rhs1))
> + && !VECTOR_MODE_P (TYPE_MODE (TREE_TYPE (rhs1)))
> + && TYPE_MODE (TREE_TYPE (rhs1)) != BLKmode)))
> + return false;
> +
> + machine_mode mode = TYPE_MODE (type);
> + optab op;
> + switch (code)
> + {
> + CASE_CONVERT:
> + case FLOAT_EXPR:
> + case FIX_TRUNC_EXPR:
> + case VIEW_CONVERT_EXPR:
> + return false;
> +
> + case VEC_UNPACK_FLOAT_HI_EXPR:
> + case VEC_UNPACK_FLOAT_LO_EXPR:
> + case VEC_PACK_FLOAT_EXPR:
> + return false;
> +
> + case WIDEN_SUM_EXPR:
> + case VEC_WIDEN_PLUS_HI_EXPR:
> + case VEC_WIDEN_PLUS_LO_EXPR:
> + case VEC_WIDEN_MINUS_HI_EXPR:
> + case VEC_WIDEN_MINUS_LO_EXPR:
> + case VEC_WIDEN_MULT_HI_EXPR:
> + case VEC_WIDEN_MULT_LO_EXPR:
> + case VEC_WIDEN_MULT_EVEN_EXPR:
> + case VEC_WIDEN_MULT_ODD_EXPR:
> + case VEC_UNPACK_HI_EXPR:
> + case VEC_UNPACK_LO_EXPR:
> + case VEC_UNPACK_FIX_TRUNC_HI_EXPR:
> + case VEC_UNPACK_FIX_TRUNC_LO_EXPR:
> + case VEC_PACK_TRUNC_EXPR:
> + case VEC_PACK_SAT_EXPR:
> + case VEC_PACK_FIX_TRUNC_EXPR:
> + case VEC_WIDEN_LSHIFT_HI_EXPR:
> + case VEC_WIDEN_LSHIFT_LO_EXPR:
> + return false;
> +
> + case LSHIFT_EXPR:
> + case RSHIFT_EXPR:
> + case LROTATE_EXPR:
> + case RROTATE_EXPR:
> + if (!VECTOR_MODE_P (mode))
> + return true;
> + if (!VECTOR_INTEGER_TYPE_P (TREE_TYPE (rhs2)))
> + {
> + op = optab_for_tree_code (code, type, optab_scalar);
> + if (op && optab_handler (op, mode) != CODE_FOR_nothing)
> + return false;
> + }
> + op = optab_for_tree_code (code, type, optab_vector);
> + if (op && optab_handler (op, mode) != CODE_FOR_nothing)
> + return false;
> + return true;
> +
> + case MULT_HIGHPART_EXPR:
> + if (!VECTOR_MODE_P (mode)
> + || !can_mult_highpart_p (mode, TYPE_UNSIGNED (type)))
> + return true;
> + return false;
> +
> + default:
> + if (!VECTOR_MODE_P (mode))
> + return true;
> + op = optab_for_tree_code (code, type, optab_default);
> + if (op == unknown_optab
> + && code == NEGATE_EXPR
> + && INTEGRAL_TYPE_P (TREE_TYPE (type)))
> + op = optab_for_tree_code (MINUS_EXPR, type, optab_default);
> + if (op && optab_handler (op, mode) != CODE_FOR_nothing)
> + return false;
> + return true;
This doesn't look right for things like SVE comparisons, where the
result is a VECTOR_BOOLEAN_TYPE_P with MODE_VECTOR_BOOL and where
the inputs are something else. I guess it might also affect FP
comparisons on most targets.
So I think it would be worth separating out comparisons, even if we
just accept them unconditionally to start with.
Thanks,
Richard
> + }
> +
> + return false;
> +}
> +
> /* Push the exploded expression described by RES_OP as a statement to
> SEQ if necessary and return a gimple value denoting the value of the
> expression. If RES is not NULL then the result will be always RES
> @@ -597,6 +700,12 @@ maybe_push_res_to_seq (gimple_match_op *
>
> if (res_op->code.is_tree_code ())
> {
> + if (VECTOR_TYPE_P (res_op->type)
> + && cfun
> + && (cfun->curr_properties & PROP_gimple_lvec) != 0
> + && check_gimple_lvec (res_op))
> + return NULL_TREE;
> +
> if (!res)
> {
> if (gimple_in_ssa_p (cfun))
> --- gcc/gimple-fold.c.jj 2021-01-22 11:41:56.510499898 +0100
> +++ gcc/gimple-fold.c 2021-02-01 13:22:11.992002840 +0100
> @@ -66,6 +66,7 @@ along with GCC; see the file COPYING3.
> #include "tree-vector-builder.h"
> #include "tree-ssa-strlen.h"
> #include "varasm.h"
> +#include "tree-pass.h"
>
> enum strlen_range_kind {
> /* Compute the exact constant string length. */
> @@ -5678,6 +5679,11 @@ replace_stmt_with_simplification (gimple
> if (!inplace
> || gimple_num_ops (stmt) > get_gimple_rhs_num_ops (res_op->code))
> {
> + if (VECTOR_TYPE_P (res_op->type)
> + && cfun
> + && (cfun->curr_properties & PROP_gimple_lvec) != 0
> + && check_gimple_lvec (res_op))
> + return false;
> maybe_build_generic_op (res_op);
> gimple_assign_set_rhs_with_ops (gsi, res_op->code,
> res_op->op_or_null (0),
> --- gcc/testsuite/gcc.dg/pr98287.c.jj 2021-02-01 13:32:22.735981006 +0100
> +++ gcc/testsuite/gcc.dg/pr98287.c 2021-02-01 13:32:01.155229122 +0100
> @@ -0,0 +1,19 @@
> +/* PR tree-optimization/98287 */
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -fno-tree-ccp -fno-tree-forwprop -Wno-psabi -w" } */
> +
> +typedef unsigned long __attribute__((__vector_size__ (8))) V;
> +V v;
> +
> +static __attribute__((noinline, noclone)) V
> +bar (unsigned short s)
> +{
> + return v >> s << s | v >> s >> 63;
> +}
> +
> +unsigned long
> +foo (void)
> +{
> + V x = bar (1);
> + return x[0];
> +}
>
> Jakub
