[PATCH] Handle vector COND_EXPRs in vector genericization (PR tree-optimization/65427)

[PATCH] Handle vector COND_EXPRs in vector genericization (PR tree-optimization/65427)

[Jakub Jelinek]

Hi!

On the following testcase, gimple LIM creates a vector COND_EXPR (scalar
condition, vector lhs, rhs2 and rhs3), but if we don't have corresponding
vector mode for it, we ICE trying to expand the BLKmode COND_EXPR, as it is
unprepared for that.

This patch lowers those (parallel or piecewise).
Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2015-03-16  Jakub Jelinek  <jakub@redhat.com>

	PR tree-optimization/65427
	* tree-vect-generic.c (do_cond, expand_vector_scalar_condition): New
	functions.
	(expand_vector_operations_1): Handle BLKmode vector COND_EXPR.

	* gcc.c-torture/execute/pr65427.c: New test.

--- gcc/tree-vect-generic.c.jj	2015-01-15 20:25:40.000000000 +0100
+++ gcc/tree-vect-generic.c	2015-03-16 14:25:37.391932269 +0100
@@ -1417,6 +1417,57 @@ count_type_subparts (tree type)
   return VECTOR_TYPE_P (type) ? TYPE_VECTOR_SUBPARTS (type) : 1;
 }
 
+static tree
+do_cond (gimple_stmt_iterator *gsi, tree inner_type, tree a, tree b,
+	 tree bitpos, tree bitsize, enum tree_code code)
+{
+  if (TREE_CODE (TREE_TYPE (a)) == VECTOR_TYPE)
+    a = tree_vec_extract (gsi, inner_type, a, bitsize, bitpos);
+  if (TREE_CODE (TREE_TYPE (b)) == VECTOR_TYPE)
+    b = tree_vec_extract (gsi, inner_type, b, bitsize, bitpos);
+  tree cond = gimple_assign_rhs1 (gsi_stmt (*gsi));
+  return gimplify_build3 (gsi, code, inner_type, cond, a, b);
+}
+
+/* Expand a vector COND_EXPR to scalars, piecewise.  */
+static void
+expand_vector_scalar_condition (gimple_stmt_iterator *gsi)
+{
+  gassign *stmt = as_a <gassign *> (gsi_stmt (*gsi));
+  tree type = gimple_expr_type (stmt);
+  tree compute_type = get_compute_type (COND_EXPR, mov_optab, type);
+  machine_mode compute_mode = TYPE_MODE (compute_type);
+  gcc_assert (compute_mode != BLKmode);
+  tree lhs = gimple_assign_lhs (stmt);
+  tree rhs2 = gimple_assign_rhs2 (stmt);
+  tree rhs3 = gimple_assign_rhs3 (stmt);
+  tree new_rhs;
+
+  /* If the compute mode is not a vector mode (hence we are not decomposing
+     a BLKmode vector to smaller, hardware-supported vectors), we may want
+     to expand the operations in parallel.  */
+  if (GET_MODE_CLASS (compute_mode) != MODE_VECTOR_INT
+      && GET_MODE_CLASS (compute_mode) != MODE_VECTOR_FLOAT
+      && GET_MODE_CLASS (compute_mode) != MODE_VECTOR_FRACT
+      && GET_MODE_CLASS (compute_mode) != MODE_VECTOR_UFRACT
+      && GET_MODE_CLASS (compute_mode) != MODE_VECTOR_ACCUM
+      && GET_MODE_CLASS (compute_mode) != MODE_VECTOR_UACCUM)
+    new_rhs = expand_vector_parallel (gsi, do_cond, type, rhs2, rhs3,
+				      COND_EXPR);
+  else
+    new_rhs = expand_vector_piecewise (gsi, do_cond, type, compute_type,
+				       rhs2, rhs3, COND_EXPR);
+  if (!useless_type_conversion_p (TREE_TYPE (lhs), TREE_TYPE (new_rhs)))
+    new_rhs = gimplify_build1 (gsi, VIEW_CONVERT_EXPR, TREE_TYPE (lhs),
+			       new_rhs);
+
+  /* NOTE:  We should avoid using gimple_assign_set_rhs_from_tree. One
+     way to do it is change expand_vector_operation and its callees to
+     return a tree_code, RHS1 and RHS2 instead of a tree. */
+  gimple_assign_set_rhs_from_tree (gsi, new_rhs);
+  update_stmt (gsi_stmt (*gsi));
+}
+
 /* Process one statement.  If we identify a vector operation, expand it.  */
 
 static void
@@ -1449,6 +1500,14 @@ expand_vector_operations_1 (gimple_stmt_
       return;
     }
 
+  if (code == COND_EXPR
+      && TREE_CODE (TREE_TYPE (gimple_assign_lhs (stmt))) == VECTOR_TYPE
+      && TYPE_MODE (TREE_TYPE (gimple_assign_lhs (stmt))) == BLKmode)
+    {
+      expand_vector_scalar_condition (gsi);
+      return;
+    }
+
   if (code == CONSTRUCTOR
       && TREE_CODE (lhs) == SSA_NAME
       && VECTOR_MODE_P (TYPE_MODE (TREE_TYPE (lhs)))
--- gcc/testsuite/gcc.c-torture/execute/pr65427.c.jj	2015-03-16 14:36:29.489254701 +0100
+++ gcc/testsuite/gcc.c-torture/execute/pr65427.c	2015-03-16 14:40:58.789851433 +0100
@@ -0,0 +1,34 @@
+/* PR tree-optimization/65427 */
+
+typedef int V __attribute__ ((vector_size (8 * sizeof (int))));
+V a, b, c, d, e, f;
+
+__attribute__((noinline, noclone)) void
+foo (int x, int y)
+{
+  do
+    {
+      if (x)
+	d = a ^ c;
+      else
+	d = a ^ b;
+    }
+  while (y);
+}
+
+int
+main ()
+{
+  a = (V) { 1, 2, 3, 4, 5, 6, 7, 8 };
+  b = (V) { 0x40, 0x80, 0x40, 0x80, 0x40, 0x80, 0x40, 0x80 };
+  e = (V) { 0x41, 0x82, 0x43, 0x84, 0x45, 0x86, 0x47, 0x88 };
+  foo (0, 0);
+  if (__builtin_memcmp (&d, &e, sizeof (V)) != 0)
+    __builtin_abort ();
+  c = (V) { 0x80, 0x40, 0x80, 0x40, 0x80, 0x40, 0x80, 0x40 };
+  f = (V) { 0x81, 0x42, 0x83, 0x44, 0x85, 0x46, 0x87, 0x48 };
+  foo (1, 0);
+  if (__builtin_memcmp (&d, &f, sizeof (V)) != 0)
+    __builtin_abort ();
+  return 0;
+}

	Jakub

Re: [PATCH] Handle vector COND_EXPRs in vector genericization (PR tree-optimization/65427)

[Richard Biener]

On March 16, 2015 5:21:02 PM GMT+01:00, Jakub Jelinek <jakub@redhat.com> wrote:
>Hi!
>
>On the following testcase, gimple LIM creates a vector COND_EXPR
>(scalar
>condition, vector lhs, rhs2 and rhs3), but if we don't have
>corresponding
>vector mode for it, we ICE trying to expand the BLKmode COND_EXPR, as
>it is
>unprepared for that.
>
>This patch lowers those (parallel or piecewise).
>Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

OK.  Though maybe LIM should not create these for cost reasons? I also wonder if we should lower them to control flow?

Thanks,
Richard.

>2015-03-16  Jakub Jelinek  <jakub@redhat.com>
>
>	PR tree-optimization/65427
>	* tree-vect-generic.c (do_cond, expand_vector_scalar_condition): New
>	functions.
>	(expand_vector_operations_1): Handle BLKmode vector COND_EXPR.
>
>	* gcc.c-torture/execute/pr65427.c: New test.
>
>--- gcc/tree-vect-generic.c.jj	2015-01-15 20:25:40.000000000 +0100
>+++ gcc/tree-vect-generic.c	2015-03-16 14:25:37.391932269 +0100
>@@ -1417,6 +1417,57 @@ count_type_subparts (tree type)
>   return VECTOR_TYPE_P (type) ? TYPE_VECTOR_SUBPARTS (type) : 1;
> }
> 
>+static tree
>+do_cond (gimple_stmt_iterator *gsi, tree inner_type, tree a, tree b,
>+	 tree bitpos, tree bitsize, enum tree_code code)
>+{
>+  if (TREE_CODE (TREE_TYPE (a)) == VECTOR_TYPE)
>+    a = tree_vec_extract (gsi, inner_type, a, bitsize, bitpos);
>+  if (TREE_CODE (TREE_TYPE (b)) == VECTOR_TYPE)
>+    b = tree_vec_extract (gsi, inner_type, b, bitsize, bitpos);
>+  tree cond = gimple_assign_rhs1 (gsi_stmt (*gsi));
>+  return gimplify_build3 (gsi, code, inner_type, cond, a, b);
>+}
>+
>+/* Expand a vector COND_EXPR to scalars, piecewise.  */
>+static void
>+expand_vector_scalar_condition (gimple_stmt_iterator *gsi)
>+{
>+  gassign *stmt = as_a <gassign *> (gsi_stmt (*gsi));
>+  tree type = gimple_expr_type (stmt);
>+  tree compute_type = get_compute_type (COND_EXPR, mov_optab, type);
>+  machine_mode compute_mode = TYPE_MODE (compute_type);
>+  gcc_assert (compute_mode != BLKmode);
>+  tree lhs = gimple_assign_lhs (stmt);
>+  tree rhs2 = gimple_assign_rhs2 (stmt);
>+  tree rhs3 = gimple_assign_rhs3 (stmt);
>+  tree new_rhs;
>+
>+  /* If the compute mode is not a vector mode (hence we are not
>decomposing
>+     a BLKmode vector to smaller, hardware-supported vectors), we may
>want
>+     to expand the operations in parallel.  */
>+  if (GET_MODE_CLASS (compute_mode) != MODE_VECTOR_INT
>+      && GET_MODE_CLASS (compute_mode) != MODE_VECTOR_FLOAT
>+      && GET_MODE_CLASS (compute_mode) != MODE_VECTOR_FRACT
>+      && GET_MODE_CLASS (compute_mode) != MODE_VECTOR_UFRACT
>+      && GET_MODE_CLASS (compute_mode) != MODE_VECTOR_ACCUM
>+      && GET_MODE_CLASS (compute_mode) != MODE_VECTOR_UACCUM)
>+    new_rhs = expand_vector_parallel (gsi, do_cond, type, rhs2, rhs3,
>+				      COND_EXPR);
>+  else
>+    new_rhs = expand_vector_piecewise (gsi, do_cond, type,
>compute_type,
>+				       rhs2, rhs3, COND_EXPR);
>+  if (!useless_type_conversion_p (TREE_TYPE (lhs), TREE_TYPE
>(new_rhs)))
>+    new_rhs = gimplify_build1 (gsi, VIEW_CONVERT_EXPR, TREE_TYPE
>(lhs),
>+			       new_rhs);
>+
>+  /* NOTE:  We should avoid using gimple_assign_set_rhs_from_tree. One
>+     way to do it is change expand_vector_operation and its callees to
>+     return a tree_code, RHS1 and RHS2 instead of a tree. */
>+  gimple_assign_set_rhs_from_tree (gsi, new_rhs);
>+  update_stmt (gsi_stmt (*gsi));
>+}
>+
>/* Process one statement.  If we identify a vector operation, expand
>it.  */
> 
> static void
>@@ -1449,6 +1500,14 @@ expand_vector_operations_1 (gimple_stmt_
>       return;
>     }
> 
>+  if (code == COND_EXPR
>+      && TREE_CODE (TREE_TYPE (gimple_assign_lhs (stmt))) ==
>VECTOR_TYPE
>+      && TYPE_MODE (TREE_TYPE (gimple_assign_lhs (stmt))) == BLKmode)
>+    {
>+      expand_vector_scalar_condition (gsi);
>+      return;
>+    }
>+
>   if (code == CONSTRUCTOR
>       && TREE_CODE (lhs) == SSA_NAME
>       && VECTOR_MODE_P (TYPE_MODE (TREE_TYPE (lhs)))
>--- gcc/testsuite/gcc.c-torture/execute/pr65427.c.jj	2015-03-16
>14:36:29.489254701 +0100
>+++ gcc/testsuite/gcc.c-torture/execute/pr65427.c	2015-03-16
>14:40:58.789851433 +0100
>@@ -0,0 +1,34 @@
>+/* PR tree-optimization/65427 */
>+
>+typedef int V __attribute__ ((vector_size (8 * sizeof (int))));
>+V a, b, c, d, e, f;
>+
>+__attribute__((noinline, noclone)) void
>+foo (int x, int y)
>+{
>+  do
>+    {
>+      if (x)
>+	d = a ^ c;
>+      else
>+	d = a ^ b;
>+    }
>+  while (y);
>+}
>+
>+int
>+main ()
>+{
>+  a = (V) { 1, 2, 3, 4, 5, 6, 7, 8 };
>+  b = (V) { 0x40, 0x80, 0x40, 0x80, 0x40, 0x80, 0x40, 0x80 };
>+  e = (V) { 0x41, 0x82, 0x43, 0x84, 0x45, 0x86, 0x47, 0x88 };
>+  foo (0, 0);
>+  if (__builtin_memcmp (&d, &e, sizeof (V)) != 0)
>+    __builtin_abort ();
>+  c = (V) { 0x80, 0x40, 0x80, 0x40, 0x80, 0x40, 0x80, 0x40 };
>+  f = (V) { 0x81, 0x42, 0x83, 0x44, 0x85, 0x46, 0x87, 0x48 };
>+  foo (1, 0);
>+  if (__builtin_memcmp (&d, &f, sizeof (V)) != 0)
>+    __builtin_abort ();
>+  return 0;
>+}
>
>	Jakub

Re: [PATCH] Handle vector COND_EXPRs in vector genericization (PR tree-optimization/65427)

[Jakub Jelinek]

On Mon, Mar 16, 2015 at 07:15:59PM +0100, Richard Biener wrote:
> On March 16, 2015 5:21:02 PM GMT+01:00, Jakub Jelinek <jakub@redhat.com> wrote:
> >On the following testcase, gimple LIM creates a vector COND_EXPR
> >(scalar
> >condition, vector lhs, rhs2 and rhs3), but if we don't have
> >corresponding
> >vector mode for it, we ICE trying to expand the BLKmode COND_EXPR, as
> >it is
> >unprepared for that.
> >
> >This patch lowers those (parallel or piecewise).
> >Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> 
> OK.  Though maybe LIM should not create these for cost reasons? I also wonder if we should lower them to control flow?

Yeah, I've been thinking about teaching LIM not to do that if it is BLKmode.
But then found how many other spots create COND_EXPRs and thought I just
can't catch all of them.  But guess I can change LIM incrementally too.

I've also been thinking about lowering it to control flow, but only if it
couldn't be done in say two halves comparison as in the testcase.  I suppose
doing say 2 V4SImode COND_EXPRs would still be beneficial over control flow,
but if it is more than that (say 4 or 8+) it might be already better to just
expand it as a PHI.  But, as we don't create basic blocks in
tree-vect-generic.c right now, I thought it might be too much for stage4.

	Jakub

Re: [PATCH] Handle vector COND_EXPRs in vector genericization (PR tree-optimization/65427)

[Richard Biener]

On March 16, 2015 7:26:58 PM GMT+01:00, Jakub Jelinek <jakub@redhat.com> wrote:
>On Mon, Mar 16, 2015 at 07:15:59PM +0100, Richard Biener wrote:
>> On March 16, 2015 5:21:02 PM GMT+01:00, Jakub Jelinek
><jakub@redhat.com> wrote:
>> >On the following testcase, gimple LIM creates a vector COND_EXPR
>> >(scalar
>> >condition, vector lhs, rhs2 and rhs3), but if we don't have
>> >corresponding
>> >vector mode for it, we ICE trying to expand the BLKmode COND_EXPR,
>as
>> >it is
>> >unprepared for that.
>> >
>> >This patch lowers those (parallel or piecewise).
>> >Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
>> 
>> OK.  Though maybe LIM should not create these for cost reasons? I
>also wonder if we should lower them to control flow?
>
>Yeah, I've been thinking about teaching LIM not to do that if it is
>BLKmode.
>But then found how many other spots create COND_EXPRs and thought I
>just
>can't catch all of them.  But guess I can change LIM incrementally too.
>
>I've also been thinking about lowering it to control flow, but only if
>it
>couldn't be done in say two halves comparison as in the testcase.  I
>suppose
>doing say 2 V4SImode COND_EXPRs would still be beneficial over control
>flow,
>but if it is more than that (say 4 or 8+) it might be already better to
>just
>expand it as a PHI.  But, as we don't create basic blocks in
>tree-vect-generic.c right now, I thought it might be too much for
>stage4.

Yeah, the patch is fine for stage 4.  I just wrote down my random thoughts.

Richard.

>	Jakub

Re: [PATCH] Handle vector COND_EXPRs in vector genericization (PR tree-optimization/65427)

[Kyrill Tkachov]

On 16/03/15 18:26, Jakub Jelinek wrote:
> On Mon, Mar 16, 2015 at 07:15:59PM +0100, Richard Biener wrote:
>> On March 16, 2015 5:21:02 PM GMT+01:00, Jakub Jelinek <jakub@redhat.com> wrote:
>>> On the following testcase, gimple LIM creates a vector COND_EXPR
>>> (scalar
>>> condition, vector lhs, rhs2 and rhs3), but if we don't have
>>> corresponding
>>> vector mode for it, we ICE trying to expand the BLKmode COND_EXPR, as
>>> it is
>>> unprepared for that.
>>>
>>> This patch lowers those (parallel or piecewise).
>>> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
>> OK.  Though maybe LIM should not create these for cost reasons? I also wonder if we should lower them to control flow?
> Yeah, I've been thinking about teaching LIM not to do that if it is BLKmode.
> But then found how many other spots create COND_EXPRs and thought I just
> can't catch all of them.  But guess I can change LIM incrementally too.
>
> I've also been thinking about lowering it to control flow, but only if it
> couldn't be done in say two halves comparison as in the testcase.  I suppose
> doing say 2 V4SImode COND_EXPRs would still be beneficial over control flow,
> but if it is more than that (say 4 or 8+) it might be already better to just
> expand it as a PHI.  But, as we don't create basic blocks in
> tree-vect-generic.c right now, I thought it might be too much for stage4.

Hi Jakub,
The testcase ICEs on arm-none-eabi at -Os (only):
0x7e6f11 process_insert_insn
         $TOP/gcc/gcse.c:2174
0x7e8a80 insert_insn_end_basic_block
         $TOP/gcc/gcse.c:2196
0x7eab01 hoist_code
         $TOP/gcc/gcse.c:3492
0x7eab01 one_code_hoisting_pass
         $TOP/gcc/gcse.c:3722
0x7eab01 execute_rtl_hoist
         $TOP/gcc/gcse.c:4212
0x7eab01 execute
         $TOP/gcc/gcse.c:4293

Thanks,
Kyrill

> 	Jakub
>

Re: [PATCH] Handle vector COND_EXPRs in vector genericization (PR tree-optimization/65427)

[Jakub Jelinek]

On Wed, Mar 18, 2015 at 03:10:44PM +0000, Kyrill Tkachov wrote:
> >I've also been thinking about lowering it to control flow, but only if it
> >couldn't be done in say two halves comparison as in the testcase.  I suppose
> >doing say 2 V4SImode COND_EXPRs would still be beneficial over control flow,
> >but if it is more than that (say 4 or 8+) it might be already better to just
> >expand it as a PHI.  But, as we don't create basic blocks in
> >tree-vect-generic.c right now, I thought it might be too much for stage4.

> The testcase ICEs on arm-none-eabi at -Os (only):
> 0x7e6f11 process_insert_insn
>         $TOP/gcc/gcse.c:2174
> 0x7e8a80 insert_insn_end_basic_block
>         $TOP/gcc/gcse.c:2196
> 0x7eab01 hoist_code
>         $TOP/gcc/gcse.c:3492
> 0x7eab01 one_code_hoisting_pass
>         $TOP/gcc/gcse.c:3722
> 0x7eab01 execute_rtl_hoist
>         $TOP/gcc/gcse.c:4212
> 0x7eab01 execute
>         $TOP/gcc/gcse.c:4293

Must be some unrelated latent issue, either in arm backend or in gcse.
I think the lowering I've done could be very easily just present in user
code, so shouldn't be that hard to adjust the testcase so that it ICEs
already before the commit.

	Jakub

Re: [PATCH] Handle vector COND_EXPRs in vector genericization (PR tree-optimization/65427)

[Kyrill Tkachov]

On 18/03/15 15:14, Jakub Jelinek wrote:
> On Wed, Mar 18, 2015 at 03:10:44PM +0000, Kyrill Tkachov wrote:
>>> I've also been thinking about lowering it to control flow, but only if it
>>> couldn't be done in say two halves comparison as in the testcase.  I suppose
>>> doing say 2 V4SImode COND_EXPRs would still be beneficial over control flow,
>>> but if it is more than that (say 4 or 8+) it might be already better to just
>>> expand it as a PHI.  But, as we don't create basic blocks in
>>> tree-vect-generic.c right now, I thought it might be too much for stage4.
>> The testcase ICEs on arm-none-eabi at -Os (only):
>> 0x7e6f11 process_insert_insn
>>          $TOP/gcc/gcse.c:2174
>> 0x7e8a80 insert_insn_end_basic_block
>>          $TOP/gcc/gcse.c:2196
>> 0x7eab01 hoist_code
>>          $TOP/gcc/gcse.c:3492
>> 0x7eab01 one_code_hoisting_pass
>>          $TOP/gcc/gcse.c:3722
>> 0x7eab01 execute_rtl_hoist
>>          $TOP/gcc/gcse.c:4212
>> 0x7eab01 execute
>>          $TOP/gcc/gcse.c:4293
> Must be some unrelated latent issue, either in arm backend or in gcse.
> I think the lowering I've done could be very easily just present in user
> code, so shouldn't be that hard to adjust the testcase so that it ICEs
> already before the commit.

Yep, I'm seeing the same ICE in the testcsase before the patch.
Kyrill

>
> 	Jakub
>

Re: [PATCH] Handle vector COND_EXPRs in vector genericization (PR tree-optimization/65427)

[Kyrill Tkachov]

On 18/03/15 15:20, Kyrill Tkachov wrote:
> On 18/03/15 15:14, Jakub Jelinek wrote:
>> On Wed, Mar 18, 2015 at 03:10:44PM +0000, Kyrill Tkachov wrote:
>>>> I've also been thinking about lowering it to control flow, but only if it
>>>> couldn't be done in say two halves comparison as in the testcase.  I suppose
>>>> doing say 2 V4SImode COND_EXPRs would still be beneficial over control flow,
>>>> but if it is more than that (say 4 or 8+) it might be already better to just
>>>> expand it as a PHI.  But, as we don't create basic blocks in
>>>> tree-vect-generic.c right now, I thought it might be too much for stage4.
>>> The testcase ICEs on arm-none-eabi at -Os (only):
>>> 0x7e6f11 process_insert_insn
>>>           $TOP/gcc/gcse.c:2174
>>> 0x7e8a80 insert_insn_end_basic_block
>>>           $TOP/gcc/gcse.c:2196
>>> 0x7eab01 hoist_code
>>>           $TOP/gcc/gcse.c:3492
>>> 0x7eab01 one_code_hoisting_pass
>>>           $TOP/gcc/gcse.c:3722
>>> 0x7eab01 execute_rtl_hoist
>>>           $TOP/gcc/gcse.c:4212
>>> 0x7eab01 execute
>>>           $TOP/gcc/gcse.c:4293
>> Must be some unrelated latent issue, either in arm backend or in gcse.
>> I think the lowering I've done could be very easily just present in user
>> code, so shouldn't be that hard to adjust the testcase so that it ICEs
>> already before the commit.
> Yep, I'm seeing the same ICE in the testcsase before the patch.

With some help from James G, I've got a potential fix to gcse.c in testing.

Kyrill

> Kyrill
>
>> 	Jakub
>>
>
>