PR81815: Invalid conditional reduction

Message ID 87zib2pb7g.fsf@linaro.org
State New
Headers show

Commit Message

Richard Sandiford Aug. 14, 2017, 12:48 p.m.
We weren't checking whether the phi in a conditional reduction was
used by the condition itself (which isn't a case we handle).

Tested on aarch64-linux-gnu and x86_64-linux-gnu.  OK to install?

Thanks,
Richard


2017-08-11  Richard Sandiford  <richard.sandiford@linaro.org>

gcc/
	PR tree-optimization/81835
	* tree-vect-loop.c (vect_is_simple_reduction): Simply checks for
	the phi SSA_NAME.  Check that the condition in a COND_EXPR does
	not depend on the phi.

gcc/testsuite/
	PR tree-optimization/81835
	* gcc.dg/vect/pr81815.c: New test.

Comments

Richard Biener Aug. 15, 2017, 11:35 a.m. | #1
On Mon, Aug 14, 2017 at 2:48 PM, Richard Sandiford
<richard.sandiford@linaro.org> wrote:
> We weren't checking whether the phi in a conditional reduction was

> used by the condition itself (which isn't a case we handle).

>

> Tested on aarch64-linux-gnu and x86_64-linux-gnu.  OK to install?


Ok.

Richard.

> Thanks,

> Richard

>

>

> 2017-08-11  Richard Sandiford  <richard.sandiford@linaro.org>

>

> gcc/

>         PR tree-optimization/81835

>         * tree-vect-loop.c (vect_is_simple_reduction): Simply checks for

>         the phi SSA_NAME.  Check that the condition in a COND_EXPR does

>         not depend on the phi.

>

> gcc/testsuite/

>         PR tree-optimization/81835

>         * gcc.dg/vect/pr81815.c: New test.

>

> Index: gcc/tree-vect-loop.c

> ===================================================================

> --- gcc/tree-vect-loop.c        2017-08-04 11:41:05.710206063 +0100

> +++ gcc/tree-vect-loop.c        2017-08-14 13:46:28.336459006 +0100

> @@ -2690,15 +2690,15 @@ vect_is_simple_reduction (loop_vec_info

>    *double_reduc = false;

>    *v_reduc_type = TREE_CODE_REDUCTION;

>

> -  name = PHI_RESULT (phi);

> +  tree phi_name = PHI_RESULT (phi);

>    /* ???  If there are no uses of the PHI result the inner loop reduction

>       won't be detected as possibly double-reduction by vectorizable_reduction

>       because that tries to walk the PHI arg from the preheader edge which

>       can be constant.  See PR60382.  */

> -  if (has_zero_uses (name))

> +  if (has_zero_uses (phi_name))

>      return NULL;

>    nloop_uses = 0;

> -  FOR_EACH_IMM_USE_FAST (use_p, imm_iter, name)

> +  FOR_EACH_IMM_USE_FAST (use_p, imm_iter, phi_name)

>      {

>        gimple *use_stmt = USE_STMT (use_p);

>        if (is_gimple_debug (use_stmt))

> @@ -2847,10 +2847,7 @@ vect_is_simple_reduction (loop_vec_info

>       simply rewriting this into "res += -x[i]".  Avoid changing

>       gimple instruction for the first simple tests and only do this

>       if we're allowed to change code at all.  */

> -  if (code == MINUS_EXPR

> -      && ! ((op1 = gimple_assign_rhs2 (def_stmt))

> -           && TREE_CODE (op1) == SSA_NAME

> -           && SSA_NAME_DEF_STMT (op1) == phi))

> +  if (code == MINUS_EXPR && gimple_assign_rhs2 (def_stmt) != phi_name)

>      code = PLUS_EXPR;

>

>    if (code == COND_EXPR)

> @@ -2864,6 +2861,14 @@ vect_is_simple_reduction (loop_vec_info

>            op4 = TREE_OPERAND (op3, 1);

>            op3 = TREE_OPERAND (op3, 0);

>          }

> +      if (op3 == phi_name || op4 == phi_name)

> +       {

> +         if (dump_enabled_p ())

> +           report_vect_op (MSG_MISSED_OPTIMIZATION, def_stmt,

> +                           "reduction: condition depends on previous"

> +                           " iteration: ");

> +         return NULL;

> +       }

>

>        op1 = gimple_assign_rhs2 (def_stmt);

>        op2 = gimple_assign_rhs3 (def_stmt);

> Index: gcc/testsuite/gcc.dg/vect/pr81815.c

> ===================================================================

> --- /dev/null   2017-08-14 10:10:17.469973707 +0100

> +++ gcc/testsuite/gcc.dg/vect/pr81815.c 2017-08-14 13:46:28.334496436 +0100

> @@ -0,0 +1,26 @@

> +/* { dg-do run } */

> +

> +int __attribute__ ((noinline, noclone))

> +f (int *x, int n)

> +{

> +  int b = 13;

> +  for (int i = 0; i < n; ++i)

> +    {

> +      int next = x[i];

> +      b = b < 100 ? next : 200;

> +    }

> +  return b;

> +}

> +

> +static int res[32];

> +

> +int

> +main (void)

> +{

> +  for (int i = 0; i < 32; ++i)

> +    res[i] = i;

> +  res[15] = 100;

> +  if (f (res, 32) != 200)

> +    __builtin_abort ();

> +  return 0;

> +}

Patch

Index: gcc/tree-vect-loop.c
===================================================================
--- gcc/tree-vect-loop.c	2017-08-04 11:41:05.710206063 +0100
+++ gcc/tree-vect-loop.c	2017-08-14 13:46:28.336459006 +0100
@@ -2690,15 +2690,15 @@  vect_is_simple_reduction (loop_vec_info
   *double_reduc = false;
   *v_reduc_type = TREE_CODE_REDUCTION;
 
-  name = PHI_RESULT (phi);
+  tree phi_name = PHI_RESULT (phi);
   /* ???  If there are no uses of the PHI result the inner loop reduction
      won't be detected as possibly double-reduction by vectorizable_reduction
      because that tries to walk the PHI arg from the preheader edge which
      can be constant.  See PR60382.  */
-  if (has_zero_uses (name))
+  if (has_zero_uses (phi_name))
     return NULL;
   nloop_uses = 0;
-  FOR_EACH_IMM_USE_FAST (use_p, imm_iter, name)
+  FOR_EACH_IMM_USE_FAST (use_p, imm_iter, phi_name)
     {
       gimple *use_stmt = USE_STMT (use_p);
       if (is_gimple_debug (use_stmt))
@@ -2847,10 +2847,7 @@  vect_is_simple_reduction (loop_vec_info
      simply rewriting this into "res += -x[i]".  Avoid changing
      gimple instruction for the first simple tests and only do this
      if we're allowed to change code at all.  */
-  if (code == MINUS_EXPR
-      && ! ((op1 = gimple_assign_rhs2 (def_stmt))
-	    && TREE_CODE (op1) == SSA_NAME
-	    && SSA_NAME_DEF_STMT (op1) == phi))
+  if (code == MINUS_EXPR && gimple_assign_rhs2 (def_stmt) != phi_name)
     code = PLUS_EXPR;
 
   if (code == COND_EXPR)
@@ -2864,6 +2861,14 @@  vect_is_simple_reduction (loop_vec_info
           op4 = TREE_OPERAND (op3, 1);
           op3 = TREE_OPERAND (op3, 0);
         }
+      if (op3 == phi_name || op4 == phi_name)
+	{
+	  if (dump_enabled_p ())
+	    report_vect_op (MSG_MISSED_OPTIMIZATION, def_stmt,
+			    "reduction: condition depends on previous"
+			    " iteration: ");
+	  return NULL;
+	}
 
       op1 = gimple_assign_rhs2 (def_stmt);
       op2 = gimple_assign_rhs3 (def_stmt);
Index: gcc/testsuite/gcc.dg/vect/pr81815.c
===================================================================
--- /dev/null	2017-08-14 10:10:17.469973707 +0100
+++ gcc/testsuite/gcc.dg/vect/pr81815.c	2017-08-14 13:46:28.334496436 +0100
@@ -0,0 +1,26 @@ 
+/* { dg-do run } */
+
+int __attribute__ ((noinline, noclone))
+f (int *x, int n)
+{
+  int b = 13;
+  for (int i = 0; i < n; ++i)
+    {
+      int next = x[i];
+      b = b < 100 ? next : 200;
+    }
+  return b;
+}
+
+static int res[32];
+
+int
+main (void)
+{
+  for (int i = 0; i < 32; ++i)
+    res[i] = i;
+  res[15] = 100;
+  if (f (res, 32) != 200)
+    __builtin_abort ();
+  return 0;
+}