[PATCH] Fix ICE for boolean comparison

[PATCH] Fix ICE for boolean comparison

[Ilya Enkovich]

Hi,

Currently compiler may ICE when loaded boolean is compared with vector invariant or another boolean value.  This is because we don't detect mix of bool and non-bool vectypes and incorrectly determine vectype for boolean loop invariant for comparison.  This was fixed for COND_EXP before but also needs to be fixed for comparison.  This patch was bootstrapped and tested on x86_64-unknown-linux-gnu.  OK for trunk?

Thanks,
Ilya
--
gcc/

2015-11-12  Ilya Enkovich  <enkovich.gnu@gmail.com>

	* tree-vect-loop.c (vect_determine_vectorization_factor): Check
	mix of boolean and integer vectors in a single statement.
	* tree-vect-slp.c (vect_mask_constant_operand_p): New.
	(vect_get_constant_vectors): Use vect_mask_constant_operand_p to
	determine constant type.
	* tree-vect-stmts.c (vectorizable_comparison): Provide vectype
	for loop invariants.

gcc/testsuite/

2015-11-12  Ilya Enkovich  <enkovich.gnu@gmail.com>

	* g++.dg/vect/simd-bool-comparison-1.cc: New test.
	* g++.dg/vect/simd-bool-comparison-2.cc: New test.


diff --git a/gcc/testsuite/g++.dg/vect/simd-bool-comparison-1.cc b/gcc/testsuite/g++.dg/vect/simd-bool-comparison-1.cc
new file mode 100644
index 0000000..a08362f
--- /dev/null
+++ b/gcc/testsuite/g++.dg/vect/simd-bool-comparison-1.cc
@@ -0,0 +1,21 @@
+// { dg-do compile }
+// { dg-additional-options "-mavx512bw -mavx512dq" { target { i?86-*-* x86_64-*-* } } }
+
+#define N 1024
+
+double a[N];
+bool b[N];
+bool c;
+
+void test ()
+{
+  int i;
+
+  for (i = 0; i < N; i++)
+    if (b[i] != c)
+      a[i] = 0.0;
+    else
+      a[i] = 1.0;
+}
+
+// { dg-final { scan-tree-dump-times "vectorized 1 loops" 1 "vect" { target { i?86-*-* x86_64-*-* } } } }
diff --git a/gcc/testsuite/g++.dg/vect/simd-bool-comparison-2.cc b/gcc/testsuite/g++.dg/vect/simd-bool-comparison-2.cc
new file mode 100644
index 0000000..4accf56
--- /dev/null
+++ b/gcc/testsuite/g++.dg/vect/simd-bool-comparison-2.cc
@@ -0,0 +1,20 @@
+// { dg-do compile }
+// { dg-additional-options "-mavx512bw -mavx512dq" { target { i?86-*-* x86_64-*-* } } }
+
+#define N 1024
+
+double a[N];
+bool b[N];
+char c[N];
+
+void test ()
+{
+  int i;
+
+  #pragma omp simd
+  for (i = 0; i < N; i++)
+    if ((c[i] > 0) && b[i])
+      a[i] = 0.0;
+    else
+      a[i] = 1.0;
+}
diff --git a/gcc/tree-vect-loop.c b/gcc/tree-vect-loop.c
index 55e5309..6b78b55 100644
--- a/gcc/tree-vect-loop.c
+++ b/gcc/tree-vect-loop.c
@@ -649,7 +649,32 @@ vect_determine_vectorization_factor (loop_vec_info loop_vinfo)
 		    }
 		  return false;
 		}
+	      else if (VECTOR_BOOLEAN_TYPE_P (mask_type)
+		       != VECTOR_BOOLEAN_TYPE_P (vectype))
+		{
+		  if (dump_enabled_p ())
+		    {
+		      dump_printf_loc (MSG_MISSED_OPTIMIZATION, vect_location,
+				       "not vectorized: mixed mask and "
+				       "nonmask vector types in statement, ");
+		      dump_generic_expr (MSG_MISSED_OPTIMIZATION, TDF_SLIM,
+					 mask_type);
+		      dump_printf (MSG_MISSED_OPTIMIZATION, " and ");
+		      dump_generic_expr (MSG_MISSED_OPTIMIZATION, TDF_SLIM,
+					 vectype);
+		      dump_printf (MSG_MISSED_OPTIMIZATION, "\n");
+		    }
+		  return false;
+		}
 	    }
+
+	  /* We may compare boolean value loaded as vector of integers.
+	     Fix mask_type in such case.  */
+	  if (mask_type
+	      && !VECTOR_BOOLEAN_TYPE_P (mask_type)
+	      && gimple_code (stmt) == GIMPLE_ASSIGN
+	      && TREE_CODE_CLASS (gimple_assign_rhs_code (stmt)) == tcc_comparison)
+	    mask_type = build_same_sized_truth_vector_type (mask_type);
 	}
 
       /* No mask_type should mean loop invariant predicate.
diff --git a/gcc/tree-vect-slp.c b/gcc/tree-vect-slp.c
index 9d97140..f3acb04 100644
--- a/gcc/tree-vect-slp.c
+++ b/gcc/tree-vect-slp.c
@@ -2589,6 +2589,57 @@ vect_slp_bb (basic_block bb)
 }
 
 
+/* Return 1 if vector type of boolean constant which is OPNUM
+   operand in statement STMT is a boolean vector.  */
+
+static bool
+vect_mask_constant_operand_p (gimple *stmt, int opnum)
+{
+  stmt_vec_info stmt_vinfo = vinfo_for_stmt (stmt);
+  enum tree_code code = gimple_expr_code (stmt);
+  tree op, vectype;
+  gimple *def_stmt;
+  enum vect_def_type dt;
+
+  /* For comparison and COND_EXPR type is chosen depending
+     on the other comparison operand.  */
+  if (TREE_CODE_CLASS (code) == tcc_comparison)
+    {
+      if (opnum)
+	op = gimple_assign_rhs1 (stmt);
+      else
+	op = gimple_assign_rhs2 (stmt);
+
+      if (!vect_is_simple_use (op, stmt_vinfo->vinfo, &def_stmt,
+			       &dt, &vectype))
+	gcc_unreachable ();
+
+      return !vectype || VECTOR_BOOLEAN_TYPE_P (vectype);
+    }
+
+  if (code == COND_EXPR)
+    {
+      tree cond = gimple_assign_rhs1 (stmt);
+
+      if (TREE_CODE (cond) == SSA_NAME)
+	return false;
+
+      if (opnum)
+	op = TREE_OPERAND (cond, 1);
+      else
+	op = TREE_OPERAND (cond, 0);
+
+      if (!vect_is_simple_use (op, stmt_vinfo->vinfo, &def_stmt,
+			       &dt, &vectype))
+	gcc_unreachable ();
+
+      return !vectype || VECTOR_BOOLEAN_TYPE_P (vectype);
+    }
+
+  return VECTOR_BOOLEAN_TYPE_P (STMT_VINFO_VECTYPE (stmt_vinfo));
+}
+
+
 /* For constant and loop invariant defs of SLP_NODE this function returns
    (vector) defs (VEC_OPRNDS) that will be used in the vectorized stmts.
    OP_NUM determines if we gather defs for operand 0 or operand 1 of the RHS of
@@ -2625,8 +2676,7 @@ vect_get_constant_vectors (tree op, slp_tree slp_node,
 
   /* Check if vector type is a boolean vector.  */
   if (TREE_CODE (TREE_TYPE (op)) == BOOLEAN_TYPE
-      && (VECTOR_BOOLEAN_TYPE_P (STMT_VINFO_VECTYPE (stmt_vinfo))
-	  || (code == COND_EXPR && op_num < 2)))
+      && vect_mask_constant_operand_p (stmt, op_num))
     vector_type
       = build_same_sized_truth_vector_type (STMT_VINFO_VECTYPE (stmt_vinfo));
   else
diff --git a/gcc/tree-vect-stmts.c b/gcc/tree-vect-stmts.c
index cfe30e0..c990dfa 100644
--- a/gcc/tree-vect-stmts.c
+++ b/gcc/tree-vect-stmts.c
@@ -7642,8 +7642,8 @@ vectorizable_comparison (gimple *stmt, gimple_stmt_iterator *gsi,
 	    }
 	  else
 	    {
-	      vec_rhs1 = vect_get_vec_def_for_operand (rhs1, stmt, NULL);
-	      vec_rhs2 = vect_get_vec_def_for_operand (rhs2, stmt, NULL);
+	      vec_rhs1 = vect_get_vec_def_for_operand (rhs1, stmt, vectype);
+	      vec_rhs2 = vect_get_vec_def_for_operand (rhs2, stmt, vectype);
 	    }
 	}
       else

Re: [PATCH] Fix ICE for boolean comparison

[Richard Biener]

On Thu, Nov 12, 2015 at 4:44 PM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote:
> Hi,
>
> Currently compiler may ICE when loaded boolean is compared with vector invariant or another boolean value.  This is because we don't detect mix of bool and non-bool vectypes and incorrectly determine vectype for boolean loop invariant for comparison.  This was fixed for COND_EXP before but also needs to be fixed for comparison.  This patch was bootstrapped and tested on x86_64-unknown-linux-gnu.  OK for trunk?

Hmm, so this disables vectorization in these cases.  Isn't this a
regression?  Shouldn't we simply "materialize"
the non-bool vector from the boolean one say, with

 vec = boolvec ? {-1, -1 ... } : {0, 0, 0 ...}

?

Thanks,
Richard.

> Thanks,
> Ilya
> --
> gcc/
>
> 2015-11-12  Ilya Enkovich  <enkovich.gnu@gmail.com>
>
>         * tree-vect-loop.c (vect_determine_vectorization_factor): Check
>         mix of boolean and integer vectors in a single statement.
>         * tree-vect-slp.c (vect_mask_constant_operand_p): New.
>         (vect_get_constant_vectors): Use vect_mask_constant_operand_p to
>         determine constant type.
>         * tree-vect-stmts.c (vectorizable_comparison): Provide vectype
>         for loop invariants.
>
> gcc/testsuite/
>
> 2015-11-12  Ilya Enkovich  <enkovich.gnu@gmail.com>
>
>         * g++.dg/vect/simd-bool-comparison-1.cc: New test.
>         * g++.dg/vect/simd-bool-comparison-2.cc: New test.
>
>
> diff --git a/gcc/testsuite/g++.dg/vect/simd-bool-comparison-1.cc b/gcc/testsuite/g++.dg/vect/simd-bool-comparison-1.cc
> new file mode 100644
> index 0000000..a08362f
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/vect/simd-bool-comparison-1.cc
> @@ -0,0 +1,21 @@
> +// { dg-do compile }
> +// { dg-additional-options "-mavx512bw -mavx512dq" { target { i?86-*-* x86_64-*-* } } }
> +
> +#define N 1024
> +
> +double a[N];
> +bool b[N];
> +bool c;
> +
> +void test ()
> +{
> +  int i;
> +
> +  for (i = 0; i < N; i++)
> +    if (b[i] != c)
> +      a[i] = 0.0;
> +    else
> +      a[i] = 1.0;
> +}
> +
> +// { dg-final { scan-tree-dump-times "vectorized 1 loops" 1 "vect" { target { i?86-*-* x86_64-*-* } } } }
> diff --git a/gcc/testsuite/g++.dg/vect/simd-bool-comparison-2.cc b/gcc/testsuite/g++.dg/vect/simd-bool-comparison-2.cc
> new file mode 100644
> index 0000000..4accf56
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/vect/simd-bool-comparison-2.cc
> @@ -0,0 +1,20 @@
> +// { dg-do compile }
> +// { dg-additional-options "-mavx512bw -mavx512dq" { target { i?86-*-* x86_64-*-* } } }
> +
> +#define N 1024
> +
> +double a[N];
> +bool b[N];
> +char c[N];
> +
> +void test ()
> +{
> +  int i;
> +
> +  #pragma omp simd
> +  for (i = 0; i < N; i++)
> +    if ((c[i] > 0) && b[i])
> +      a[i] = 0.0;
> +    else
> +      a[i] = 1.0;
> +}
> diff --git a/gcc/tree-vect-loop.c b/gcc/tree-vect-loop.c
> index 55e5309..6b78b55 100644
> --- a/gcc/tree-vect-loop.c
> +++ b/gcc/tree-vect-loop.c
> @@ -649,7 +649,32 @@ vect_determine_vectorization_factor (loop_vec_info loop_vinfo)
>                     }
>                   return false;
>                 }
> +             else if (VECTOR_BOOLEAN_TYPE_P (mask_type)
> +                      != VECTOR_BOOLEAN_TYPE_P (vectype))
> +               {
> +                 if (dump_enabled_p ())
> +                   {
> +                     dump_printf_loc (MSG_MISSED_OPTIMIZATION, vect_location,
> +                                      "not vectorized: mixed mask and "
> +                                      "nonmask vector types in statement, ");
> +                     dump_generic_expr (MSG_MISSED_OPTIMIZATION, TDF_SLIM,
> +                                        mask_type);
> +                     dump_printf (MSG_MISSED_OPTIMIZATION, " and ");
> +                     dump_generic_expr (MSG_MISSED_OPTIMIZATION, TDF_SLIM,
> +                                        vectype);
> +                     dump_printf (MSG_MISSED_OPTIMIZATION, "\n");
> +                   }
> +                 return false;
> +               }
>             }
> +
> +         /* We may compare boolean value loaded as vector of integers.
> +            Fix mask_type in such case.  */
> +         if (mask_type
> +             && !VECTOR_BOOLEAN_TYPE_P (mask_type)
> +             && gimple_code (stmt) == GIMPLE_ASSIGN
> +             && TREE_CODE_CLASS (gimple_assign_rhs_code (stmt)) == tcc_comparison)
> +           mask_type = build_same_sized_truth_vector_type (mask_type);
>         }
>
>        /* No mask_type should mean loop invariant predicate.
> diff --git a/gcc/tree-vect-slp.c b/gcc/tree-vect-slp.c
> index 9d97140..f3acb04 100644
> --- a/gcc/tree-vect-slp.c
> +++ b/gcc/tree-vect-slp.c
> @@ -2589,6 +2589,57 @@ vect_slp_bb (basic_block bb)
>  }
>
>
> +/* Return 1 if vector type of boolean constant which is OPNUM
> +   operand in statement STMT is a boolean vector.  */
> +
> +static bool
> +vect_mask_constant_operand_p (gimple *stmt, int opnum)
> +{
> +  stmt_vec_info stmt_vinfo = vinfo_for_stmt (stmt);
> +  enum tree_code code = gimple_expr_code (stmt);
> +  tree op, vectype;
> +  gimple *def_stmt;
> +  enum vect_def_type dt;
> +
> +  /* For comparison and COND_EXPR type is chosen depending
> +     on the other comparison operand.  */
> +  if (TREE_CODE_CLASS (code) == tcc_comparison)
> +    {
> +      if (opnum)
> +       op = gimple_assign_rhs1 (stmt);
> +      else
> +       op = gimple_assign_rhs2 (stmt);
> +
> +      if (!vect_is_simple_use (op, stmt_vinfo->vinfo, &def_stmt,
> +                              &dt, &vectype))
> +       gcc_unreachable ();
> +
> +      return !vectype || VECTOR_BOOLEAN_TYPE_P (vectype);
> +    }
> +
> +  if (code == COND_EXPR)
> +    {
> +      tree cond = gimple_assign_rhs1 (stmt);
> +
> +      if (TREE_CODE (cond) == SSA_NAME)
> +       return false;
> +
> +      if (opnum)
> +       op = TREE_OPERAND (cond, 1);
> +      else
> +       op = TREE_OPERAND (cond, 0);
> +
> +      if (!vect_is_simple_use (op, stmt_vinfo->vinfo, &def_stmt,
> +                              &dt, &vectype))
> +       gcc_unreachable ();
> +
> +      return !vectype || VECTOR_BOOLEAN_TYPE_P (vectype);
> +    }
> +
> +  return VECTOR_BOOLEAN_TYPE_P (STMT_VINFO_VECTYPE (stmt_vinfo));
> +}
> +
> +
>  /* For constant and loop invariant defs of SLP_NODE this function returns
>     (vector) defs (VEC_OPRNDS) that will be used in the vectorized stmts.
>     OP_NUM determines if we gather defs for operand 0 or operand 1 of the RHS of
> @@ -2625,8 +2676,7 @@ vect_get_constant_vectors (tree op, slp_tree slp_node,
>
>    /* Check if vector type is a boolean vector.  */
>    if (TREE_CODE (TREE_TYPE (op)) == BOOLEAN_TYPE
> -      && (VECTOR_BOOLEAN_TYPE_P (STMT_VINFO_VECTYPE (stmt_vinfo))
> -         || (code == COND_EXPR && op_num < 2)))
> +      && vect_mask_constant_operand_p (stmt, op_num))
>      vector_type
>        = build_same_sized_truth_vector_type (STMT_VINFO_VECTYPE (stmt_vinfo));
>    else
> diff --git a/gcc/tree-vect-stmts.c b/gcc/tree-vect-stmts.c
> index cfe30e0..c990dfa 100644
> --- a/gcc/tree-vect-stmts.c
> +++ b/gcc/tree-vect-stmts.c
> @@ -7642,8 +7642,8 @@ vectorizable_comparison (gimple *stmt, gimple_stmt_iterator *gsi,
>             }
>           else
>             {
> -             vec_rhs1 = vect_get_vec_def_for_operand (rhs1, stmt, NULL);
> -             vec_rhs2 = vect_get_vec_def_for_operand (rhs2, stmt, NULL);
> +             vec_rhs1 = vect_get_vec_def_for_operand (rhs1, stmt, vectype);
> +             vec_rhs2 = vect_get_vec_def_for_operand (rhs2, stmt, vectype);
>             }
>         }
>        else

Re: [PATCH] Fix ICE for boolean comparison

[Ilya Enkovich]

2015-11-13 13:38 GMT+03:00 Richard Biener <richard.guenther@gmail.com>:
> On Thu, Nov 12, 2015 at 4:44 PM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote:
>> Hi,
>>
>> Currently compiler may ICE when loaded boolean is compared with vector invariant or another boolean value.  This is because we don't detect mix of bool and non-bool vectypes and incorrectly determine vectype for boolean loop invariant for comparison.  This was fixed for COND_EXP before but also needs to be fixed for comparison.  This patch was bootstrapped and tested on x86_64-unknown-linux-gnu.  OK for trunk?
>
> Hmm, so this disables vectorization in these cases.  Isn't this a
> regression?  Shouldn't we simply "materialize"
> the non-bool vector from the boolean one say, with
>
>  vec = boolvec ? {-1, -1 ... } : {0, 0, 0 ...}

We may do this using patterns, but still should catch cases when
patterns don't catch it. Patterns don't have vectypes computed and
therefore may miss such cases. Thus stability fix is still valid.

I don't think we have a compiler version which can vectorize
simd-bool-comparison-2.cc, thus technically it is not a regression.
There are also other similar cases, e.g. store of comparison result or
use loaded boolean as a predicate. I was going to support
vectorization for such cases later (seems I don't hit stage1 for them
and not sure if it will be OK for stage3).

Ilya

>
> ?
>
> Thanks,
> Richard.
>
>> Thanks,
>> Ilya

Re: [PATCH] Fix ICE for boolean comparison

[Richard Biener]

On Fri, Nov 13, 2015 at 11:52 AM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote:
> 2015-11-13 13:38 GMT+03:00 Richard Biener <richard.guenther@gmail.com>:
>> On Thu, Nov 12, 2015 at 4:44 PM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote:
>>> Hi,
>>>
>>> Currently compiler may ICE when loaded boolean is compared with vector invariant or another boolean value.  This is because we don't detect mix of bool and non-bool vectypes and incorrectly determine vectype for boolean loop invariant for comparison.  This was fixed for COND_EXP before but also needs to be fixed for comparison.  This patch was bootstrapped and tested on x86_64-unknown-linux-gnu.  OK for trunk?
>>
>> Hmm, so this disables vectorization in these cases.  Isn't this a
>> regression?  Shouldn't we simply "materialize"
>> the non-bool vector from the boolean one say, with
>>
>>  vec = boolvec ? {-1, -1 ... } : {0, 0, 0 ...}
>
> We may do this using patterns, but still should catch cases when
> patterns don't catch it. Patterns don't have vectypes computed and
> therefore may miss such cases. Thus stability fix is still valid.
>
> I don't think we have a compiler version which can vectorize
> simd-bool-comparison-2.cc, thus technically it is not a regression.
> There are also other similar cases, e.g. store of comparison result or
> use loaded boolean as a predicate. I was going to support
> vectorization for such cases later (seems I don't hit stage1 for them
> and not sure if it will be OK for stage3).

I still think those checks show that there is an issue we should fix
differently.  We're accumulating more mess into the already messy
vectorizer :(

Ok.

Thanks,
Richard.

> Ilya
>
>>
>> ?
>>
>> Thanks,
>> Richard.
>>
>>> Thanks,
>>> Ilya

Re: [PATCH] Fix ICE for boolean comparison

[Ilya Enkovich]

2015-11-13 14:28 GMT+03:00 Richard Biener <richard.guenther@gmail.com>:
> On Fri, Nov 13, 2015 at 11:52 AM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote:
>> 2015-11-13 13:38 GMT+03:00 Richard Biener <richard.guenther@gmail.com>:
>>> On Thu, Nov 12, 2015 at 4:44 PM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote:
>>>> Hi,
>>>>
>>>> Currently compiler may ICE when loaded boolean is compared with vector invariant or another boolean value.  This is because we don't detect mix of bool and non-bool vectypes and incorrectly determine vectype for boolean loop invariant for comparison.  This was fixed for COND_EXP before but also needs to be fixed for comparison.  This patch was bootstrapped and tested on x86_64-unknown-linux-gnu.  OK for trunk?
>>>
>>> Hmm, so this disables vectorization in these cases.  Isn't this a
>>> regression?  Shouldn't we simply "materialize"
>>> the non-bool vector from the boolean one say, with
>>>
>>>  vec = boolvec ? {-1, -1 ... } : {0, 0, 0 ...}
>>
>> We may do this using patterns, but still should catch cases when
>> patterns don't catch it. Patterns don't have vectypes computed and
>> therefore may miss such cases. Thus stability fix is still valid.
>>
>> I don't think we have a compiler version which can vectorize
>> simd-bool-comparison-2.cc, thus technically it is not a regression.
>> There are also other similar cases, e.g. store of comparison result or
>> use loaded boolean as a predicate. I was going to support
>> vectorization for such cases later (seems I don't hit stage1 for them
>> and not sure if it will be OK for stage3).
>
> I still think those checks show that there is an issue we should fix
> differently.  We're accumulating more mess into the already messy
> vectorizer :(

Right. Earlier vectype computation would let to reveal such cases more easily.

>
> Ok.

Thanks!
Ilya

>
> Thanks,
> Richard.
>
>> Ilya
>>
>>>
>>> ?
>>>
>>> Thanks,
>>> Richard.
>>>
>>>> Thanks,
>>>> Ilya