Re: [PATCH] tree-vect-generic: Fix up expand_vector_condition [PR109176]

[Richard Biener <rguenther@suse.de>]

On Thu, 23 Mar 2023, Jakub Jelinek wrote:

> Hi!
> 
> The following testcase ICEs on aarch64-linux, because
> expand_vector_condition attempts to piecewise lower SVE
>   d_3 = a_1(D) < b_2(D);
>   _5 = VEC_COND_EXPR <d_3, c_4(D), d_3>;
> which isn't possible - nunits_for_known_piecewise_op ICEs but
> the rest of the code assumes constant number of elements too.
> 
> expand_vector_condition attempts to find if a (rhs1) is a SSA_NAME
> for comparison and calls expand_vec_cond_expr_p (type, TREE_TYPE (a1), code)
> where a1 is one of the operands of the comparison and code is the comparison
> code.  That one indeed isn't supported here, but what aarch64 SVE supports
> are the individual statements, comparison (expand_vec_cmp_expr_p) and
> expand_vec_cond_expr_p (type, TREE_TYPE (a), SSA_NAME), the latter because
> that function starts with
>   if (VECTOR_BOOLEAN_TYPE_P (cmp_op_type)
>       && get_vcond_mask_icode (TYPE_MODE (value_type),
>                                TYPE_MODE (cmp_op_type)) != CODE_FOR_nothing)
>     return true;
> 
> In an earlier version of the patch (in the PR), we did this
>   if (VECTOR_BOOLEAN_TYPE_P (TREE_TYPE (a))
>       && expand_vec_cond_expr_p (type, TREE_TYPE (a), ERROR_MARK))
>     return true;
> before the code == SSA_NAME handling plus some further tweaks later.
> While that fixed the ICE, it broke quite a few tests on x86 and some on
> aarch64 too.  The problem is that expand_vector_comparison doesn't lower
> comparisons which aren't supported and only feed VEC_COND_EXPR first operand
> and expand_vector_condition succeeds for those, so with the above mentioned
> change we'd verify the VEC_COND_EXPR is implementable using optab alone,
> but nothing would verify the tcc_comparison which relied on
> expand_vector_condition to verify.

Ah, indeed - all a bit twisty.

> So, the following patch instead queries whether optabs can handle the
> comparison and VEC_COND_EXPR together (if a (rhs1) is a comparison;
> otherwise as before it checks only the VEC_COND_EXPR) and if that fails,
> also checks whether the two operations could be supported individually
> and only if even that fails does the piecewise lowering.
> 
> Bootstrapped/regtested on x86_64-linux, i686-linux and aarch64-linux, ok for
> trunk?

OK.

Thanks for digging into it.

Richard.

> 2023-03-23  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR tree-optimization/109176
> 	* tree-vect-generic.cc (expand_vector_condition): If a has
> 	vector boolean type and is a comparison, also check if both
> 	the comparison and VEC_COND_EXPR could be successfully expanded
> 	individually.
> 
> 	* gcc.target/aarch64/sve/pr109176.c: New test.
> 
> --- gcc/tree-vect-generic.cc.jj	2023-03-21 13:28:21.354671095 +0100
> +++ gcc/tree-vect-generic.cc	2023-03-22 12:53:27.853986127 +0100
> @@ -1063,6 +1063,15 @@ expand_vector_condition (gimple_stmt_ite
>        return true;
>      }
>  
> +  /* If a has vector boolean type and is a comparison, above
> +     expand_vec_cond_expr_p might fail, even if both the comparison and
> +     VEC_COND_EXPR could be supported individually.  See PR109176.  */
> +  if (a_is_comparison
> +      && VECTOR_BOOLEAN_TYPE_P (TREE_TYPE (a))
> +      && expand_vec_cond_expr_p (type, TREE_TYPE (a), SSA_NAME)
> +      && expand_vec_cmp_expr_p (TREE_TYPE (a1), TREE_TYPE (a), code))
> +    return true;
> +
>    /* Handle vector boolean types with bitmasks.  If there is a comparison
>       and we can expand the comparison into the vector boolean bitmask,
>       or otherwise if it is compatible with type, we can transform
> --- gcc/testsuite/gcc.target/aarch64/sve/pr109176.c.jj	2023-03-22 12:19:21.672218631 +0100
> +++ gcc/testsuite/gcc.target/aarch64/sve/pr109176.c	2023-03-22 12:19:21.672218631 +0100
> @@ -0,0 +1,12 @@
> +/* PR tree-optimization/109176 */
> +/* { dg-do compile } */
> +/* { dg-additional-options "-O2" } */
> +
> +#include <arm_sve.h>
> +
> +svbool_t
> +foo (svint8_t a, svint8_t b, svbool_t c)
> +{
> +  svbool_t d = svcmplt_s8 (svptrue_pat_b8 (SV_ALL), a, b);
> +  return svsel_b (d, c, d);
> +}
> 
> 	Jakub
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg,
Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman;
HRB 36809 (AG Nuernberg)