public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* [PATCH GCC]Do more tree if-conversions by handlding PHIs with more than two arguments.
@ 2016-04-27 15:49 Bin Cheng
  2016-04-28  9:18 ` Richard Biener
  0 siblings, 1 reply; 10+ messages in thread
From: Bin Cheng @ 2016-04-27 15:49 UTC (permalink / raw)
  To: gcc-patches; +Cc: nd

[-- Attachment #1: Type: text/plain, Size: 1774 bytes --]

Hi,
Currently tree if-conversion only supports PHIs with no more than two arguments unless the loop is marked with "simd pragma".  This patch makes such PHIs supported unconditionally if they have no more than MAX_PHI_ARG_NUM arguments, thus cases like PR56541 can be fixed.  Note because a chain of "?:" operators are needed to compute mult-arg PHI, this patch records the case and versions loop so that vectorizer can fall back to the original loop if if-conversion+vectorization isn't beneficial.  Ideally, cost computation in vectorizer should be improved to measure benefit against the original loop, rather than if-converted loop.  So far MAX_PHI_ARG_NUM is set to (4) because cases with more arguments are rare and not likely beneficial.

Apart from above change, the patch also makes changes like: only split critical edge when we have to; cleanups code logic in if_convertible_loop_p about aggressive_if_conv.

Bootstrap and test on x86_64 and AArch64, is it OK?

Thanks,
bin

2016-04-26  Bin Cheng  <bin.cheng@arm.com>

	PR tree-optimization/56541
	* tree-if-conv.c (MAX_PHI_ARG_NUM): New macro.
	(any_complicated_phi): New static variable.
	(aggressive_if_conv): Delete.
	(if_convertible_phi_p): Support PHIs with more than two arguments.
	(if_convertible_bb_p): Remvoe check on aggressive_if_conv and
	critical pred edges.
	(ifcvt_split_critical_edges): Support PHIs with more than two
	arguments by checking new parameter.  Only split critical edges
	if needed.
	(tree_if_conversion): Handle simd pragma marked loop using new
	local variable aggressive_if_conv.  Check any_complicated_phi.

gcc/testsuite/ChangeLog
2016-04-26  Bin Cheng  <bin.cheng@arm.com>

	PR tree-optimization/56541
	* gcc.dg/tree-ssa/ifc-pr56541.c: New test.

[-- Attachment #2: pr56541-20160424.txt --]
[-- Type: text/plain, Size: 8770 bytes --]

diff --git a/gcc/tree-if-conv.c b/gcc/tree-if-conv.c
index 32ced16..31fe390 100644
--- a/gcc/tree-if-conv.c
+++ b/gcc/tree-if-conv.c
@@ -113,11 +113,21 @@ along with GCC; see the file COPYING3.  If not see
 #include "varasm.h"
 #include "builtins.h"
 #include "params.h"
- 
+
+/* Only handle PHIs with no more arguments unless we are asked to by
+   simd pragma.  */
+#define MAX_PHI_ARG_NUM (4)
+
 /* Indicate if new load/store that needs to be predicated is introduced
    during if conversion.  */
 static bool any_pred_load_store;
 
+/* Indicate if there are any complicated PHIs that need to be handled in
+   if-conversion.  Complicated PHI has more than two arguments and can't
+   be degenerated to two arguments PHI.  See more information in comment
+   before phi_convertible_by_degenerating_args.  */
+static bool any_complicated_phi;
+
 /* Hash for struct innermost_loop_behavior.  It depends on the user to
    free the memory.  */
 
@@ -172,9 +182,6 @@ innermost_loop_behavior_hash::equal (const value_type &e1,
 /* List of basic blocks in if-conversion-suitable order.  */
 static basic_block *ifc_bbs;
 
-/* Apply more aggressive (extended) if-conversion if true.  */
-static bool aggressive_if_conv;
-
 /* Hash table to store <DR's innermost loop behavior, DR> pairs.  */
 static hash_map<innermost_loop_behavior_hash,
 		data_reference_p> *innermost_DR_map;
@@ -639,13 +646,9 @@ phi_convertible_by_degenerating_args (gphi *phi)
 }
 
 /* Return true when PHI is if-convertible.  PHI is part of loop LOOP
-   and it belongs to basic block BB.
-
-   PHI is not if-convertible if:
-   - it has more than 2 arguments.
-
-   When the aggressive_if_conv is set, PHI can have more than
-   two arguments.  */
+   and it belongs to basic block BB.  Note at this point, it is sure
+   that PHI is if-convertible.  This function updates global variable
+   ANY_COMPLICATED_PHI if PHI is complicated.  */
 
 static bool
 if_convertible_phi_p (struct loop *loop, basic_block bb, gphi *phi)
@@ -656,17 +659,10 @@ if_convertible_phi_p (struct loop *loop, basic_block bb, gphi *phi)
       print_gimple_stmt (dump_file, phi, 0, TDF_SLIM);
     }
 
-  if (bb != loop->header)
-    {
-      if (gimple_phi_num_args (phi) > 2
-	  && !aggressive_if_conv
-	  && !phi_convertible_by_degenerating_args (phi))
-	{
-	  if (dump_file && (dump_flags & TDF_DETAILS))
-	    fprintf (dump_file, "Phi can't be predicated by single cond.\n");
-	  return false;
-        }
-    }
+  if (bb != loop->header
+      && gimple_phi_num_args (phi) > 2
+      && !phi_convertible_by_degenerating_args (phi))
+    any_complicated_phi = true;
 
   return true;
 }
@@ -1012,8 +1008,6 @@ has_pred_critical_p (basic_block bb)
    - it is after the exit block but before the latch,
    - its edges are not normal.
 
-   Last restriction is valid if aggressive_if_conv is false.
-
    EXIT_BB is the basic block containing the exit of the LOOP.  BB is
    inside LOOP.  */
 
@@ -1062,19 +1056,6 @@ if_convertible_bb_p (struct loop *loop, basic_block bb, basic_block exit_bb)
 	return false;
       }
 
-  /* At least one incoming edge has to be non-critical as otherwise edge
-     predicates are not equal to basic-block predicates of the edge
-     source.  This check is skipped if aggressive_if_conv is true.  */
-  if (!aggressive_if_conv
-      && EDGE_COUNT (bb->preds) > 1
-      && bb != loop->header
-      && all_preds_critical_p (bb))
-    {
-      if (dump_file && (dump_flags & TDF_DETAILS))
-	fprintf (dump_file, "only critical predecessors\n");
-      return false;
-    }
-
   return true;
 }
 
@@ -2380,11 +2361,16 @@ version_loop_for_if_conversion (struct loop *loop)
   return true;
 }
 
-/* Performs splitting of critical edges if aggressive_if_conv is true.
-   Returns false if loop won't be if converted and true otherwise.  */
+/* Performs splitting of critical edges.  Skip splitting and return false
+   if LOOP will not be converted because:
+
+     - LOOP is not well formed.
+     - LOOP has PHI with more than MAX_PHI_ARG_NUM arguments.
+
+   Last restriction is valid only if AGGRESSIVE_IF_CONV is false.  */
 
 static bool
-ifcvt_split_critical_edges (struct loop *loop)
+ifcvt_split_critical_edges (struct loop *loop, bool aggressive_if_conv)
 {
   basic_block *body;
   basic_block bb;
@@ -2393,30 +2379,51 @@ ifcvt_split_critical_edges (struct loop *loop)
   gimple *stmt;
   edge e;
   edge_iterator ei;
+  vec<edge> critical_edges = vNULL;
 
-  if (num <= 2)
-    return false;
-  if (loop->inner)
-    return false;
-  if (!single_exit (loop))
+  /* Loop is not well formed.  */
+  if (num <= 2 || loop->inner || !single_exit (loop))
     return false;
 
   body = get_loop_body (loop);
   for (i = 0; i < num; i++)
     {
       bb = body[i];
-      if (bb == loop->latch
-	  || bb_with_exit_edge_p (loop, bb))
+      if (!aggressive_if_conv
+	  && phi_nodes (bb)
+	  && EDGE_COUNT (bb->preds) > MAX_PHI_ARG_NUM)
+	{
+	  if (dump_file && (dump_flags & TDF_DETAILS))
+	    fprintf (dump_file,
+		     "BB %d has complicated PHI with more than %d args.\n",
+		     bb->index, MAX_PHI_ARG_NUM);
+
+	  free (body);
+	  critical_edges.release ();
+	  return false;
+	}
+      if (bb == loop->latch || bb_with_exit_edge_p (loop, bb))
 	continue;
+
       stmt = last_stmt (bb);
       /* Skip basic blocks not ending with conditional branch.  */
-      if (!(stmt && gimple_code (stmt) == GIMPLE_COND))
+      if (!stmt || gimple_code (stmt) != GIMPLE_COND)
 	continue;
+
       FOR_EACH_EDGE (e, ei, bb->succs)
 	if (EDGE_CRITICAL_P (e) && e->dest->loop_father == loop)
-	  split_edge (e);
+	  critical_edges.safe_push (e);
     }
   free (body);
+
+  while (critical_edges.length () > 0)
+    {
+      e = critical_edges.pop ();
+      /* Don't split if bb can be predicated along non-critical edge.  */
+      if (EDGE_COUNT (e->dest->preds) > 2 || all_preds_critical_p (e->dest))
+	split_edge (e);
+    }
+
   return true;
 }
 
@@ -2713,12 +2720,16 @@ static unsigned int
 tree_if_conversion (struct loop *loop)
 {
   unsigned int todo = 0;
+  bool aggressive_if_conv;
+
   ifc_bbs = NULL;
   any_pred_load_store = false;
+  any_complicated_phi = false;
 
-  /* Set up aggressive if-conversion for loops marked with simd pragma.  */
+  /* Apply more aggressive if-conversion when loop or its outer loop were
+     marked with simd pragma.  When that's the case, we try to if-convert
+     loop containing PHIs with more than MAX_PHI_ARG_NUM arguments.  */
   aggressive_if_conv = loop->force_vectorize;
-  /* Check either outer loop was marked with simd pragma.  */
   if (!aggressive_if_conv)
     {
       struct loop *outer_loop = loop_outer (loop);
@@ -2726,20 +2737,20 @@ tree_if_conversion (struct loop *loop)
 	aggressive_if_conv = true;
     }
 
-  if (aggressive_if_conv)
-    if (!ifcvt_split_critical_edges (loop))
-      goto cleanup;
+  if (!ifcvt_split_critical_edges (loop, aggressive_if_conv))
+    goto cleanup;
 
   if (!if_convertible_loop_p (loop)
       || !dbg_cnt (if_conversion_tree))
     goto cleanup;
 
-  if (any_pred_load_store
+  if ((any_pred_load_store || any_complicated_phi)
       && ((!flag_tree_loop_vectorize && !loop->force_vectorize)
 	  || loop->dont_vectorize))
     goto cleanup;
 
-  if (any_pred_load_store && !version_loop_for_if_conversion (loop))
+  if ((any_pred_load_store || any_complicated_phi)
+      && !version_loop_for_if_conversion (loop))
     goto cleanup;
 
   /* Now all statements are if-convertible.  Combine all the basic
@@ -2749,11 +2760,8 @@ tree_if_conversion (struct loop *loop)
 
   /* Delete dead predicate computations and repair tree correspondent
      to bool pattern to delete multiple uses of predicates.  */
-  if (aggressive_if_conv)
-    {
-      ifcvt_local_dce (loop->header);
-      ifcvt_repair_bool_pattern (loop->header);
-    }
+  ifcvt_local_dce (loop->header);
+  ifcvt_repair_bool_pattern (loop->header);
 
   todo |= TODO_cleanup_cfg;
   mark_virtual_operands_for_renaming (cfun);
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/ifc-pr56541.c b/gcc/testsuite/gcc.dg/tree-ssa/ifc-pr56541.c
new file mode 100644
index 0000000..bd0aa47
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tree-ssa/ifc-pr56541.c
@@ -0,0 +1,25 @@
+/* { dg-do compile } */
+/* { dg-options "-O3 -fdump-tree-ifcvt-stats" { target *-*-* } } */
+
+float a,b,c,d;
+
+float z[1024]; int ok[1024];
+const float rBig = 150.;
+
+void foo()
+{
+  int i;
+
+  for (i=0; i!=1024; ++i)
+    {
+      float rR = a*z[i];
+      float rL = b*z[i];
+      float rMin = (rR<rL) ? rR : rL;
+      float rMax = (rR<rL) ? rL : rR;
+      rMin = (rMax>0) ? rMin : rBig;
+      rMin = (rMin>0) ? rMin : rMax;
+      ok[i] = rMin-c<rMax+d;
+    }
+}
+
+/* { dg-final { scan-tree-dump-times "Applying if-conversion" 1 "ifcvt" } } */

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH GCC]Do more tree if-conversions by handlding PHIs with more than two arguments.
  2016-04-27 15:49 [PATCH GCC]Do more tree if-conversions by handlding PHIs with more than two arguments Bin Cheng
@ 2016-04-28  9:18 ` Richard Biener
  2016-04-29 15:51   ` Bin.Cheng
  0 siblings, 1 reply; 10+ messages in thread
From: Richard Biener @ 2016-04-28  9:18 UTC (permalink / raw)
  To: Bin Cheng; +Cc: gcc-patches, nd

On Wed, Apr 27, 2016 at 5:49 PM, Bin Cheng <Bin.Cheng@arm.com> wrote:
> Hi,
> Currently tree if-conversion only supports PHIs with no more than two arguments unless the loop is marked with "simd pragma".  This patch makes such PHIs supported unconditionally if they have no more than MAX_PHI_ARG_NUM arguments, thus cases like PR56541 can be fixed.  Note because a chain of "?:" operators are needed to compute mult-arg PHI, this patch records the case and versions loop so that vectorizer can fall back to the original loop if if-conversion+vectorization isn't beneficial.  Ideally, cost computation in vectorizer should be improved to measure benefit against the original loop, rather than if-converted loop.  So far MAX_PHI_ARG_NUM is set to (4) because cases with more arguments are rare and not likely beneficial.
>
> Apart from above change, the patch also makes changes like: only split critical edge when we have to; cleanups code logic in if_convertible_loop_p about aggressive_if_conv.
>
> Bootstrap and test on x86_64 and AArch64, is it OK?

Can you make this magic number a --param please?  Otherwise ok.

Thanks,
Richard.

> Thanks,
> bin
>
> 2016-04-26  Bin Cheng  <bin.cheng@arm.com>
>
>         PR tree-optimization/56541
>         * tree-if-conv.c (MAX_PHI_ARG_NUM): New macro.
>         (any_complicated_phi): New static variable.
>         (aggressive_if_conv): Delete.
>         (if_convertible_phi_p): Support PHIs with more than two arguments.
>         (if_convertible_bb_p): Remvoe check on aggressive_if_conv and
>         critical pred edges.
>         (ifcvt_split_critical_edges): Support PHIs with more than two
>         arguments by checking new parameter.  Only split critical edges
>         if needed.
>         (tree_if_conversion): Handle simd pragma marked loop using new
>         local variable aggressive_if_conv.  Check any_complicated_phi.
>
> gcc/testsuite/ChangeLog
> 2016-04-26  Bin Cheng  <bin.cheng@arm.com>
>
>         PR tree-optimization/56541
>         * gcc.dg/tree-ssa/ifc-pr56541.c: New test.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH GCC]Do more tree if-conversions by handlding PHIs with more than two arguments.
  2016-04-28  9:18 ` Richard Biener
@ 2016-04-29 15:51   ` Bin.Cheng
  2016-04-29 15:53     ` Bin.Cheng
  2016-05-02  9:02     ` Richard Biener
  0 siblings, 2 replies; 10+ messages in thread
From: Bin.Cheng @ 2016-04-29 15:51 UTC (permalink / raw)
  To: Richard Biener; +Cc: Bin Cheng, gcc-patches, nd

[-- Attachment #1: Type: text/plain, Size: 1423 bytes --]

On Thu, Apr 28, 2016 at 10:18 AM, Richard Biener
<richard.guenther@gmail.com> wrote:
> On Wed, Apr 27, 2016 at 5:49 PM, Bin Cheng <Bin.Cheng@arm.com> wrote:
>> Hi,
>> Currently tree if-conversion only supports PHIs with no more than two arguments unless the loop is marked with "simd pragma".  This patch makes such PHIs supported unconditionally if they have no more than MAX_PHI_ARG_NUM arguments, thus cases like PR56541 can be fixed.  Note because a chain of "?:" operators are needed to compute mult-arg PHI, this patch records the case and versions loop so that vectorizer can fall back to the original loop if if-conversion+vectorization isn't beneficial.  Ideally, cost computation in vectorizer should be improved to measure benefit against the original loop, rather than if-converted loop.  So far MAX_PHI_ARG_NUM is set to (4) because cases with more arguments are rare and not likely beneficial.
>>
>> Apart from above change, the patch also makes changes like: only split critical edge when we have to; cleanups code logic in if_convertible_loop_p about aggressive_if_conv.
>>
>> Bootstrap and test on x86_64 and AArch64, is it OK?
>
> Can you make this magic number a --param please?  Otherwise ok.
Hi,
Here is the updated patch.  I also added a vectorization test case
since PR56541 was reported against it.
Bootstrap & test on x86_64, is it OK?

Thanks,
bin

>
> Thanks,
> Richard.
>

[-- Attachment #2: pr56541-20160427.txt --]
[-- Type: text/plain, Size: 11048 bytes --]

diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index 67760b5..e26ea96 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -9020,6 +9020,10 @@ Large expressions slow the analyzer.
 Bound on the complexity of the expressions in the scalar evolutions analyzer.
 Complex expressions slow the analyzer.
 
+@item max-tree-if-conversion-phi-args
+Maximum number of arguments in a PHI supported by TREE if conversion
+unless the loop is marked with simd pragma.
+
 @item vect-max-version-for-alignment-checks
 The maximum number of run-time checks that can be performed when
 doing loop versioning for alignment in the vectorizer.
diff --git a/gcc/params.def b/gcc/params.def
index dbff305..754417e 100644
--- a/gcc/params.def
+++ b/gcc/params.def
@@ -530,6 +530,12 @@ DEFPARAM(PARAM_SCEV_MAX_EXPR_COMPLEXITY,
 	 "Bound on the complexity of the expressions in the scalar evolutions analyzer.",
 	 10, 0, 0)
 
+DEFPARAM (PARAM_MAX_TREE_IF_CONVERSION_PHI_ARGS,
+	  "max-tree-if-conversion-phi-args",
+	  "Maximum number of arguments in a PHI supported by TREE if-conversion "
+	  "unless the loop is marked with simd pragma.",
+	  4, 2, 0)
+
 DEFPARAM(PARAM_VECT_MAX_VERSION_FOR_ALIGNMENT_CHECKS,
          "vect-max-version-for-alignment-checks",
          "Bound on number of runtime checks inserted by the vectorizer's loop versioning for alignment check.",
diff --git a/gcc/tree-if-conv.c b/gcc/tree-if-conv.c
index 32ced16..97b62b7 100644
--- a/gcc/tree-if-conv.c
+++ b/gcc/tree-if-conv.c
@@ -113,11 +113,22 @@ along with GCC; see the file COPYING3.  If not see
 #include "varasm.h"
 #include "builtins.h"
 #include "params.h"
- 
+
+/* Only handle PHIs with no more arguments unless we are asked to by
+   simd pragma.  */
+#define MAX_PHI_ARG_NUM \
+  ((unsigned) PARAM_VALUE (PARAM_MAX_TREE_IF_CONVERSION_PHI_ARGS))
+
 /* Indicate if new load/store that needs to be predicated is introduced
    during if conversion.  */
 static bool any_pred_load_store;
 
+/* Indicate if there are any complicated PHIs that need to be handled in
+   if-conversion.  Complicated PHI has more than two arguments and can't
+   be degenerated to two arguments PHI.  See more information in comment
+   before phi_convertible_by_degenerating_args.  */
+static bool any_complicated_phi;
+
 /* Hash for struct innermost_loop_behavior.  It depends on the user to
    free the memory.  */
 
@@ -172,9 +183,6 @@ innermost_loop_behavior_hash::equal (const value_type &e1,
 /* List of basic blocks in if-conversion-suitable order.  */
 static basic_block *ifc_bbs;
 
-/* Apply more aggressive (extended) if-conversion if true.  */
-static bool aggressive_if_conv;
-
 /* Hash table to store <DR's innermost loop behavior, DR> pairs.  */
 static hash_map<innermost_loop_behavior_hash,
 		data_reference_p> *innermost_DR_map;
@@ -639,13 +647,9 @@ phi_convertible_by_degenerating_args (gphi *phi)
 }
 
 /* Return true when PHI is if-convertible.  PHI is part of loop LOOP
-   and it belongs to basic block BB.
-
-   PHI is not if-convertible if:
-   - it has more than 2 arguments.
-
-   When the aggressive_if_conv is set, PHI can have more than
-   two arguments.  */
+   and it belongs to basic block BB.  Note at this point, it is sure
+   that PHI is if-convertible.  This function updates global variable
+   ANY_COMPLICATED_PHI if PHI is complicated.  */
 
 static bool
 if_convertible_phi_p (struct loop *loop, basic_block bb, gphi *phi)
@@ -656,17 +660,10 @@ if_convertible_phi_p (struct loop *loop, basic_block bb, gphi *phi)
       print_gimple_stmt (dump_file, phi, 0, TDF_SLIM);
     }
 
-  if (bb != loop->header)
-    {
-      if (gimple_phi_num_args (phi) > 2
-	  && !aggressive_if_conv
-	  && !phi_convertible_by_degenerating_args (phi))
-	{
-	  if (dump_file && (dump_flags & TDF_DETAILS))
-	    fprintf (dump_file, "Phi can't be predicated by single cond.\n");
-	  return false;
-        }
-    }
+  if (bb != loop->header
+      && gimple_phi_num_args (phi) > 2
+      && !phi_convertible_by_degenerating_args (phi))
+    any_complicated_phi = true;
 
   return true;
 }
@@ -1012,8 +1009,6 @@ has_pred_critical_p (basic_block bb)
    - it is after the exit block but before the latch,
    - its edges are not normal.
 
-   Last restriction is valid if aggressive_if_conv is false.
-
    EXIT_BB is the basic block containing the exit of the LOOP.  BB is
    inside LOOP.  */
 
@@ -1062,19 +1057,6 @@ if_convertible_bb_p (struct loop *loop, basic_block bb, basic_block exit_bb)
 	return false;
       }
 
-  /* At least one incoming edge has to be non-critical as otherwise edge
-     predicates are not equal to basic-block predicates of the edge
-     source.  This check is skipped if aggressive_if_conv is true.  */
-  if (!aggressive_if_conv
-      && EDGE_COUNT (bb->preds) > 1
-      && bb != loop->header
-      && all_preds_critical_p (bb))
-    {
-      if (dump_file && (dump_flags & TDF_DETAILS))
-	fprintf (dump_file, "only critical predecessors\n");
-      return false;
-    }
-
   return true;
 }
 
@@ -2380,11 +2362,16 @@ version_loop_for_if_conversion (struct loop *loop)
   return true;
 }
 
-/* Performs splitting of critical edges if aggressive_if_conv is true.
-   Returns false if loop won't be if converted and true otherwise.  */
+/* Performs splitting of critical edges.  Skip splitting and return false
+   if LOOP will not be converted because:
+
+     - LOOP is not well formed.
+     - LOOP has PHI with more than MAX_PHI_ARG_NUM arguments.
+
+   Last restriction is valid only if AGGRESSIVE_IF_CONV is false.  */
 
 static bool
-ifcvt_split_critical_edges (struct loop *loop)
+ifcvt_split_critical_edges (struct loop *loop, bool aggressive_if_conv)
 {
   basic_block *body;
   basic_block bb;
@@ -2393,30 +2380,51 @@ ifcvt_split_critical_edges (struct loop *loop)
   gimple *stmt;
   edge e;
   edge_iterator ei;
+  vec<edge> critical_edges = vNULL;
 
-  if (num <= 2)
-    return false;
-  if (loop->inner)
-    return false;
-  if (!single_exit (loop))
+  /* Loop is not well formed.  */
+  if (num <= 2 || loop->inner || !single_exit (loop))
     return false;
 
   body = get_loop_body (loop);
   for (i = 0; i < num; i++)
     {
       bb = body[i];
-      if (bb == loop->latch
-	  || bb_with_exit_edge_p (loop, bb))
+      if (!aggressive_if_conv
+	  && phi_nodes (bb)
+	  && EDGE_COUNT (bb->preds) > MAX_PHI_ARG_NUM)
+	{
+	  if (dump_file && (dump_flags & TDF_DETAILS))
+	    fprintf (dump_file,
+		     "BB %d has complicated PHI with more than %u args.\n",
+		     bb->index, MAX_PHI_ARG_NUM);
+
+	  free (body);
+	  critical_edges.release ();
+	  return false;
+	}
+      if (bb == loop->latch || bb_with_exit_edge_p (loop, bb))
 	continue;
+
       stmt = last_stmt (bb);
       /* Skip basic blocks not ending with conditional branch.  */
-      if (!(stmt && gimple_code (stmt) == GIMPLE_COND))
+      if (!stmt || gimple_code (stmt) != GIMPLE_COND)
 	continue;
+
       FOR_EACH_EDGE (e, ei, bb->succs)
 	if (EDGE_CRITICAL_P (e) && e->dest->loop_father == loop)
-	  split_edge (e);
+	  critical_edges.safe_push (e);
     }
   free (body);
+
+  while (critical_edges.length () > 0)
+    {
+      e = critical_edges.pop ();
+      /* Don't split if bb can be predicated along non-critical edge.  */
+      if (EDGE_COUNT (e->dest->preds) > 2 || all_preds_critical_p (e->dest))
+	split_edge (e);
+    }
+
   return true;
 }
 
@@ -2713,12 +2721,16 @@ static unsigned int
 tree_if_conversion (struct loop *loop)
 {
   unsigned int todo = 0;
+  bool aggressive_if_conv;
+
   ifc_bbs = NULL;
   any_pred_load_store = false;
+  any_complicated_phi = false;
 
-  /* Set up aggressive if-conversion for loops marked with simd pragma.  */
+  /* Apply more aggressive if-conversion when loop or its outer loop were
+     marked with simd pragma.  When that's the case, we try to if-convert
+     loop containing PHIs with more than MAX_PHI_ARG_NUM arguments.  */
   aggressive_if_conv = loop->force_vectorize;
-  /* Check either outer loop was marked with simd pragma.  */
   if (!aggressive_if_conv)
     {
       struct loop *outer_loop = loop_outer (loop);
@@ -2726,20 +2738,20 @@ tree_if_conversion (struct loop *loop)
 	aggressive_if_conv = true;
     }
 
-  if (aggressive_if_conv)
-    if (!ifcvt_split_critical_edges (loop))
-      goto cleanup;
+  if (!ifcvt_split_critical_edges (loop, aggressive_if_conv))
+    goto cleanup;
 
   if (!if_convertible_loop_p (loop)
       || !dbg_cnt (if_conversion_tree))
     goto cleanup;
 
-  if (any_pred_load_store
+  if ((any_pred_load_store || any_complicated_phi)
       && ((!flag_tree_loop_vectorize && !loop->force_vectorize)
 	  || loop->dont_vectorize))
     goto cleanup;
 
-  if (any_pred_load_store && !version_loop_for_if_conversion (loop))
+  if ((any_pred_load_store || any_complicated_phi)
+      && !version_loop_for_if_conversion (loop))
     goto cleanup;
 
   /* Now all statements are if-convertible.  Combine all the basic
@@ -2749,11 +2761,8 @@ tree_if_conversion (struct loop *loop)
 
   /* Delete dead predicate computations and repair tree correspondent
      to bool pattern to delete multiple uses of predicates.  */
-  if (aggressive_if_conv)
-    {
-      ifcvt_local_dce (loop->header);
-      ifcvt_repair_bool_pattern (loop->header);
-    }
+  ifcvt_local_dce (loop->header);
+  ifcvt_repair_bool_pattern (loop->header);
 
   todo |= TODO_cleanup_cfg;
   mark_virtual_operands_for_renaming (cfun);
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/ifc-pr56541.c b/gcc/testsuite/gcc.dg/tree-ssa/ifc-pr56541.c
new file mode 100644
index 0000000..bd0aa47
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tree-ssa/ifc-pr56541.c
@@ -0,0 +1,25 @@
+/* { dg-do compile } */
+/* { dg-options "-O3 -fdump-tree-ifcvt-stats" { target *-*-* } } */
+
+float a,b,c,d;
+
+float z[1024]; int ok[1024];
+const float rBig = 150.;
+
+void foo()
+{
+  int i;
+
+  for (i=0; i!=1024; ++i)
+    {
+      float rR = a*z[i];
+      float rL = b*z[i];
+      float rMin = (rR<rL) ? rR : rL;
+      float rMax = (rR<rL) ? rL : rR;
+      rMin = (rMax>0) ? rMin : rBig;
+      rMin = (rMin>0) ? rMin : rMax;
+      ok[i] = rMin-c<rMax+d;
+    }
+}
+
+/* { dg-final { scan-tree-dump-times "Applying if-conversion" 1 "ifcvt" } } */
diff --git a/gcc/testsuite/gcc.dg/vect/pr56541.c b/gcc/testsuite/gcc.dg/vect/pr56541.c
new file mode 100644
index 0000000..16b8d7c
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/vect/pr56541.c
@@ -0,0 +1,27 @@
+/* { dg-do compile } */
+/* { dg-require-effective-target vect_int } */
+/* { dg-require-effective-target vect_float } */
+/* { dg-require-effective-target vect_condition } */
+
+float a,b,c,d;
+
+float z[1024]; int ok[1024];
+const float rBig = 150.;
+
+void foo()
+{
+  int i;
+
+  for (i=0; i!=1024; ++i)
+    {
+      float rR = a*z[i];
+      float rL = b*z[i];
+      float rMin = (rR<rL) ? rR : rL;
+      float rMax = (rR<rL) ? rL : rR;
+      rMin = (rMax>0) ? rMin : rBig;
+      rMin = (rMin>0) ? rMin : rMax;
+      ok[i] = rMin-c<rMax+d;
+    }
+}
+
+/* { dg-final { scan-tree-dump-times "vectorized 1 loops" 1 "vect" } } */

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH GCC]Do more tree if-conversions by handlding PHIs with more than two arguments.
  2016-04-29 15:51   ` Bin.Cheng
@ 2016-04-29 15:53     ` Bin.Cheng
  2016-05-02  9:02     ` Richard Biener
  1 sibling, 0 replies; 10+ messages in thread
From: Bin.Cheng @ 2016-04-29 15:53 UTC (permalink / raw)
  To: Richard Biener; +Cc: gcc-patches

On Fri, Apr 29, 2016 at 4:51 PM, Bin.Cheng <amker.cheng@gmail.com> wrote:
> On Thu, Apr 28, 2016 at 10:18 AM, Richard Biener
> <richard.guenther@gmail.com> wrote:
>> On Wed, Apr 27, 2016 at 5:49 PM, Bin Cheng <Bin.Cheng@arm.com> wrote:
>>> Hi,
>>> Currently tree if-conversion only supports PHIs with no more than two arguments unless the loop is marked with "simd pragma".  This patch makes such PHIs supported unconditionally if they have no more than MAX_PHI_ARG_NUM arguments, thus cases like PR56541 can be fixed.  Note because a chain of "?:" operators are needed to compute mult-arg PHI, this patch records the case and versions loop so that vectorizer can fall back to the original loop if if-conversion+vectorization isn't beneficial.  Ideally, cost computation in vectorizer should be improved to measure benefit against the original loop, rather than if-converted loop.  So far MAX_PHI_ARG_NUM is set to (4) because cases with more arguments are rare and not likely beneficial.
>>>
>>> Apart from above change, the patch also makes changes like: only split critical edge when we have to; cleanups code logic in if_convertible_loop_p about aggressive_if_conv.
>>>
>>> Bootstrap and test on x86_64 and AArch64, is it OK?
>>
>> Can you make this magic number a --param please?  Otherwise ok.
> Hi,
> Here is the updated patch.  I also added a vectorization test case
> since PR56541 was reported against it.
> Bootstrap & test on x86_64, is it OK?
And the new ChangeLog entry.

2016-04-28  bin cheng  <bin.cheng@arm.com>

    PR tree-optimization/56541
    * doc/invoke.texi (@item max-tree-if-conversion-phi-args): New item.
    * params.def (PARAM_MAX_TREE_IF_CONVERSION_PHI_ARGS): new param.
    * tree-if-conv.c (MAX_PHI_ARG_NUM): new macro.
    (any_complicated_phi): new static variable.
    (aggressive_if_conv): delete.
    (if_convertible_phi_p): support phis with more than two arguments.
    (if_convertible_bb_p): remvoe check on aggressive_if_conv and
    critical pred edges.
    (ifcvt_split_critical_edges): support phis with more than two
    arguments by checking new parameter.  only split critical edges
    if needed.
    (tree_if_conversion): handle simd pragma marked loop using new
    local variable aggressive_if_conv.  check any_complicated_phi.

gcc/testsuite/changelog
2016-04-28  bin cheng  <bin.cheng@arm.com>

    PR tree-optimization/56541
    * gcc.dg/tree-ssa/ifc-pr56541.c: new test.
    * gcc.dg/vect/pr56541.c: new test.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH GCC]Do more tree if-conversions by handlding PHIs with more than two arguments.
  2016-04-29 15:51   ` Bin.Cheng
  2016-04-29 15:53     ` Bin.Cheng
@ 2016-05-02  9:02     ` Richard Biener
  2016-05-03  9:07       ` Bin.Cheng
  1 sibling, 1 reply; 10+ messages in thread
From: Richard Biener @ 2016-05-02  9:02 UTC (permalink / raw)
  To: Bin.Cheng; +Cc: Bin Cheng, gcc-patches, nd

On Fri, Apr 29, 2016 at 5:51 PM, Bin.Cheng <amker.cheng@gmail.com> wrote:
> On Thu, Apr 28, 2016 at 10:18 AM, Richard Biener
> <richard.guenther@gmail.com> wrote:
>> On Wed, Apr 27, 2016 at 5:49 PM, Bin Cheng <Bin.Cheng@arm.com> wrote:
>>> Hi,
>>> Currently tree if-conversion only supports PHIs with no more than two arguments unless the loop is marked with "simd pragma".  This patch makes such PHIs supported unconditionally if they have no more than MAX_PHI_ARG_NUM arguments, thus cases like PR56541 can be fixed.  Note because a chain of "?:" operators are needed to compute mult-arg PHI, this patch records the case and versions loop so that vectorizer can fall back to the original loop if if-conversion+vectorization isn't beneficial.  Ideally, cost computation in vectorizer should be improved to measure benefit against the original loop, rather than if-converted loop.  So far MAX_PHI_ARG_NUM is set to (4) because cases with more arguments are rare and not likely beneficial.
>>>
>>> Apart from above change, the patch also makes changes like: only split critical edge when we have to; cleanups code logic in if_convertible_loop_p about aggressive_if_conv.
>>>
>>> Bootstrap and test on x86_64 and AArch64, is it OK?
>>
>> Can you make this magic number a --param please?  Otherwise ok.
> Hi,
> Here is the updated patch.  I also added a vectorization test case
> since PR56541 was reported against it.
> Bootstrap & test on x86_64, is it OK?

+/* { dg-options "-O3 -fdump-tree-ifcvt-stats" { target *-*-* } } */

you can omit { target *-*-* } here.

Ok with that change.

Thanks,
Richard.

> Thanks,
> bin
>
>>
>> Thanks,
>> Richard.
>>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH GCC]Do more tree if-conversions by handlding PHIs with more than two arguments.
  2016-05-02  9:02     ` Richard Biener
@ 2016-05-03  9:07       ` Bin.Cheng
  2016-05-06 12:40         ` [PATCH] Fix memory leak in tree-if-conv.c Martin Liška
  2016-05-10 14:00         ` [PATCH GCC]Do more tree if-conversions by handlding PHIs with more than two arguments Christophe Lyon
  0 siblings, 2 replies; 10+ messages in thread
From: Bin.Cheng @ 2016-05-03  9:07 UTC (permalink / raw)
  To: Richard Biener; +Cc: gcc-patches

On Mon, May 2, 2016 at 10:02 AM, Richard Biener
<richard.guenther@gmail.com> wrote:
> On Fri, Apr 29, 2016 at 5:51 PM, Bin.Cheng <amker.cheng@gmail.com> wrote:
>> On Thu, Apr 28, 2016 at 10:18 AM, Richard Biener
>> <richard.guenther@gmail.com> wrote:
>>> On Wed, Apr 27, 2016 at 5:49 PM, Bin Cheng <Bin.Cheng@arm.com> wrote:
>>>> Hi,
>>>> Currently tree if-conversion only supports PHIs with no more than two arguments unless the loop is marked with "simd pragma".  This patch makes such PHIs supported unconditionally if they have no more than MAX_PHI_ARG_NUM arguments, thus cases like PR56541 can be fixed.  Note because a chain of "?:" operators are needed to compute mult-arg PHI, this patch records the case and versions loop so that vectorizer can fall back to the original loop if if-conversion+vectorization isn't beneficial.  Ideally, cost computation in vectorizer should be improved to measure benefit against the original loop, rather than if-converted loop.  So far MAX_PHI_ARG_NUM is set to (4) because cases with more arguments are rare and not likely beneficial.
>>>>
>>>> Apart from above change, the patch also makes changes like: only split critical edge when we have to; cleanups code logic in if_convertible_loop_p about aggressive_if_conv.
>>>>
>>>> Bootstrap and test on x86_64 and AArch64, is it OK?
>>>
>>> Can you make this magic number a --param please?  Otherwise ok.
>> Hi,
>> Here is the updated patch.  I also added a vectorization test case
>> since PR56541 was reported against it.
>> Bootstrap & test on x86_64, is it OK?
>
> +/* { dg-options "-O3 -fdump-tree-ifcvt-stats" { target *-*-* } } */
>
> you can omit { target *-*-* } here.
>
> Ok with that change.
>
Patch applied as suggested at r235808.

Thanks,
bin

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH] Fix memory leak in tree-if-conv.c
  2016-05-03  9:07       ` Bin.Cheng
@ 2016-05-06 12:40         ` Martin Liška
  2016-05-06 12:55           ` Richard Biener
  2016-05-10 14:00         ` [PATCH GCC]Do more tree if-conversions by handlding PHIs with more than two arguments Christophe Lyon
  1 sibling, 1 reply; 10+ messages in thread
From: Martin Liška @ 2016-05-06 12:40 UTC (permalink / raw)
  To: Bin.Cheng, Richard Biener; +Cc: gcc-patches

[-- Attachment #1: Type: text/plain, Size: 2218 bytes --]

On 05/03/2016 11:07 AM, Bin.Cheng wrote:
> Patch applied as suggested at r235808.
> 
> Thanks,
> bin

Hi.

Following patch introduces memory leak:
/home/marxin/Programming/gcc2/objdir/gcc/xgcc -B/home/marxin/Programming/gcc2/objdir/gcc/    -fno-diagnostics-show-caret -fdiagnostics-color=never    -O3 -fomit-frame-pointer -funroll-loops -fpeel-loops -ftracer -finline-functions  -w -c -o 920928-2.o /home/marxin/Programming/gcc2/gcc/testsuite/gcc.c-torture/compile/920928-2.c

==5714== 40 bytes in 1 blocks are definitely lost in loss record 38 of 905

==5714==    at 0x4C2A00F: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)

==5714==    by 0x109C76F: xrealloc (xmalloc.c:178)

==5714==    by 0xA89696: reserve<edge_def*> (vec.h:288)

==5714==    by 0xA89696: reserve (vec.h:1438)

==5714==    by 0xA89696: safe_push (vec.h:1547)

==5714==    by 0xA89696: ifcvt_split_critical_edges (tree-if-conv.c:2397)

==5714==    by 0xA89696: tree_if_conversion (tree-if-conv.c:2725)

==5714==    by 0xA89696: (anonymous namespace)::pass_if_conversion::execute(function*) (tree-if-conv.c:2832)

==5714==    by 0x98A9D3: execute_one_pass(opt_pass*) (passes.c:2348)

==5714==    by 0x98AF17: execute_pass_list_1(opt_pass*) [clone .constprop.84] (passes.c:2432)

==5714==    by 0x98AF29: execute_pass_list_1(opt_pass*) [clone .constprop.84] (passes.c:2433)

==5714==    by 0x98AF29: execute_pass_list_1(opt_pass*) [clone .constprop.84] (passes.c:2433)

==5714==    by 0x98AF74: execute_pass_list(function*, opt_pass*) (passes.c:2443)

==5714==    by 0x72DEB2: cgraph_node::expand() (cgraphunit.c:1982)

==5714==    by 0x72F3A3: expand_all_functions (cgraphunit.c:2118)

==5714==    by 0x72F3A3: symbol_table::compile() [clone .part.49] (cgraphunit.c:2474)

==5714==    by 0x730D47: compile (cgraphunit.c:2538)

==5714==    by 0x730D47: symbol_table::finalize_compilation_unit() (cgraphunit.c:2564)

==5714==    by 0xA392B7: compile_file() (toplev.c:488)

==5714==    by 0x616117: do_compile (toplev.c:1987)

==5714==    by 0x616117: toplev::main(int, char**) (toplev.c:2095)

==5714==    by 0x6182B6: main (main.c:39)


Following patch fixes that, ready after it bootstraps and survives regtests?

Thanks,
Martin

[-- Attachment #2: 0001-Fix-memory-leak-in-tree-if-conv.c.patch --]
[-- Type: text/x-patch, Size: 1165 bytes --]

From 6ea5e00ce55e8e22fe9429f0cbd942f4938643a6 Mon Sep 17 00:00:00 2001
From: marxin <mliska@suse.cz>
Date: Fri, 6 May 2016 14:20:28 +0200
Subject: [PATCH] Fix memory leak in tree-if-conv.c

gcc/ChangeLog:

2016-05-06  Martin Liska  <mliska@suse.cz>

	* tree-if-conv.c (ifcvt_split_critical_edges): Use auto_vec
	instead of vec as the vector is local to the function.
---
 gcc/tree-if-conv.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/gcc/tree-if-conv.c b/gcc/tree-if-conv.c
index 3d7c613..3ad8e87 100644
--- a/gcc/tree-if-conv.c
+++ b/gcc/tree-if-conv.c
@@ -2361,7 +2361,7 @@ ifcvt_split_critical_edges (struct loop *loop, bool aggressive_if_conv)
   gimple *stmt;
   edge e;
   edge_iterator ei;
-  vec<edge> critical_edges = vNULL;
+  auto_vec<edge> critical_edges;
 
   /* Loop is not well formed.  */
   if (num <= 2 || loop->inner || !single_exit (loop))
@@ -2381,7 +2381,6 @@ ifcvt_split_critical_edges (struct loop *loop, bool aggressive_if_conv)
 		     bb->index, MAX_PHI_ARG_NUM);
 
 	  free (body);
-	  critical_edges.release ();
 	  return false;
 	}
       if (bb == loop->latch || bb_with_exit_edge_p (loop, bb))
-- 
2.8.1


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] Fix memory leak in tree-if-conv.c
  2016-05-06 12:40         ` [PATCH] Fix memory leak in tree-if-conv.c Martin Liška
@ 2016-05-06 12:55           ` Richard Biener
  0 siblings, 0 replies; 10+ messages in thread
From: Richard Biener @ 2016-05-06 12:55 UTC (permalink / raw)
  To: Martin Liška; +Cc: Bin.Cheng, gcc-patches

On Fri, May 6, 2016 at 2:40 PM, Martin Liška <mliska@suse.cz> wrote:
> On 05/03/2016 11:07 AM, Bin.Cheng wrote:
>> Patch applied as suggested at r235808.
>>
>> Thanks,
>> bin
>
> Hi.
>
> Following patch introduces memory leak:
> /home/marxin/Programming/gcc2/objdir/gcc/xgcc -B/home/marxin/Programming/gcc2/objdir/gcc/    -fno-diagnostics-show-caret -fdiagnostics-color=never    -O3 -fomit-frame-pointer -funroll-loops -fpeel-loops -ftracer -finline-functions  -w -c -o 920928-2.o /home/marxin/Programming/gcc2/gcc/testsuite/gcc.c-torture/compile/920928-2.c
>
> ==5714== 40 bytes in 1 blocks are definitely lost in loss record 38 of 905
>
> ==5714==    at 0x4C2A00F: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
>
> ==5714==    by 0x109C76F: xrealloc (xmalloc.c:178)
>
> ==5714==    by 0xA89696: reserve<edge_def*> (vec.h:288)
>
> ==5714==    by 0xA89696: reserve (vec.h:1438)
>
> ==5714==    by 0xA89696: safe_push (vec.h:1547)
>
> ==5714==    by 0xA89696: ifcvt_split_critical_edges (tree-if-conv.c:2397)
>
> ==5714==    by 0xA89696: tree_if_conversion (tree-if-conv.c:2725)
>
> ==5714==    by 0xA89696: (anonymous namespace)::pass_if_conversion::execute(function*) (tree-if-conv.c:2832)
>
> ==5714==    by 0x98A9D3: execute_one_pass(opt_pass*) (passes.c:2348)
>
> ==5714==    by 0x98AF17: execute_pass_list_1(opt_pass*) [clone .constprop.84] (passes.c:2432)
>
> ==5714==    by 0x98AF29: execute_pass_list_1(opt_pass*) [clone .constprop.84] (passes.c:2433)
>
> ==5714==    by 0x98AF29: execute_pass_list_1(opt_pass*) [clone .constprop.84] (passes.c:2433)
>
> ==5714==    by 0x98AF74: execute_pass_list(function*, opt_pass*) (passes.c:2443)
>
> ==5714==    by 0x72DEB2: cgraph_node::expand() (cgraphunit.c:1982)
>
> ==5714==    by 0x72F3A3: expand_all_functions (cgraphunit.c:2118)
>
> ==5714==    by 0x72F3A3: symbol_table::compile() [clone .part.49] (cgraphunit.c:2474)
>
> ==5714==    by 0x730D47: compile (cgraphunit.c:2538)
>
> ==5714==    by 0x730D47: symbol_table::finalize_compilation_unit() (cgraphunit.c:2564)
>
> ==5714==    by 0xA392B7: compile_file() (toplev.c:488)
>
> ==5714==    by 0x616117: do_compile (toplev.c:1987)
>
> ==5714==    by 0x616117: toplev::main(int, char**) (toplev.c:2095)
>
> ==5714==    by 0x6182B6: main (main.c:39)
>
>
> Following patch fixes that, ready after it bootstraps and survives regtests?

Ok.

Richard.

> Thanks,
> Martin

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH GCC]Do more tree if-conversions by handlding PHIs with more than two arguments.
  2016-05-03  9:07       ` Bin.Cheng
  2016-05-06 12:40         ` [PATCH] Fix memory leak in tree-if-conv.c Martin Liška
@ 2016-05-10 14:00         ` Christophe Lyon
  2016-05-10 14:03           ` Bin.Cheng
  1 sibling, 1 reply; 10+ messages in thread
From: Christophe Lyon @ 2016-05-10 14:00 UTC (permalink / raw)
  To: Bin.Cheng; +Cc: Richard Biener, gcc-patches

On 3 May 2016 at 11:07, Bin.Cheng <amker.cheng@gmail.com> wrote:
> On Mon, May 2, 2016 at 10:02 AM, Richard Biener
> <richard.guenther@gmail.com> wrote:
>> On Fri, Apr 29, 2016 at 5:51 PM, Bin.Cheng <amker.cheng@gmail.com> wrote:
>>> On Thu, Apr 28, 2016 at 10:18 AM, Richard Biener
>>> <richard.guenther@gmail.com> wrote:
>>>> On Wed, Apr 27, 2016 at 5:49 PM, Bin Cheng <Bin.Cheng@arm.com> wrote:
>>>>> Hi,
>>>>> Currently tree if-conversion only supports PHIs with no more than two arguments unless the loop is marked with "simd pragma".  This patch makes such PHIs supported unconditionally if they have no more than MAX_PHI_ARG_NUM arguments, thus cases like PR56541 can be fixed.  Note because a chain of "?:" operators are needed to compute mult-arg PHI, this patch records the case and versions loop so that vectorizer can fall back to the original loop if if-conversion+vectorization isn't beneficial.  Ideally, cost computation in vectorizer should be improved to measure benefit against the original loop, rather than if-converted loop.  So far MAX_PHI_ARG_NUM is set to (4) because cases with more arguments are rare and not likely beneficial.
>>>>>
>>>>> Apart from above change, the patch also makes changes like: only split critical edge when we have to; cleanups code logic in if_convertible_loop_p about aggressive_if_conv.
>>>>>
>>>>> Bootstrap and test on x86_64 and AArch64, is it OK?
>>>>
>>>> Can you make this magic number a --param please?  Otherwise ok.
>>> Hi,
>>> Here is the updated patch.  I also added a vectorization test case
>>> since PR56541 was reported against it.
>>> Bootstrap & test on x86_64, is it OK?
>>
>> +/* { dg-options "-O3 -fdump-tree-ifcvt-stats" { target *-*-* } } */
>>
>> you can omit { target *-*-* } here.
>>
>> Ok with that change.
>>
> Patch applied as suggested at r235808.
>

Hi Bin,

I'm seeing:
FAIL: gcc.dg/vect/pr56541.c -flto -ffat-lto-objects
scan-tree-dump-times vect "vectorized 1 loops" 1
FAIL: gcc.dg/vect/pr56541.c scan-tree-dump-times vect "vectorized 1 loops" 1

on arm and aarch64 targets.

Didn't you see this in your aarch64 bootstrap?

Christophe.


> Thanks,
> bin

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH GCC]Do more tree if-conversions by handlding PHIs with more than two arguments.
  2016-05-10 14:00         ` [PATCH GCC]Do more tree if-conversions by handlding PHIs with more than two arguments Christophe Lyon
@ 2016-05-10 14:03           ` Bin.Cheng
  0 siblings, 0 replies; 10+ messages in thread
From: Bin.Cheng @ 2016-05-10 14:03 UTC (permalink / raw)
  To: Christophe Lyon; +Cc: Richard Biener, gcc-patches

On Tue, May 10, 2016 at 3:00 PM, Christophe Lyon
<christophe.lyon@linaro.org> wrote:
> On 3 May 2016 at 11:07, Bin.Cheng <amker.cheng@gmail.com> wrote:
>> On Mon, May 2, 2016 at 10:02 AM, Richard Biener
>> <richard.guenther@gmail.com> wrote:
>>> On Fri, Apr 29, 2016 at 5:51 PM, Bin.Cheng <amker.cheng@gmail.com> wrote:
>>>> On Thu, Apr 28, 2016 at 10:18 AM, Richard Biener
>>>> <richard.guenther@gmail.com> wrote:
>>>>> On Wed, Apr 27, 2016 at 5:49 PM, Bin Cheng <Bin.Cheng@arm.com> wrote:
>>>>>> Hi,
>>>>>> Currently tree if-conversion only supports PHIs with no more than two arguments unless the loop is marked with "simd pragma".  This patch makes such PHIs supported unconditionally if they have no more than MAX_PHI_ARG_NUM arguments, thus cases like PR56541 can be fixed.  Note because a chain of "?:" operators are needed to compute mult-arg PHI, this patch records the case and versions loop so that vectorizer can fall back to the original loop if if-conversion+vectorization isn't beneficial.  Ideally, cost computation in vectorizer should be improved to measure benefit against the original loop, rather than if-converted loop.  So far MAX_PHI_ARG_NUM is set to (4) because cases with more arguments are rare and not likely beneficial.
>>>>>>
>>>>>> Apart from above change, the patch also makes changes like: only split critical edge when we have to; cleanups code logic in if_convertible_loop_p about aggressive_if_conv.
>>>>>>
>>>>>> Bootstrap and test on x86_64 and AArch64, is it OK?
>>>>>
>>>>> Can you make this magic number a --param please?  Otherwise ok.
>>>> Hi,
>>>> Here is the updated patch.  I also added a vectorization test case
>>>> since PR56541 was reported against it.
>>>> Bootstrap & test on x86_64, is it OK?
>>>
>>> +/* { dg-options "-O3 -fdump-tree-ifcvt-stats" { target *-*-* } } */
>>>
>>> you can omit { target *-*-* } here.
>>>
>>> Ok with that change.
>>>
>> Patch applied as suggested at r235808.
>>
>
> Hi Bin,
>
> I'm seeing:
> FAIL: gcc.dg/vect/pr56541.c -flto -ffat-lto-objects
> scan-tree-dump-times vect "vectorized 1 loops" 1
> FAIL: gcc.dg/vect/pr56541.c scan-tree-dump-times vect "vectorized 1 loops" 1
Hi,
Thanks for reporting this, I remembered it passes on AArch64 when the
patch is tested.  I will look into it.

Thanks,
bin
>
> on arm and aarch64 targets.
>
> Didn't you see this in your aarch64 bootstrap?
>
> Christophe.
>
>
>> Thanks,
>> bin

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2016-05-10 14:03 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-04-27 15:49 [PATCH GCC]Do more tree if-conversions by handlding PHIs with more than two arguments Bin Cheng
2016-04-28  9:18 ` Richard Biener
2016-04-29 15:51   ` Bin.Cheng
2016-04-29 15:53     ` Bin.Cheng
2016-05-02  9:02     ` Richard Biener
2016-05-03  9:07       ` Bin.Cheng
2016-05-06 12:40         ` [PATCH] Fix memory leak in tree-if-conv.c Martin Liška
2016-05-06 12:55           ` Richard Biener
2016-05-10 14:00         ` [PATCH GCC]Do more tree if-conversions by handlding PHIs with more than two arguments Christophe Lyon
2016-05-10 14:03           ` Bin.Cheng

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).