diff mbox series

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

Message ID ZBwOTvM4REORW9kQ@tucnak
State Unresolved
Headers
Series tree-vect-generic: Fix up expand_vector_condition [PR109176] |

Checks

Context Check Description
snail/gcc-patch-check warning Git am fail log

Commit Message

Jakub Jelinek March 23, 2023, 8:31 a.m. UTC 
  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.

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?

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.


	Jakub

Comments

Richard Biener March 23, 2023, 8:48 a.m. UTC | #1 
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
> 
>
diff mbox series

Patch

--- 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);
+}