[PATCH 1/3] PHIOPT: Add diamond bb form to factor_out_conditional_conversion

[PATCH 1/3] PHIOPT: Add diamond bb form to factor_out_conditional_conversion

[Andrew Pinski]

So the function factor_out_conditional_conversion already supports
diamond shaped bb forms, just need to be called for such a thing.

harden-cond-comp.c needed to be changed as we would optimize out the
conversion now and that causes the compare hardening not needing to
split the block which it was testing. So change it such that there
would be no chance of optimization.

Also add two testcases that showed the improvement. PR 103771 is
solved in ifconvert also for the vectorizer but now it is solved
in a general sense.

OK? Bootstrapped and tested on x86_64-linux-gnu with no regressions.

	PR tree-optimization/49959
	PR tree-optimization/103771

gcc/ChangeLog:

	* tree-ssa-phiopt.cc (pass_phiopt::execute): Support
	Diamond shapped bb form for factor_out_conditional_conversion.

gcc/testsuite/ChangeLog:

	* c-c++-common/torture/harden-cond-comp.c: Change testcase
	slightly to avoid the new phiopt optimization.
	* gcc.dg/tree-ssa/abs-2.c: New test.
	* gcc.dg/tree-ssa/pr103771.c: New test.
---
 .../c-c++-common/torture/harden-cond-comp.c   |  6 +++---
 gcc/testsuite/gcc.dg/tree-ssa/abs-2.c         | 20 +++++++++++++++++++
 gcc/testsuite/gcc.dg/tree-ssa/pr103771.c      | 18 +++++++++++++++++
 gcc/tree-ssa-phiopt.cc                        |  2 +-
 4 files changed, 42 insertions(+), 4 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/abs-2.c
 create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/pr103771.c

diff --git a/gcc/testsuite/c-c++-common/torture/harden-cond-comp.c b/gcc/testsuite/c-c++-common/torture/harden-cond-comp.c
index 5aad890a1d3..dcf364ee993 100644
--- a/gcc/testsuite/c-c++-common/torture/harden-cond-comp.c
+++ b/gcc/testsuite/c-c++-common/torture/harden-cond-comp.c
@@ -1,11 +1,11 @@
 /* { dg-do compile } */
 /* { dg-options "-fharden-conditional-branches -fharden-compares -fdump-tree-hardcbr -fdump-tree-hardcmp -ffat-lto-objects" } */
 
-int f(int i, int j) {
+int f(int i, int j, int k, int l) {
   if (i == 0)
-    return j != 0;
+    return (j != 0) + l;
   else
-    return i * j != 0;
+    return (i * j != 0) * k;
 }
 
 /* { dg-final { scan-tree-dump-times "Splitting edge" 2 "hardcbr" } } */
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/abs-2.c b/gcc/testsuite/gcc.dg/tree-ssa/abs-2.c
new file mode 100644
index 00000000000..328b1802541
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tree-ssa/abs-2.c
@@ -0,0 +1,20 @@
+/* PR tree-optimization/49959 */
+/* { dg-do compile } */
+/* { dg-options "-O -fdump-tree-phiopt1-details" } */
+
+#define ABS(X)    (((X)>0)?(X):-(X))
+unsigned long
+test_abs(int *cur)
+{
+  unsigned long sad = 0;
+  if (cur[0] > 0)
+    sad = cur[0];
+  else
+    sad = -cur[0];
+  return sad;
+}
+
+/* We should figure out that test_abs has an ABS_EXPR in it. */
+/* { dg-final { scan-tree-dump " = ABS_EXPR" "phiopt1"} } */
+/* { dg-final { scan-tree-dump-times "changed to factor conversion out from" 1 "phiopt1"} } */
+
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr103771.c b/gcc/testsuite/gcc.dg/tree-ssa/pr103771.c
new file mode 100644
index 00000000000..97c9db846cb
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tree-ssa/pr103771.c
@@ -0,0 +1,18 @@
+/* { dg-do compile } */
+/* { dg-options "-O3 -fdump-tree-phiopt1-details" } */
+/* { dg-final { scan-tree-dump-times "changed to factor conversion out from COND_EXPR." 1 "phiopt1" } } */
+
+typedef unsigned char uint8_t;
+
+static uint8_t x264_clip_uint8 (int x)
+{
+  return x & (~255) ? (-x) >> 31 : x;
+}
+
+void
+mc_weight (uint8_t* __restrict dst, uint8_t* __restrict src,
+	   int i_width,int i_scale)
+{
+  for(int x = 0; x < i_width; x++)
+    dst[x] = x264_clip_uint8 (src[x] * i_scale);
+}
diff --git a/gcc/tree-ssa-phiopt.cc b/gcc/tree-ssa-phiopt.cc
index f14b7e8b7e6..41fea78dc8d 100644
--- a/gcc/tree-ssa-phiopt.cc
+++ b/gcc/tree-ssa-phiopt.cc
@@ -4087,7 +4087,7 @@ pass_phiopt::execute (function *)
 
       gphi *newphi;
       if (single_pred_p (bb1)
-	  && !diamond_p
+	  && EDGE_COUNT (merge->preds) == 2
 	  && (newphi = factor_out_conditional_conversion (e1, e2, phi,
 							  arg0, arg1,
 							  cond_stmt)))
-- 
2.31.1

[PATCH 2/3] PHIOPT: Loop over calling factor_out_conditional_conversion

[Andrew Pinski]

After adding diamond shaped bb support to factor_out_conditional_conversion,
we can get a case where we have two conversions that needs factored out
and then would have another phiopt happen.
An example is:
```
static inline unsigned long long g(int t)
{
  unsigned t1 = t;
  return t1;
}
unsigned long long f(int c, int d, int e)
{
  unsigned long long t;
  if (c > d)
    t = g(c);
  else
    t = g(d);
  return t;
}
```
In this case we should get a MAX_EXPR in phiopt1 with two casts.
Before this patch, we would just factor out the outer cast and then
wait till phiopt2 to factor out the inner cast.

OK? Bootstrapped and tested on x86_64-linux-gnu with no regressions.

gcc/ChangeLog:

	* tree-ssa-phiopt.cc (pass_phiopt::execute): Loop
	over factor_out_conditional_conversion.

gcc/testsuite/ChangeLog:

	* gcc.dg/tree-ssa/minmax-17.c: New test.
---
 gcc/testsuite/gcc.dg/tree-ssa/minmax-17.c | 21 ++++++++++++++++++
 gcc/tree-ssa-phiopt.cc                    | 27 +++++++++++++----------
 2 files changed, 36 insertions(+), 12 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/minmax-17.c

diff --git a/gcc/testsuite/gcc.dg/tree-ssa/minmax-17.c b/gcc/testsuite/gcc.dg/tree-ssa/minmax-17.c
new file mode 100644
index 00000000000..bd737e6b4cb
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tree-ssa/minmax-17.c
@@ -0,0 +1,21 @@
+/* { dg-do compile } */
+/* { dg-options "-O -fdump-tree-phiopt1-details" } */
+
+static inline unsigned long long g(int t)
+{
+  unsigned t1 = t;
+  return t1;
+}
+unsigned long long test_max(int c, int d, int e)
+{
+  unsigned long long t;
+  if (c > d)
+    t = g(c);
+  else
+    t = g(d);
+  return t;
+}
+
+/* We should figure out that test_max has an MAX_EXPR in it. */
+/* { dg-final { scan-tree-dump " = MAX_EXPR" "phiopt1"} } */
+/* { dg-final { scan-tree-dump-times "changed to factor conversion out from" 2 "phiopt1"} } */
diff --git a/gcc/tree-ssa-phiopt.cc b/gcc/tree-ssa-phiopt.cc
index 41fea78dc8d..7fe088b13ff 100644
--- a/gcc/tree-ssa-phiopt.cc
+++ b/gcc/tree-ssa-phiopt.cc
@@ -4085,20 +4085,23 @@ pass_phiopt::execute (function *)
 	  node.  */
       gcc_assert (arg0 != NULL_TREE && arg1 != NULL_TREE);
 
-      gphi *newphi;
       if (single_pred_p (bb1)
-	  && EDGE_COUNT (merge->preds) == 2
-	  && (newphi = factor_out_conditional_conversion (e1, e2, phi,
-							  arg0, arg1,
-							  cond_stmt)))
+	  && EDGE_COUNT (merge->preds) == 2)
 	{
-	  phi = newphi;
-	  /* factor_out_conditional_conversion may create a new PHI in
-	     BB2 and eliminate an existing PHI in BB2.  Recompute values
-	     that may be affected by that change.  */
-	  arg0 = gimple_phi_arg_def (phi, e1->dest_idx);
-	  arg1 = gimple_phi_arg_def (phi, e2->dest_idx);
-	  gcc_assert (arg0 != NULL_TREE && arg1 != NULL_TREE);
+	  gphi *newphi = phi;
+	  while (newphi)
+	    {
+	      phi = newphi;
+	      /* factor_out_conditional_conversion may create a new PHI in
+		 BB2 and eliminate an existing PHI in BB2.  Recompute values
+		 that may be affected by that change.  */
+	      arg0 = gimple_phi_arg_def (phi, e1->dest_idx);
+	      arg1 = gimple_phi_arg_def (phi, e2->dest_idx);
+	      gcc_assert (arg0 != NULL_TREE && arg1 != NULL_TREE);
+	      newphi = factor_out_conditional_conversion (e1, e2, phi,
+							  arg0, arg1,
+							  cond_stmt);
+	    }
 	}
 
       /* Do the replacement of conditional if it can be done.  */
-- 
2.31.1

Re: [PATCH 2/3] PHIOPT: Loop over calling factor_out_conditional_conversion

[Richard Biener]

On Sun, May 7, 2023 at 6:45 AM Andrew Pinski via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> After adding diamond shaped bb support to factor_out_conditional_conversion,
> we can get a case where we have two conversions that needs factored out
> and then would have another phiopt happen.
> An example is:
> ```
> static inline unsigned long long g(int t)
> {
>   unsigned t1 = t;
>   return t1;
> }
> unsigned long long f(int c, int d, int e)
> {
>   unsigned long long t;
>   if (c > d)
>     t = g(c);
>   else
>     t = g(d);
>   return t;
> }
> ```
> In this case we should get a MAX_EXPR in phiopt1 with two casts.
> Before this patch, we would just factor out the outer cast and then
> wait till phiopt2 to factor out the inner cast.
>
> OK? Bootstrapped and tested on x86_64-linux-gnu with no regressions.

OK.

Thanks,
Richard.

> gcc/ChangeLog:
>
>         * tree-ssa-phiopt.cc (pass_phiopt::execute): Loop
>         over factor_out_conditional_conversion.
>
> gcc/testsuite/ChangeLog:
>
>         * gcc.dg/tree-ssa/minmax-17.c: New test.
> ---
>  gcc/testsuite/gcc.dg/tree-ssa/minmax-17.c | 21 ++++++++++++++++++
>  gcc/tree-ssa-phiopt.cc                    | 27 +++++++++++++----------
>  2 files changed, 36 insertions(+), 12 deletions(-)
>  create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/minmax-17.c
>
> diff --git a/gcc/testsuite/gcc.dg/tree-ssa/minmax-17.c b/gcc/testsuite/gcc.dg/tree-ssa/minmax-17.c
> new file mode 100644
> index 00000000000..bd737e6b4cb
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/tree-ssa/minmax-17.c
> @@ -0,0 +1,21 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O -fdump-tree-phiopt1-details" } */
> +
> +static inline unsigned long long g(int t)
> +{
> +  unsigned t1 = t;
> +  return t1;
> +}
> +unsigned long long test_max(int c, int d, int e)
> +{
> +  unsigned long long t;
> +  if (c > d)
> +    t = g(c);
> +  else
> +    t = g(d);
> +  return t;
> +}
> +
> +/* We should figure out that test_max has an MAX_EXPR in it. */
> +/* { dg-final { scan-tree-dump " = MAX_EXPR" "phiopt1"} } */
> +/* { dg-final { scan-tree-dump-times "changed to factor conversion out from" 2 "phiopt1"} } */
> diff --git a/gcc/tree-ssa-phiopt.cc b/gcc/tree-ssa-phiopt.cc
> index 41fea78dc8d..7fe088b13ff 100644
> --- a/gcc/tree-ssa-phiopt.cc
> +++ b/gcc/tree-ssa-phiopt.cc
> @@ -4085,20 +4085,23 @@ pass_phiopt::execute (function *)
>           node.  */
>        gcc_assert (arg0 != NULL_TREE && arg1 != NULL_TREE);
>
> -      gphi *newphi;
>        if (single_pred_p (bb1)
> -         && EDGE_COUNT (merge->preds) == 2
> -         && (newphi = factor_out_conditional_conversion (e1, e2, phi,
> -                                                         arg0, arg1,
> -                                                         cond_stmt)))
> +         && EDGE_COUNT (merge->preds) == 2)
>         {
> -         phi = newphi;
> -         /* factor_out_conditional_conversion may create a new PHI in
> -            BB2 and eliminate an existing PHI in BB2.  Recompute values
> -            that may be affected by that change.  */
> -         arg0 = gimple_phi_arg_def (phi, e1->dest_idx);
> -         arg1 = gimple_phi_arg_def (phi, e2->dest_idx);
> -         gcc_assert (arg0 != NULL_TREE && arg1 != NULL_TREE);
> +         gphi *newphi = phi;
> +         while (newphi)
> +           {
> +             phi = newphi;
> +             /* factor_out_conditional_conversion may create a new PHI in
> +                BB2 and eliminate an existing PHI in BB2.  Recompute values
> +                that may be affected by that change.  */
> +             arg0 = gimple_phi_arg_def (phi, e1->dest_idx);
> +             arg1 = gimple_phi_arg_def (phi, e2->dest_idx);
> +             gcc_assert (arg0 != NULL_TREE && arg1 != NULL_TREE);
> +             newphi = factor_out_conditional_conversion (e1, e2, phi,
> +                                                         arg0, arg1,
> +                                                         cond_stmt);
> +           }
>         }
>
>        /* Do the replacement of conditional if it can be done.  */
> --
> 2.31.1
>

[PATCH 3/3] PHIOPT: factor out unary operations instead of just conversions

[Andrew Pinski]

After using factor_out_conditional_conversion with diamond bb,
we should be able do use it also for all normal unary gimple and not
just conversions. This allows to optimize PR 59424 for an example.
This is also a start to optimize PR 64700 and a few others.

OK? Bootstrapped and tested on x86_64-linux-gnu with no regressions.

An example of this is:
```
static inline unsigned long long g(int t)
{
  unsigned t1 = t;
  return t1;
}
static int abs1(int a)
{
  if (a < 0)
    a = -a;
  return a;
}
unsigned long long f(int c, int d, int e)
{
  unsigned long long t;
  if (d > e)
    t = g(abs1(d));
  else
    t = g(abs1(e));
  return t;
}
```

Which should be optimized to:
  _9 = MAX_EXPR <d_5(D), e_6(D)>;
  _4 = ABS_EXPR <_9>;
  t_3 = (long long unsigned intD.16) _4;

gcc/ChangeLog:

	* tree-ssa-phiopt.cc (factor_out_conditional_conversion): Rename to ...
	(factor_out_conditional_operation): This and add support for all unary
	operations.
	(pass_phiopt::execute): Update call to factor_out_conditional_conversion
	to call factor_out_conditional_operation instead.

	PR tree-optimization/109424
	PR tree-optimization/59424

gcc/testsuite/ChangeLog:

	* gcc.dg/tree-ssa/abs-2.c: Update tree scan for
	details change in wording.
	* gcc.dg/tree-ssa/minmax-17.c: Likewise.
	* gcc.dg/tree-ssa/pr103771.c: Likewise.
	* gcc.dg/tree-ssa/minmax-18.c: New test.
	* gcc.dg/tree-ssa/minmax-19.c: New test.
---
 gcc/testsuite/gcc.dg/tree-ssa/abs-2.c     |  2 +-
 gcc/testsuite/gcc.dg/tree-ssa/minmax-17.c |  2 +-
 gcc/testsuite/gcc.dg/tree-ssa/minmax-18.c | 27 +++++++++++
 gcc/testsuite/gcc.dg/tree-ssa/minmax-19.c | 10 ++++
 gcc/testsuite/gcc.dg/tree-ssa/pr103771.c  |  2 +-
 gcc/tree-ssa-phiopt.cc                    | 56 +++++++++++++----------
 6 files changed, 71 insertions(+), 28 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/minmax-18.c
 create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/minmax-19.c

diff --git a/gcc/testsuite/gcc.dg/tree-ssa/abs-2.c b/gcc/testsuite/gcc.dg/tree-ssa/abs-2.c
index 328b1802541..f8bbeb43237 100644
--- a/gcc/testsuite/gcc.dg/tree-ssa/abs-2.c
+++ b/gcc/testsuite/gcc.dg/tree-ssa/abs-2.c
@@ -16,5 +16,5 @@ test_abs(int *cur)
 
 /* We should figure out that test_abs has an ABS_EXPR in it. */
 /* { dg-final { scan-tree-dump " = ABS_EXPR" "phiopt1"} } */
-/* { dg-final { scan-tree-dump-times "changed to factor conversion out from" 1 "phiopt1"} } */
+/* { dg-final { scan-tree-dump-times "changed to factor operation out from" 1 "phiopt1"} } */
 
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/minmax-17.c b/gcc/testsuite/gcc.dg/tree-ssa/minmax-17.c
index bd737e6b4cb..7c76cfc62a9 100644
--- a/gcc/testsuite/gcc.dg/tree-ssa/minmax-17.c
+++ b/gcc/testsuite/gcc.dg/tree-ssa/minmax-17.c
@@ -18,4 +18,4 @@ unsigned long long test_max(int c, int d, int e)
 
 /* We should figure out that test_max has an MAX_EXPR in it. */
 /* { dg-final { scan-tree-dump " = MAX_EXPR" "phiopt1"} } */
-/* { dg-final { scan-tree-dump-times "changed to factor conversion out from" 2 "phiopt1"} } */
+/* { dg-final { scan-tree-dump-times "changed to factor operation out from" 2 "phiopt1"} } */
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/minmax-18.c b/gcc/testsuite/gcc.dg/tree-ssa/minmax-18.c
new file mode 100644
index 00000000000..c8e1670f64a
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tree-ssa/minmax-18.c
@@ -0,0 +1,27 @@
+/* { dg-do compile } */
+/* { dg-options "-O -fdump-tree-phiopt1-details" } */
+
+static inline unsigned long long g(int t)
+{
+  unsigned t1 = t;
+  return t1;
+}
+static inline int abs1(int a)
+{
+  if (a < 0)
+    a = -a;
+  return a;
+}
+unsigned long long f(int c, int d, int e)
+{
+  unsigned long long t;
+  if (d > e)
+    t = g(abs1(d));
+  else
+    t = g(abs1(e));
+  return t;
+}
+
+/* { dg-final { scan-tree-dump " = MAX_EXPR" "phiopt1"} } */
+/* { dg-final { scan-tree-dump-times " = ABS_EXPR" 2 "phiopt1"} } */
+/* { dg-final { scan-tree-dump-times "changed to factor operation out from" 3 "phiopt1"} } */
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/minmax-19.c b/gcc/testsuite/gcc.dg/tree-ssa/minmax-19.c
new file mode 100644
index 00000000000..5ed55fe2e23
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tree-ssa/minmax-19.c
@@ -0,0 +1,10 @@
+/* PR tree-optimization/109424 */
+/* { dg-do compile } */
+/* { dg-options "-O -fdump-tree-phiopt1-details" } */
+
+int f2(int x, int y)
+{
+    return (x > y) ? ~x : ~y;
+}
+/* { dg-final { scan-tree-dump " = MAX_EXPR" "phiopt1"} } */
+/* { dg-final { scan-tree-dump-times "changed to factor operation out from" 1 "phiopt1"} } */
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr103771.c b/gcc/testsuite/gcc.dg/tree-ssa/pr103771.c
index 97c9db846cb..8faa45a8222 100644
--- a/gcc/testsuite/gcc.dg/tree-ssa/pr103771.c
+++ b/gcc/testsuite/gcc.dg/tree-ssa/pr103771.c
@@ -1,6 +1,6 @@
 /* { dg-do compile } */
 /* { dg-options "-O3 -fdump-tree-phiopt1-details" } */
-/* { dg-final { scan-tree-dump-times "changed to factor conversion out from COND_EXPR." 1 "phiopt1" } } */
+/* { dg-final { scan-tree-dump-times "changed to factor operation out from COND_EXPR." 1 "phiopt1" } } */
 
 typedef unsigned char uint8_t;
 
diff --git a/gcc/tree-ssa-phiopt.cc b/gcc/tree-ssa-phiopt.cc
index 7fe088b13ff..2fb28b4e60e 100644
--- a/gcc/tree-ssa-phiopt.cc
+++ b/gcc/tree-ssa-phiopt.cc
@@ -209,13 +209,13 @@ replace_phi_edge_with_variable (basic_block cond_block,
 	      bb->index);
 }
 
-/* PR66726: Factor conversion out of COND_EXPR.  If the arguments of the PHI
+/* PR66726: Factor operations out of COND_EXPR.  If the arguments of the PHI
    stmt are CONVERT_STMT, factor out the conversion and perform the conversion
    to the result of PHI stmt.  COND_STMT is the controlling predicate.
    Return the newly-created PHI, if any.  */
 
 static gphi *
-factor_out_conditional_conversion (edge e0, edge e1, gphi *phi,
+factor_out_conditional_operation (edge e0, edge e1, gphi *phi,
 				   tree arg0, tree arg1, gimple *cond_stmt)
 {
   gimple *arg0_def_stmt = NULL, *arg1_def_stmt = NULL, *new_stmt;
@@ -224,7 +224,7 @@ factor_out_conditional_conversion (edge e0, edge e1, gphi *phi,
   gphi *newphi;
   gimple_stmt_iterator gsi, gsi_for_def;
   location_t locus = gimple_location (phi);
-  enum tree_code convert_code;
+  enum tree_code op_code;
 
   /* Handle only PHI statements with two arguments.  TODO: If all
      other arguments to PHI are INTEGER_CST or if their defining
@@ -246,15 +246,17 @@ factor_out_conditional_conversion (edge e0, edge e1, gphi *phi,
     return NULL;
 
   /* Check if arg0 is an SSA_NAME and the stmt which defines arg0 is
-     a conversion.  */
+     an unary operation.  */
   arg0_def_stmt = SSA_NAME_DEF_STMT (arg0);
-  if (!gimple_assign_cast_p (arg0_def_stmt))
+  if (!is_gimple_assign (arg0_def_stmt)
+      || (gimple_assign_rhs_class (arg0_def_stmt) != GIMPLE_UNARY_RHS
+	  && gimple_assign_rhs_code (arg0_def_stmt) != VIEW_CONVERT_EXPR))
     return NULL;
 
   /* Use the RHS as new_arg0.  */
-  convert_code = gimple_assign_rhs_code (arg0_def_stmt);
+  op_code = gimple_assign_rhs_code (arg0_def_stmt);
   new_arg0 = gimple_assign_rhs1 (arg0_def_stmt);
-  if (convert_code == VIEW_CONVERT_EXPR)
+  if (op_code == VIEW_CONVERT_EXPR)
     {
       new_arg0 = TREE_OPERAND (new_arg0, 0);
       if (!is_gimple_reg_type (TREE_TYPE (new_arg0)))
@@ -267,10 +269,10 @@ factor_out_conditional_conversion (edge e0, edge e1, gphi *phi,
   if (TREE_CODE (arg1) == SSA_NAME)
     {
       /* Check if arg1 is an SSA_NAME and the stmt which defines arg1
-	 is a conversion.  */
+	 is an unary operation.  */
       arg1_def_stmt = SSA_NAME_DEF_STMT (arg1);
-      if (!is_gimple_assign (arg1_def_stmt)
-	  || gimple_assign_rhs_code (arg1_def_stmt) != convert_code)
+       if (!is_gimple_assign (arg1_def_stmt)
+	   || gimple_assign_rhs_code (arg1_def_stmt) != op_code)
 	return NULL;
 
       /* Either arg1_def_stmt or arg0_def_stmt should be conditional.  */
@@ -281,7 +283,7 @@ factor_out_conditional_conversion (edge e0, edge e1, gphi *phi,
 
       /* Use the RHS as new_arg1.  */
       new_arg1 = gimple_assign_rhs1 (arg1_def_stmt);
-      if (convert_code == VIEW_CONVERT_EXPR)
+      if (op_code == VIEW_CONVERT_EXPR)
 	new_arg1 = TREE_OPERAND (new_arg1, 0);
       if (TREE_CODE (new_arg1) == SSA_NAME
 	  && SSA_NAME_OCCURS_IN_ABNORMAL_PHI (new_arg1))
@@ -289,6 +291,10 @@ factor_out_conditional_conversion (edge e0, edge e1, gphi *phi,
     }
   else
     {
+      /* TODO: handle more than just casts here. */
+      if (!gimple_assign_cast_p (arg0_def_stmt))
+	return NULL;
+
       /* arg0_def_stmt should be conditional.  */
       if (dominated_by_p (CDI_DOMINATORS, gimple_bb (phi), gimple_bb (arg0_def_stmt)))
 	return NULL;
@@ -366,13 +372,13 @@ factor_out_conditional_conversion (edge e0, edge e1, gphi *phi,
       fprintf (dump_file, "PHI ");
       print_generic_expr (dump_file, gimple_phi_result (phi));
       fprintf (dump_file,
-	       " changed to factor conversion out from COND_EXPR.\n");
-      fprintf (dump_file, "New stmt with CAST that defines ");
+	       " changed to factor operation out from COND_EXPR.\n");
+      fprintf (dump_file, "New stmt with OPERATION that defines ");
       print_generic_expr (dump_file, result);
       fprintf (dump_file, ".\n");
     }
 
-  /* Remove the old cast(s) that has single use.  */
+  /* Remove the old operation(s) that has single use.  */
   gsi_for_def = gsi_for_stmt (arg0_def_stmt);
   gsi_remove (&gsi_for_def, true);
   release_defs (arg0_def_stmt);
@@ -387,14 +393,14 @@ factor_out_conditional_conversion (edge e0, edge e1, gphi *phi,
   add_phi_arg (newphi, new_arg0, e0, locus);
   add_phi_arg (newphi, new_arg1, e1, locus);
 
-  /* Create the conversion stmt and insert it.  */
-  if (convert_code == VIEW_CONVERT_EXPR)
+  /* Create the operation stmt and insert it.  */
+  if (op_code == VIEW_CONVERT_EXPR)
     {
       temp = fold_build1 (VIEW_CONVERT_EXPR, TREE_TYPE (result), temp);
       new_stmt = gimple_build_assign (result, temp);
     }
   else
-    new_stmt = gimple_build_assign (result, convert_code, temp);
+    new_stmt = gimple_build_assign (result, op_code, temp);
   gsi = gsi_after_labels (gimple_bb (phi));
   gsi_insert_before (&gsi, new_stmt, GSI_SAME_STMT);
 
@@ -402,7 +408,7 @@ factor_out_conditional_conversion (edge e0, edge e1, gphi *phi,
   gsi = gsi_for_stmt (phi);
   gsi_remove (&gsi, true);
 
-  statistics_counter_event (cfun, "factored out cast", 1);
+  statistics_counter_event (cfun, "factored out operation", 1);
 
   return newphi;
 }
@@ -3847,11 +3853,11 @@ gate_hoist_loads (void)
    This pass also performs a fifth transformation of a slightly different
    flavor.
 
-   Factor conversion in COND_EXPR
+   Factor operations in COND_EXPR
    ------------------------------
 
-   This transformation factors the conversion out of COND_EXPR with
-   factor_out_conditional_conversion.
+   This transformation factors the unary operations out of COND_EXPR with
+   factor_out_conditional_operation.
 
    For example:
    if (a <= CST) goto <bb 3>; else goto <bb 4>;
@@ -4092,15 +4098,15 @@ pass_phiopt::execute (function *)
 	  while (newphi)
 	    {
 	      phi = newphi;
-	      /* factor_out_conditional_conversion may create a new PHI in
+	      /* factor_out_conditional_operation may create a new PHI in
 		 BB2 and eliminate an existing PHI in BB2.  Recompute values
 		 that may be affected by that change.  */
 	      arg0 = gimple_phi_arg_def (phi, e1->dest_idx);
 	      arg1 = gimple_phi_arg_def (phi, e2->dest_idx);
 	      gcc_assert (arg0 != NULL_TREE && arg1 != NULL_TREE);
-	      newphi = factor_out_conditional_conversion (e1, e2, phi,
-							  arg0, arg1,
-							  cond_stmt);
+	      newphi = factor_out_conditional_operation (e1, e2, phi,
+							 arg0, arg1,
+							 cond_stmt);
 	    }
 	}
 
-- 
2.31.1

Re: [PATCH 3/3] PHIOPT: factor out unary operations instead of just conversions

[Richard Biener]

On Sun, May 7, 2023 at 6:44 AM Andrew Pinski via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> After using factor_out_conditional_conversion with diamond bb,
> we should be able do use it also for all normal unary gimple and not
> just conversions. This allows to optimize PR 59424 for an example.
> This is also a start to optimize PR 64700 and a few others.
>
> OK? Bootstrapped and tested on x86_64-linux-gnu with no regressions.

OK.

> An example of this is:
> ```
> static inline unsigned long long g(int t)
> {
>   unsigned t1 = t;
>   return t1;
> }
> static int abs1(int a)
> {
>   if (a < 0)
>     a = -a;
>   return a;
> }
> unsigned long long f(int c, int d, int e)
> {
>   unsigned long long t;
>   if (d > e)
>     t = g(abs1(d));
>   else
>     t = g(abs1(e));
>   return t;
> }
> ```
>
> Which should be optimized to:
>   _9 = MAX_EXPR <d_5(D), e_6(D)>;
>   _4 = ABS_EXPR <_9>;
>   t_3 = (long long unsigned intD.16) _4;
>
> gcc/ChangeLog:
>
>         * tree-ssa-phiopt.cc (factor_out_conditional_conversion): Rename to ...
>         (factor_out_conditional_operation): This and add support for all unary
>         operations.
>         (pass_phiopt::execute): Update call to factor_out_conditional_conversion
>         to call factor_out_conditional_operation instead.
>
>         PR tree-optimization/109424
>         PR tree-optimization/59424
>
> gcc/testsuite/ChangeLog:
>
>         * gcc.dg/tree-ssa/abs-2.c: Update tree scan for
>         details change in wording.
>         * gcc.dg/tree-ssa/minmax-17.c: Likewise.
>         * gcc.dg/tree-ssa/pr103771.c: Likewise.
>         * gcc.dg/tree-ssa/minmax-18.c: New test.
>         * gcc.dg/tree-ssa/minmax-19.c: New test.
> ---
>  gcc/testsuite/gcc.dg/tree-ssa/abs-2.c     |  2 +-
>  gcc/testsuite/gcc.dg/tree-ssa/minmax-17.c |  2 +-
>  gcc/testsuite/gcc.dg/tree-ssa/minmax-18.c | 27 +++++++++++
>  gcc/testsuite/gcc.dg/tree-ssa/minmax-19.c | 10 ++++
>  gcc/testsuite/gcc.dg/tree-ssa/pr103771.c  |  2 +-
>  gcc/tree-ssa-phiopt.cc                    | 56 +++++++++++++----------
>  6 files changed, 71 insertions(+), 28 deletions(-)
>  create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/minmax-18.c
>  create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/minmax-19.c
>
> diff --git a/gcc/testsuite/gcc.dg/tree-ssa/abs-2.c b/gcc/testsuite/gcc.dg/tree-ssa/abs-2.c
> index 328b1802541..f8bbeb43237 100644
> --- a/gcc/testsuite/gcc.dg/tree-ssa/abs-2.c
> +++ b/gcc/testsuite/gcc.dg/tree-ssa/abs-2.c
> @@ -16,5 +16,5 @@ test_abs(int *cur)
>
>  /* We should figure out that test_abs has an ABS_EXPR in it. */
>  /* { dg-final { scan-tree-dump " = ABS_EXPR" "phiopt1"} } */
> -/* { dg-final { scan-tree-dump-times "changed to factor conversion out from" 1 "phiopt1"} } */
> +/* { dg-final { scan-tree-dump-times "changed to factor operation out from" 1 "phiopt1"} } */
>
> diff --git a/gcc/testsuite/gcc.dg/tree-ssa/minmax-17.c b/gcc/testsuite/gcc.dg/tree-ssa/minmax-17.c
> index bd737e6b4cb..7c76cfc62a9 100644
> --- a/gcc/testsuite/gcc.dg/tree-ssa/minmax-17.c
> +++ b/gcc/testsuite/gcc.dg/tree-ssa/minmax-17.c
> @@ -18,4 +18,4 @@ unsigned long long test_max(int c, int d, int e)
>
>  /* We should figure out that test_max has an MAX_EXPR in it. */
>  /* { dg-final { scan-tree-dump " = MAX_EXPR" "phiopt1"} } */
> -/* { dg-final { scan-tree-dump-times "changed to factor conversion out from" 2 "phiopt1"} } */
> +/* { dg-final { scan-tree-dump-times "changed to factor operation out from" 2 "phiopt1"} } */
> diff --git a/gcc/testsuite/gcc.dg/tree-ssa/minmax-18.c b/gcc/testsuite/gcc.dg/tree-ssa/minmax-18.c
> new file mode 100644
> index 00000000000..c8e1670f64a
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/tree-ssa/minmax-18.c
> @@ -0,0 +1,27 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O -fdump-tree-phiopt1-details" } */
> +
> +static inline unsigned long long g(int t)
> +{
> +  unsigned t1 = t;
> +  return t1;
> +}
> +static inline int abs1(int a)
> +{
> +  if (a < 0)
> +    a = -a;
> +  return a;
> +}
> +unsigned long long f(int c, int d, int e)
> +{
> +  unsigned long long t;
> +  if (d > e)
> +    t = g(abs1(d));
> +  else
> +    t = g(abs1(e));
> +  return t;
> +}
> +
> +/* { dg-final { scan-tree-dump " = MAX_EXPR" "phiopt1"} } */
> +/* { dg-final { scan-tree-dump-times " = ABS_EXPR" 2 "phiopt1"} } */
> +/* { dg-final { scan-tree-dump-times "changed to factor operation out from" 3 "phiopt1"} } */
> diff --git a/gcc/testsuite/gcc.dg/tree-ssa/minmax-19.c b/gcc/testsuite/gcc.dg/tree-ssa/minmax-19.c
> new file mode 100644
> index 00000000000..5ed55fe2e23
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/tree-ssa/minmax-19.c
> @@ -0,0 +1,10 @@
> +/* PR tree-optimization/109424 */
> +/* { dg-do compile } */
> +/* { dg-options "-O -fdump-tree-phiopt1-details" } */
> +
> +int f2(int x, int y)
> +{
> +    return (x > y) ? ~x : ~y;
> +}
> +/* { dg-final { scan-tree-dump " = MAX_EXPR" "phiopt1"} } */
> +/* { dg-final { scan-tree-dump-times "changed to factor operation out from" 1 "phiopt1"} } */
> diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr103771.c b/gcc/testsuite/gcc.dg/tree-ssa/pr103771.c
> index 97c9db846cb..8faa45a8222 100644
> --- a/gcc/testsuite/gcc.dg/tree-ssa/pr103771.c
> +++ b/gcc/testsuite/gcc.dg/tree-ssa/pr103771.c
> @@ -1,6 +1,6 @@
>  /* { dg-do compile } */
>  /* { dg-options "-O3 -fdump-tree-phiopt1-details" } */
> -/* { dg-final { scan-tree-dump-times "changed to factor conversion out from COND_EXPR." 1 "phiopt1" } } */
> +/* { dg-final { scan-tree-dump-times "changed to factor operation out from COND_EXPR." 1 "phiopt1" } } */
>
>  typedef unsigned char uint8_t;
>
> diff --git a/gcc/tree-ssa-phiopt.cc b/gcc/tree-ssa-phiopt.cc
> index 7fe088b13ff..2fb28b4e60e 100644
> --- a/gcc/tree-ssa-phiopt.cc
> +++ b/gcc/tree-ssa-phiopt.cc
> @@ -209,13 +209,13 @@ replace_phi_edge_with_variable (basic_block cond_block,
>               bb->index);
>  }
>
> -/* PR66726: Factor conversion out of COND_EXPR.  If the arguments of the PHI
> +/* PR66726: Factor operations out of COND_EXPR.  If the arguments of the PHI
>     stmt are CONVERT_STMT, factor out the conversion and perform the conversion
>     to the result of PHI stmt.  COND_STMT is the controlling predicate.
>     Return the newly-created PHI, if any.  */
>
>  static gphi *
> -factor_out_conditional_conversion (edge e0, edge e1, gphi *phi,
> +factor_out_conditional_operation (edge e0, edge e1, gphi *phi,
>                                    tree arg0, tree arg1, gimple *cond_stmt)
>  {
>    gimple *arg0_def_stmt = NULL, *arg1_def_stmt = NULL, *new_stmt;
> @@ -224,7 +224,7 @@ factor_out_conditional_conversion (edge e0, edge e1, gphi *phi,
>    gphi *newphi;
>    gimple_stmt_iterator gsi, gsi_for_def;
>    location_t locus = gimple_location (phi);
> -  enum tree_code convert_code;
> +  enum tree_code op_code;
>
>    /* Handle only PHI statements with two arguments.  TODO: If all
>       other arguments to PHI are INTEGER_CST or if their defining
> @@ -246,15 +246,17 @@ factor_out_conditional_conversion (edge e0, edge e1, gphi *phi,
>      return NULL;
>
>    /* Check if arg0 is an SSA_NAME and the stmt which defines arg0 is
> -     a conversion.  */
> +     an unary operation.  */
>    arg0_def_stmt = SSA_NAME_DEF_STMT (arg0);
> -  if (!gimple_assign_cast_p (arg0_def_stmt))
> +  if (!is_gimple_assign (arg0_def_stmt)
> +      || (gimple_assign_rhs_class (arg0_def_stmt) != GIMPLE_UNARY_RHS
> +         && gimple_assign_rhs_code (arg0_def_stmt) != VIEW_CONVERT_EXPR))
>      return NULL;
>
>    /* Use the RHS as new_arg0.  */
> -  convert_code = gimple_assign_rhs_code (arg0_def_stmt);
> +  op_code = gimple_assign_rhs_code (arg0_def_stmt);
>    new_arg0 = gimple_assign_rhs1 (arg0_def_stmt);
> -  if (convert_code == VIEW_CONVERT_EXPR)
> +  if (op_code == VIEW_CONVERT_EXPR)
>      {
>        new_arg0 = TREE_OPERAND (new_arg0, 0);
>        if (!is_gimple_reg_type (TREE_TYPE (new_arg0)))
> @@ -267,10 +269,10 @@ factor_out_conditional_conversion (edge e0, edge e1, gphi *phi,
>    if (TREE_CODE (arg1) == SSA_NAME)
>      {
>        /* Check if arg1 is an SSA_NAME and the stmt which defines arg1
> -        is a conversion.  */
> +        is an unary operation.  */
>        arg1_def_stmt = SSA_NAME_DEF_STMT (arg1);
> -      if (!is_gimple_assign (arg1_def_stmt)
> -         || gimple_assign_rhs_code (arg1_def_stmt) != convert_code)
> +       if (!is_gimple_assign (arg1_def_stmt)
> +          || gimple_assign_rhs_code (arg1_def_stmt) != op_code)
>         return NULL;
>
>        /* Either arg1_def_stmt or arg0_def_stmt should be conditional.  */
> @@ -281,7 +283,7 @@ factor_out_conditional_conversion (edge e0, edge e1, gphi *phi,
>
>        /* Use the RHS as new_arg1.  */
>        new_arg1 = gimple_assign_rhs1 (arg1_def_stmt);
> -      if (convert_code == VIEW_CONVERT_EXPR)
> +      if (op_code == VIEW_CONVERT_EXPR)
>         new_arg1 = TREE_OPERAND (new_arg1, 0);
>        if (TREE_CODE (new_arg1) == SSA_NAME
>           && SSA_NAME_OCCURS_IN_ABNORMAL_PHI (new_arg1))
> @@ -289,6 +291,10 @@ factor_out_conditional_conversion (edge e0, edge e1, gphi *phi,
>      }
>    else
>      {
> +      /* TODO: handle more than just casts here. */
> +      if (!gimple_assign_cast_p (arg0_def_stmt))
> +       return NULL;
> +
>        /* arg0_def_stmt should be conditional.  */
>        if (dominated_by_p (CDI_DOMINATORS, gimple_bb (phi), gimple_bb (arg0_def_stmt)))
>         return NULL;
> @@ -366,13 +372,13 @@ factor_out_conditional_conversion (edge e0, edge e1, gphi *phi,
>        fprintf (dump_file, "PHI ");
>        print_generic_expr (dump_file, gimple_phi_result (phi));
>        fprintf (dump_file,
> -              " changed to factor conversion out from COND_EXPR.\n");
> -      fprintf (dump_file, "New stmt with CAST that defines ");
> +              " changed to factor operation out from COND_EXPR.\n");
> +      fprintf (dump_file, "New stmt with OPERATION that defines ");
>        print_generic_expr (dump_file, result);
>        fprintf (dump_file, ".\n");
>      }
>
> -  /* Remove the old cast(s) that has single use.  */
> +  /* Remove the old operation(s) that has single use.  */
>    gsi_for_def = gsi_for_stmt (arg0_def_stmt);
>    gsi_remove (&gsi_for_def, true);
>    release_defs (arg0_def_stmt);
> @@ -387,14 +393,14 @@ factor_out_conditional_conversion (edge e0, edge e1, gphi *phi,
>    add_phi_arg (newphi, new_arg0, e0, locus);
>    add_phi_arg (newphi, new_arg1, e1, locus);
>
> -  /* Create the conversion stmt and insert it.  */
> -  if (convert_code == VIEW_CONVERT_EXPR)
> +  /* Create the operation stmt and insert it.  */
> +  if (op_code == VIEW_CONVERT_EXPR)
>      {
>        temp = fold_build1 (VIEW_CONVERT_EXPR, TREE_TYPE (result), temp);
>        new_stmt = gimple_build_assign (result, temp);
>      }
>    else
> -    new_stmt = gimple_build_assign (result, convert_code, temp);
> +    new_stmt = gimple_build_assign (result, op_code, temp);
>    gsi = gsi_after_labels (gimple_bb (phi));
>    gsi_insert_before (&gsi, new_stmt, GSI_SAME_STMT);
>
> @@ -402,7 +408,7 @@ factor_out_conditional_conversion (edge e0, edge e1, gphi *phi,
>    gsi = gsi_for_stmt (phi);
>    gsi_remove (&gsi, true);
>
> -  statistics_counter_event (cfun, "factored out cast", 1);
> +  statistics_counter_event (cfun, "factored out operation", 1);
>
>    return newphi;
>  }
> @@ -3847,11 +3853,11 @@ gate_hoist_loads (void)
>     This pass also performs a fifth transformation of a slightly different
>     flavor.
>
> -   Factor conversion in COND_EXPR
> +   Factor operations in COND_EXPR
>     ------------------------------
>
> -   This transformation factors the conversion out of COND_EXPR with
> -   factor_out_conditional_conversion.
> +   This transformation factors the unary operations out of COND_EXPR with
> +   factor_out_conditional_operation.
>
>     For example:
>     if (a <= CST) goto <bb 3>; else goto <bb 4>;
> @@ -4092,15 +4098,15 @@ pass_phiopt::execute (function *)
>           while (newphi)
>             {
>               phi = newphi;
> -             /* factor_out_conditional_conversion may create a new PHI in
> +             /* factor_out_conditional_operation may create a new PHI in
>                  BB2 and eliminate an existing PHI in BB2.  Recompute values
>                  that may be affected by that change.  */
>               arg0 = gimple_phi_arg_def (phi, e1->dest_idx);
>               arg1 = gimple_phi_arg_def (phi, e2->dest_idx);
>               gcc_assert (arg0 != NULL_TREE && arg1 != NULL_TREE);
> -             newphi = factor_out_conditional_conversion (e1, e2, phi,
> -                                                         arg0, arg1,
> -                                                         cond_stmt);
> +             newphi = factor_out_conditional_operation (e1, e2, phi,
> +                                                        arg0, arg1,
> +                                                        cond_stmt);
>             }
>         }
>
> --
> 2.31.1
>

Re: [PATCH 1/3] PHIOPT: Add diamond bb form to factor_out_conditional_conversion

[Richard Biener]

On Sun, May 7, 2023 at 7:05 AM Andrew Pinski via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> So the function factor_out_conditional_conversion already supports
> diamond shaped bb forms, just need to be called for such a thing.
>
> harden-cond-comp.c needed to be changed as we would optimize out the
> conversion now and that causes the compare hardening not needing to
> split the block which it was testing. So change it such that there
> would be no chance of optimization.
>
> Also add two testcases that showed the improvement. PR 103771 is
> solved in ifconvert also for the vectorizer but now it is solved
> in a general sense.
>
> OK? Bootstrapped and tested on x86_64-linux-gnu with no regressions.

So what does it do for the diamond case?  Factor out _common_ operations
only or also promote conditionally executed operations to unconditionally
executed?  Apart from cost considerations (with the looping patch any number
might end up promoted) there's correctness issues for negation (signed overflow)
and for non-call exceptions there's possible external throws.

If it only factors out common operations the patch is OK (but the testcases
suggest that's not the case).  Otherwise we should add a limit as to how many
ops we hoist (maybe one for the diamond case?).  Thinking over it the same
issue of course exists for the non-diamond case?

Richard.

>         PR tree-optimization/49959
>         PR tree-optimization/103771
>
> gcc/ChangeLog:
>
>         * tree-ssa-phiopt.cc (pass_phiopt::execute): Support
>         Diamond shapped bb form for factor_out_conditional_conversion.
>
> gcc/testsuite/ChangeLog:
>
>         * c-c++-common/torture/harden-cond-comp.c: Change testcase
>         slightly to avoid the new phiopt optimization.
>         * gcc.dg/tree-ssa/abs-2.c: New test.
>         * gcc.dg/tree-ssa/pr103771.c: New test.
> ---
>  .../c-c++-common/torture/harden-cond-comp.c   |  6 +++---
>  gcc/testsuite/gcc.dg/tree-ssa/abs-2.c         | 20 +++++++++++++++++++
>  gcc/testsuite/gcc.dg/tree-ssa/pr103771.c      | 18 +++++++++++++++++
>  gcc/tree-ssa-phiopt.cc                        |  2 +-
>  4 files changed, 42 insertions(+), 4 deletions(-)
>  create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/abs-2.c
>  create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/pr103771.c
>
> diff --git a/gcc/testsuite/c-c++-common/torture/harden-cond-comp.c b/gcc/testsuite/c-c++-common/torture/harden-cond-comp.c
> index 5aad890a1d3..dcf364ee993 100644
> --- a/gcc/testsuite/c-c++-common/torture/harden-cond-comp.c
> +++ b/gcc/testsuite/c-c++-common/torture/harden-cond-comp.c
> @@ -1,11 +1,11 @@
>  /* { dg-do compile } */
>  /* { dg-options "-fharden-conditional-branches -fharden-compares -fdump-tree-hardcbr -fdump-tree-hardcmp -ffat-lto-objects" } */
>
> -int f(int i, int j) {
> +int f(int i, int j, int k, int l) {
>    if (i == 0)
> -    return j != 0;
> +    return (j != 0) + l;
>    else
> -    return i * j != 0;
> +    return (i * j != 0) * k;
>  }
>
>  /* { dg-final { scan-tree-dump-times "Splitting edge" 2 "hardcbr" } } */
> diff --git a/gcc/testsuite/gcc.dg/tree-ssa/abs-2.c b/gcc/testsuite/gcc.dg/tree-ssa/abs-2.c
> new file mode 100644
> index 00000000000..328b1802541
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/tree-ssa/abs-2.c
> @@ -0,0 +1,20 @@
> +/* PR tree-optimization/49959 */
> +/* { dg-do compile } */
> +/* { dg-options "-O -fdump-tree-phiopt1-details" } */
> +
> +#define ABS(X)    (((X)>0)?(X):-(X))
> +unsigned long
> +test_abs(int *cur)
> +{
> +  unsigned long sad = 0;
> +  if (cur[0] > 0)
> +    sad = cur[0];
> +  else
> +    sad = -cur[0];
> +  return sad;
> +}
> +
> +/* We should figure out that test_abs has an ABS_EXPR in it. */
> +/* { dg-final { scan-tree-dump " = ABS_EXPR" "phiopt1"} } */
> +/* { dg-final { scan-tree-dump-times "changed to factor conversion out from" 1 "phiopt1"} } */
> +
> diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr103771.c b/gcc/testsuite/gcc.dg/tree-ssa/pr103771.c
> new file mode 100644
> index 00000000000..97c9db846cb
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/tree-ssa/pr103771.c
> @@ -0,0 +1,18 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O3 -fdump-tree-phiopt1-details" } */
> +/* { dg-final { scan-tree-dump-times "changed to factor conversion out from COND_EXPR." 1 "phiopt1" } } */
> +
> +typedef unsigned char uint8_t;
> +
> +static uint8_t x264_clip_uint8 (int x)
> +{
> +  return x & (~255) ? (-x) >> 31 : x;
> +}
> +
> +void
> +mc_weight (uint8_t* __restrict dst, uint8_t* __restrict src,
> +          int i_width,int i_scale)
> +{
> +  for(int x = 0; x < i_width; x++)
> +    dst[x] = x264_clip_uint8 (src[x] * i_scale);
> +}
> diff --git a/gcc/tree-ssa-phiopt.cc b/gcc/tree-ssa-phiopt.cc
> index f14b7e8b7e6..41fea78dc8d 100644
> --- a/gcc/tree-ssa-phiopt.cc
> +++ b/gcc/tree-ssa-phiopt.cc
> @@ -4087,7 +4087,7 @@ pass_phiopt::execute (function *)
>
>        gphi *newphi;
>        if (single_pred_p (bb1)
> -         && !diamond_p
> +         && EDGE_COUNT (merge->preds) == 2
>           && (newphi = factor_out_conditional_conversion (e1, e2, phi,
>                                                           arg0, arg1,
>                                                           cond_stmt)))
> --
> 2.31.1
>

Re: [PATCH 1/3] PHIOPT: Add diamond bb form to factor_out_conditional_conversion

[Richard Biener]

On Mon, May 8, 2023 at 8:51 AM Richard Biener
<richard.guenther@gmail.com> wrote:
>
> On Sun, May 7, 2023 at 7:05 AM Andrew Pinski via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
> >
> > So the function factor_out_conditional_conversion already supports
> > diamond shaped bb forms, just need to be called for such a thing.
> >
> > harden-cond-comp.c needed to be changed as we would optimize out the
> > conversion now and that causes the compare hardening not needing to
> > split the block which it was testing. So change it such that there
> > would be no chance of optimization.
> >
> > Also add two testcases that showed the improvement. PR 103771 is
> > solved in ifconvert also for the vectorizer but now it is solved
> > in a general sense.
> >
> > OK? Bootstrapped and tested on x86_64-linux-gnu with no regressions.
>
> So what does it do for the diamond case?  Factor out _common_ operations
> only or also promote conditionally executed operations to unconditionally
> executed?  Apart from cost considerations (with the looping patch any number
> might end up promoted) there's correctness issues for negation (signed overflow)
> and for non-call exceptions there's possible external throws.
>
> If it only factors out common operations the patch is OK (but the testcases
> suggest that's not the case).  Otherwise we should add a limit as to how many
> ops we hoist (maybe one for the diamond case?).  Thinking over it the same
> issue of course exists for the non-diamond case?

Oh, and we hoist the stuff without being sure the final PHI will be if-converted
in the end, correct?  Can we somehow improve on that, aka tentatively
convert if-convert the PHI first?

Richard.

> Richard.
>
> >         PR tree-optimization/49959
> >         PR tree-optimization/103771
> >
> > gcc/ChangeLog:
> >
> >         * tree-ssa-phiopt.cc (pass_phiopt::execute): Support
> >         Diamond shapped bb form for factor_out_conditional_conversion.
> >
> > gcc/testsuite/ChangeLog:
> >
> >         * c-c++-common/torture/harden-cond-comp.c: Change testcase
> >         slightly to avoid the new phiopt optimization.
> >         * gcc.dg/tree-ssa/abs-2.c: New test.
> >         * gcc.dg/tree-ssa/pr103771.c: New test.
> > ---
> >  .../c-c++-common/torture/harden-cond-comp.c   |  6 +++---
> >  gcc/testsuite/gcc.dg/tree-ssa/abs-2.c         | 20 +++++++++++++++++++
> >  gcc/testsuite/gcc.dg/tree-ssa/pr103771.c      | 18 +++++++++++++++++
> >  gcc/tree-ssa-phiopt.cc                        |  2 +-
> >  4 files changed, 42 insertions(+), 4 deletions(-)
> >  create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/abs-2.c
> >  create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/pr103771.c
> >
> > diff --git a/gcc/testsuite/c-c++-common/torture/harden-cond-comp.c b/gcc/testsuite/c-c++-common/torture/harden-cond-comp.c
> > index 5aad890a1d3..dcf364ee993 100644
> > --- a/gcc/testsuite/c-c++-common/torture/harden-cond-comp.c
> > +++ b/gcc/testsuite/c-c++-common/torture/harden-cond-comp.c
> > @@ -1,11 +1,11 @@
> >  /* { dg-do compile } */
> >  /* { dg-options "-fharden-conditional-branches -fharden-compares -fdump-tree-hardcbr -fdump-tree-hardcmp -ffat-lto-objects" } */
> >
> > -int f(int i, int j) {
> > +int f(int i, int j, int k, int l) {
> >    if (i == 0)
> > -    return j != 0;
> > +    return (j != 0) + l;
> >    else
> > -    return i * j != 0;
> > +    return (i * j != 0) * k;
> >  }
> >
> >  /* { dg-final { scan-tree-dump-times "Splitting edge" 2 "hardcbr" } } */
> > diff --git a/gcc/testsuite/gcc.dg/tree-ssa/abs-2.c b/gcc/testsuite/gcc.dg/tree-ssa/abs-2.c
> > new file mode 100644
> > index 00000000000..328b1802541
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.dg/tree-ssa/abs-2.c
> > @@ -0,0 +1,20 @@
> > +/* PR tree-optimization/49959 */
> > +/* { dg-do compile } */
> > +/* { dg-options "-O -fdump-tree-phiopt1-details" } */
> > +
> > +#define ABS(X)    (((X)>0)?(X):-(X))
> > +unsigned long
> > +test_abs(int *cur)
> > +{
> > +  unsigned long sad = 0;
> > +  if (cur[0] > 0)
> > +    sad = cur[0];
> > +  else
> > +    sad = -cur[0];
> > +  return sad;
> > +}
> > +
> > +/* We should figure out that test_abs has an ABS_EXPR in it. */
> > +/* { dg-final { scan-tree-dump " = ABS_EXPR" "phiopt1"} } */
> > +/* { dg-final { scan-tree-dump-times "changed to factor conversion out from" 1 "phiopt1"} } */
> > +
> > diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr103771.c b/gcc/testsuite/gcc.dg/tree-ssa/pr103771.c
> > new file mode 100644
> > index 00000000000..97c9db846cb
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.dg/tree-ssa/pr103771.c
> > @@ -0,0 +1,18 @@
> > +/* { dg-do compile } */
> > +/* { dg-options "-O3 -fdump-tree-phiopt1-details" } */
> > +/* { dg-final { scan-tree-dump-times "changed to factor conversion out from COND_EXPR." 1 "phiopt1" } } */
> > +
> > +typedef unsigned char uint8_t;
> > +
> > +static uint8_t x264_clip_uint8 (int x)
> > +{
> > +  return x & (~255) ? (-x) >> 31 : x;
> > +}
> > +
> > +void
> > +mc_weight (uint8_t* __restrict dst, uint8_t* __restrict src,
> > +          int i_width,int i_scale)
> > +{
> > +  for(int x = 0; x < i_width; x++)
> > +    dst[x] = x264_clip_uint8 (src[x] * i_scale);
> > +}
> > diff --git a/gcc/tree-ssa-phiopt.cc b/gcc/tree-ssa-phiopt.cc
> > index f14b7e8b7e6..41fea78dc8d 100644
> > --- a/gcc/tree-ssa-phiopt.cc
> > +++ b/gcc/tree-ssa-phiopt.cc
> > @@ -4087,7 +4087,7 @@ pass_phiopt::execute (function *)
> >
> >        gphi *newphi;
> >        if (single_pred_p (bb1)
> > -         && !diamond_p
> > +         && EDGE_COUNT (merge->preds) == 2
> >           && (newphi = factor_out_conditional_conversion (e1, e2, phi,
> >                                                           arg0, arg1,
> >                                                           cond_stmt)))
> > --
> > 2.31.1
> >

Re: [PATCH 1/3] PHIOPT: Add diamond bb form to factor_out_conditional_conversion

[Andrew Pinski]

On Sun, May 7, 2023 at 11:53 PM Richard Biener via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> On Mon, May 8, 2023 at 8:51 AM Richard Biener
> <richard.guenther@gmail.com> wrote:
> >
> > On Sun, May 7, 2023 at 7:05 AM Andrew Pinski via Gcc-patches
> > <gcc-patches@gcc.gnu.org> wrote:
> > >
> > > So the function factor_out_conditional_conversion already supports
> > > diamond shaped bb forms, just need to be called for such a thing.
> > >
> > > harden-cond-comp.c needed to be changed as we would optimize out the
> > > conversion now and that causes the compare hardening not needing to
> > > split the block which it was testing. So change it such that there
> > > would be no chance of optimization.
> > >
> > > Also add two testcases that showed the improvement. PR 103771 is
> > > solved in ifconvert also for the vectorizer but now it is solved
> > > in a general sense.
> > >
> > > OK? Bootstrapped and tested on x86_64-linux-gnu with no regressions.
> >
> > So what does it do for the diamond case?  Factor out _common_ operations
> > only or also promote conditionally executed operations to unconditionally
> > executed?  Apart from cost considerations (with the looping patch any number
> > might end up promoted) there's correctness issues for negation (signed overflow)
> > and for non-call exceptions there's possible external throws.
> >
> > If it only factors out common operations the patch is OK (but the testcases
> > suggest that's not the case).

Right because the testcases are also testing if there was another
phiopt that happened in the same pass rather than having to wait
again.

>> Otherwise we should add a limit as to how many
> > ops we hoist (maybe one for the diamond case?).  Thinking over it the same
> > issue of course exists for the non-diamond case?
>
> Oh, and we hoist the stuff without being sure the final PHI will be if-converted
> in the end, correct?  Can we somehow improve on that, aka tentatively
> convert if-convert the PHI first?

Yes it hoists the stuff without any checks on if the final PHI will
have happened.
We have the same issue with hoisting adjacent loads.
I will think of a way to handle to maybe handle this better and even
undoing it; I will have to get back to you the details though.
And yes that was already an existing issue with the non-diamond case
of this; there was some extra checks added to prevent moving some
casts from being moved out of the PHI due to extra zero extends with
respect of __builtin_popcount/clz, etc.

Thanks,
Andrew



>
> Richard.
>
> > Richard.
> >
> > >         PR tree-optimization/49959
> > >         PR tree-optimization/103771
> > >
> > > gcc/ChangeLog:
> > >
> > >         * tree-ssa-phiopt.cc (pass_phiopt::execute): Support
> > >         Diamond shapped bb form for factor_out_conditional_conversion.
> > >
> > > gcc/testsuite/ChangeLog:
> > >
> > >         * c-c++-common/torture/harden-cond-comp.c: Change testcase
> > >         slightly to avoid the new phiopt optimization.
> > >         * gcc.dg/tree-ssa/abs-2.c: New test.
> > >         * gcc.dg/tree-ssa/pr103771.c: New test.
> > > ---
> > >  .../c-c++-common/torture/harden-cond-comp.c   |  6 +++---
> > >  gcc/testsuite/gcc.dg/tree-ssa/abs-2.c         | 20 +++++++++++++++++++
> > >  gcc/testsuite/gcc.dg/tree-ssa/pr103771.c      | 18 +++++++++++++++++
> > >  gcc/tree-ssa-phiopt.cc                        |  2 +-
> > >  4 files changed, 42 insertions(+), 4 deletions(-)
> > >  create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/abs-2.c
> > >  create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/pr103771.c
> > >
> > > diff --git a/gcc/testsuite/c-c++-common/torture/harden-cond-comp.c b/gcc/testsuite/c-c++-common/torture/harden-cond-comp.c
> > > index 5aad890a1d3..dcf364ee993 100644
> > > --- a/gcc/testsuite/c-c++-common/torture/harden-cond-comp.c
> > > +++ b/gcc/testsuite/c-c++-common/torture/harden-cond-comp.c
> > > @@ -1,11 +1,11 @@
> > >  /* { dg-do compile } */
> > >  /* { dg-options "-fharden-conditional-branches -fharden-compares -fdump-tree-hardcbr -fdump-tree-hardcmp -ffat-lto-objects" } */
> > >
> > > -int f(int i, int j) {
> > > +int f(int i, int j, int k, int l) {
> > >    if (i == 0)
> > > -    return j != 0;
> > > +    return (j != 0) + l;
> > >    else
> > > -    return i * j != 0;
> > > +    return (i * j != 0) * k;
> > >  }
> > >
> > >  /* { dg-final { scan-tree-dump-times "Splitting edge" 2 "hardcbr" } } */
> > > diff --git a/gcc/testsuite/gcc.dg/tree-ssa/abs-2.c b/gcc/testsuite/gcc.dg/tree-ssa/abs-2.c
> > > new file mode 100644
> > > index 00000000000..328b1802541
> > > --- /dev/null
> > > +++ b/gcc/testsuite/gcc.dg/tree-ssa/abs-2.c
> > > @@ -0,0 +1,20 @@
> > > +/* PR tree-optimization/49959 */
> > > +/* { dg-do compile } */
> > > +/* { dg-options "-O -fdump-tree-phiopt1-details" } */
> > > +
> > > +#define ABS(X)    (((X)>0)?(X):-(X))
> > > +unsigned long
> > > +test_abs(int *cur)
> > > +{
> > > +  unsigned long sad = 0;
> > > +  if (cur[0] > 0)
> > > +    sad = cur[0];
> > > +  else
> > > +    sad = -cur[0];
> > > +  return sad;
> > > +}
> > > +
> > > +/* We should figure out that test_abs has an ABS_EXPR in it. */
> > > +/* { dg-final { scan-tree-dump " = ABS_EXPR" "phiopt1"} } */
> > > +/* { dg-final { scan-tree-dump-times "changed to factor conversion out from" 1 "phiopt1"} } */
> > > +
> > > diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr103771.c b/gcc/testsuite/gcc.dg/tree-ssa/pr103771.c
> > > new file mode 100644
> > > index 00000000000..97c9db846cb
> > > --- /dev/null
> > > +++ b/gcc/testsuite/gcc.dg/tree-ssa/pr103771.c
> > > @@ -0,0 +1,18 @@
> > > +/* { dg-do compile } */
> > > +/* { dg-options "-O3 -fdump-tree-phiopt1-details" } */
> > > +/* { dg-final { scan-tree-dump-times "changed to factor conversion out from COND_EXPR." 1 "phiopt1" } } */
> > > +
> > > +typedef unsigned char uint8_t;
> > > +
> > > +static uint8_t x264_clip_uint8 (int x)
> > > +{
> > > +  return x & (~255) ? (-x) >> 31 : x;
> > > +}
> > > +
> > > +void
> > > +mc_weight (uint8_t* __restrict dst, uint8_t* __restrict src,
> > > +          int i_width,int i_scale)
> > > +{
> > > +  for(int x = 0; x < i_width; x++)
> > > +    dst[x] = x264_clip_uint8 (src[x] * i_scale);
> > > +}
> > > diff --git a/gcc/tree-ssa-phiopt.cc b/gcc/tree-ssa-phiopt.cc
> > > index f14b7e8b7e6..41fea78dc8d 100644
> > > --- a/gcc/tree-ssa-phiopt.cc
> > > +++ b/gcc/tree-ssa-phiopt.cc
> > > @@ -4087,7 +4087,7 @@ pass_phiopt::execute (function *)
> > >
> > >        gphi *newphi;
> > >        if (single_pred_p (bb1)
> > > -         && !diamond_p
> > > +         && EDGE_COUNT (merge->preds) == 2
> > >           && (newphi = factor_out_conditional_conversion (e1, e2, phi,
> > >                                                           arg0, arg1,
> > >                                                           cond_stmt)))
> > > --
> > > 2.31.1
> > >

Re: [PATCH 1/3] PHIOPT: Add diamond bb form to factor_out_conditional_conversion

[Richard Biener]

On Mon, May 8, 2023 at 9:09 AM Andrew Pinski <pinskia@gmail.com> wrote:
>
> On Sun, May 7, 2023 at 11:53 PM Richard Biener via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
> >
> > On Mon, May 8, 2023 at 8:51 AM Richard Biener
> > <richard.guenther@gmail.com> wrote:
> > >
> > > On Sun, May 7, 2023 at 7:05 AM Andrew Pinski via Gcc-patches
> > > <gcc-patches@gcc.gnu.org> wrote:
> > > >
> > > > So the function factor_out_conditional_conversion already supports
> > > > diamond shaped bb forms, just need to be called for such a thing.
> > > >
> > > > harden-cond-comp.c needed to be changed as we would optimize out the
> > > > conversion now and that causes the compare hardening not needing to
> > > > split the block which it was testing. So change it such that there
> > > > would be no chance of optimization.
> > > >
> > > > Also add two testcases that showed the improvement. PR 103771 is
> > > > solved in ifconvert also for the vectorizer but now it is solved
> > > > in a general sense.
> > > >
> > > > OK? Bootstrapped and tested on x86_64-linux-gnu with no regressions.
> > >
> > > So what does it do for the diamond case?  Factor out _common_ operations
> > > only or also promote conditionally executed operations to unconditionally
> > > executed?  Apart from cost considerations (with the looping patch any number
> > > might end up promoted) there's correctness issues for negation (signed overflow)
> > > and for non-call exceptions there's possible external throws.
> > >
> > > If it only factors out common operations the patch is OK (but the testcases
> > > suggest that's not the case).
>
> Right because the testcases are also testing if there was another
> phiopt that happened in the same pass rather than having to wait
> again.
>
> >> Otherwise we should add a limit as to how many
> > > ops we hoist (maybe one for the diamond case?).  Thinking over it the same
> > > issue of course exists for the non-diamond case?
> >
> > Oh, and we hoist the stuff without being sure the final PHI will be if-converted
> > in the end, correct?  Can we somehow improve on that, aka tentatively
> > convert if-convert the PHI first?
>
> Yes it hoists the stuff without any checks on if the final PHI will
> have happened.
> We have the same issue with hoisting adjacent loads.
> I will think of a way to handle to maybe handle this better and even
> undoing it; I will have to get back to you the details though.
> And yes that was already an existing issue with the non-diamond case
> of this; there was some extra checks added to prevent moving some
> casts from being moved out of the PHI due to extra zero extends with
> respect of __builtin_popcount/clz, etc.

OK.  Would be nice if you can somehow handle this.  Even adding a
static limit to the number of hoisted ops isn't good - multiple phiopt
passes will simply run up to N times such limit :/  Undoing might be
"easiest", best would be separating analysis and transform phase
somehow.

Let's go with the patch as-is in the meantime.

Thanks,
Richard.

> Thanks,
> Andrew
>
>
>
> >
> > Richard.
> >
> > > Richard.
> > >
> > > >         PR tree-optimization/49959
> > > >         PR tree-optimization/103771
> > > >
> > > > gcc/ChangeLog:
> > > >
> > > >         * tree-ssa-phiopt.cc (pass_phiopt::execute): Support
> > > >         Diamond shapped bb form for factor_out_conditional_conversion.
> > > >
> > > > gcc/testsuite/ChangeLog:
> > > >
> > > >         * c-c++-common/torture/harden-cond-comp.c: Change testcase
> > > >         slightly to avoid the new phiopt optimization.
> > > >         * gcc.dg/tree-ssa/abs-2.c: New test.
> > > >         * gcc.dg/tree-ssa/pr103771.c: New test.
> > > > ---
> > > >  .../c-c++-common/torture/harden-cond-comp.c   |  6 +++---
> > > >  gcc/testsuite/gcc.dg/tree-ssa/abs-2.c         | 20 +++++++++++++++++++
> > > >  gcc/testsuite/gcc.dg/tree-ssa/pr103771.c      | 18 +++++++++++++++++
> > > >  gcc/tree-ssa-phiopt.cc                        |  2 +-
> > > >  4 files changed, 42 insertions(+), 4 deletions(-)
> > > >  create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/abs-2.c
> > > >  create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/pr103771.c
> > > >
> > > > diff --git a/gcc/testsuite/c-c++-common/torture/harden-cond-comp.c b/gcc/testsuite/c-c++-common/torture/harden-cond-comp.c
> > > > index 5aad890a1d3..dcf364ee993 100644
> > > > --- a/gcc/testsuite/c-c++-common/torture/harden-cond-comp.c
> > > > +++ b/gcc/testsuite/c-c++-common/torture/harden-cond-comp.c
> > > > @@ -1,11 +1,11 @@
> > > >  /* { dg-do compile } */
> > > >  /* { dg-options "-fharden-conditional-branches -fharden-compares -fdump-tree-hardcbr -fdump-tree-hardcmp -ffat-lto-objects" } */
> > > >
> > > > -int f(int i, int j) {
> > > > +int f(int i, int j, int k, int l) {
> > > >    if (i == 0)
> > > > -    return j != 0;
> > > > +    return (j != 0) + l;
> > > >    else
> > > > -    return i * j != 0;
> > > > +    return (i * j != 0) * k;
> > > >  }
> > > >
> > > >  /* { dg-final { scan-tree-dump-times "Splitting edge" 2 "hardcbr" } } */
> > > > diff --git a/gcc/testsuite/gcc.dg/tree-ssa/abs-2.c b/gcc/testsuite/gcc.dg/tree-ssa/abs-2.c
> > > > new file mode 100644
> > > > index 00000000000..328b1802541
> > > > --- /dev/null
> > > > +++ b/gcc/testsuite/gcc.dg/tree-ssa/abs-2.c
> > > > @@ -0,0 +1,20 @@
> > > > +/* PR tree-optimization/49959 */
> > > > +/* { dg-do compile } */
> > > > +/* { dg-options "-O -fdump-tree-phiopt1-details" } */
> > > > +
> > > > +#define ABS(X)    (((X)>0)?(X):-(X))
> > > > +unsigned long
> > > > +test_abs(int *cur)
> > > > +{
> > > > +  unsigned long sad = 0;
> > > > +  if (cur[0] > 0)
> > > > +    sad = cur[0];
> > > > +  else
> > > > +    sad = -cur[0];
> > > > +  return sad;
> > > > +}
> > > > +
> > > > +/* We should figure out that test_abs has an ABS_EXPR in it. */
> > > > +/* { dg-final { scan-tree-dump " = ABS_EXPR" "phiopt1"} } */
> > > > +/* { dg-final { scan-tree-dump-times "changed to factor conversion out from" 1 "phiopt1"} } */
> > > > +
> > > > diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr103771.c b/gcc/testsuite/gcc.dg/tree-ssa/pr103771.c
> > > > new file mode 100644
> > > > index 00000000000..97c9db846cb
> > > > --- /dev/null
> > > > +++ b/gcc/testsuite/gcc.dg/tree-ssa/pr103771.c
> > > > @@ -0,0 +1,18 @@
> > > > +/* { dg-do compile } */
> > > > +/* { dg-options "-O3 -fdump-tree-phiopt1-details" } */
> > > > +/* { dg-final { scan-tree-dump-times "changed to factor conversion out from COND_EXPR." 1 "phiopt1" } } */
> > > > +
> > > > +typedef unsigned char uint8_t;
> > > > +
> > > > +static uint8_t x264_clip_uint8 (int x)
> > > > +{
> > > > +  return x & (~255) ? (-x) >> 31 : x;
> > > > +}
> > > > +
> > > > +void
> > > > +mc_weight (uint8_t* __restrict dst, uint8_t* __restrict src,
> > > > +          int i_width,int i_scale)
> > > > +{
> > > > +  for(int x = 0; x < i_width; x++)
> > > > +    dst[x] = x264_clip_uint8 (src[x] * i_scale);
> > > > +}
> > > > diff --git a/gcc/tree-ssa-phiopt.cc b/gcc/tree-ssa-phiopt.cc
> > > > index f14b7e8b7e6..41fea78dc8d 100644
> > > > --- a/gcc/tree-ssa-phiopt.cc
> > > > +++ b/gcc/tree-ssa-phiopt.cc
> > > > @@ -4087,7 +4087,7 @@ pass_phiopt::execute (function *)
> > > >
> > > >        gphi *newphi;
> > > >        if (single_pred_p (bb1)
> > > > -         && !diamond_p
> > > > +         && EDGE_COUNT (merge->preds) == 2
> > > >           && (newphi = factor_out_conditional_conversion (e1, e2, phi,
> > > >                                                           arg0, arg1,
> > > >                                                           cond_stmt)))
> > > > --
> > > > 2.31.1
> > > >

[PATCH 1/3] PHIOPT: Add diamond bb form to factor_out_conditional_conversion

[Andrew Pinski]

So the function factor_out_conditional_conversion already supports
diamond shaped bb forms, just need to be called for such a thing.

harden-cond-comp.c needed to be changed as we would optimize out the
conversion now and that causes the compare hardening not needing to
split the block which it was testing. So change it such that there
would be no chance of optimization.

Also add two testcases that showed the improvement. PR 103771 is
solved in ifconvert also for the vectorizer but now it is solved
in a general sense.

OK? Bootstrapped and tested on x86_64-linux-gnu with no regressions.

	PR tree-optimization/49959
	PR tree-optimization/103771

gcc/ChangeLog:

	* tree-ssa-phiopt.cc (pass_phiopt::execute): Support
	Diamond shapped bb form for factor_out_conditional_conversion.

gcc/testsuite/ChangeLog:

	* c-c++-common/torture/harden-cond-comp.c: Change testcase
	slightly to avoid the new phiopt optimization.
	* gcc.dg/tree-ssa/abs-2.c: New test.
	* gcc.dg/tree-ssa/pr103771.c: New test.
---
 .../c-c++-common/torture/harden-cond-comp.c   |  6 +++---
 gcc/testsuite/gcc.dg/tree-ssa/abs-2.c         | 20 +++++++++++++++++++
 gcc/testsuite/gcc.dg/tree-ssa/pr103771.c      | 18 +++++++++++++++++
 gcc/tree-ssa-phiopt.cc                        |  2 +-
 4 files changed, 42 insertions(+), 4 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/abs-2.c
 create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/pr103771.c

diff --git a/gcc/testsuite/c-c++-common/torture/harden-cond-comp.c b/gcc/testsuite/c-c++-common/torture/harden-cond-comp.c
index 5aad890a1d3..dcf364ee993 100644
--- a/gcc/testsuite/c-c++-common/torture/harden-cond-comp.c
+++ b/gcc/testsuite/c-c++-common/torture/harden-cond-comp.c
@@ -1,11 +1,11 @@
 /* { dg-do compile } */
 /* { dg-options "-fharden-conditional-branches -fharden-compares -fdump-tree-hardcbr -fdump-tree-hardcmp -ffat-lto-objects" } */
 
-int f(int i, int j) {
+int f(int i, int j, int k, int l) {
   if (i == 0)
-    return j != 0;
+    return (j != 0) + l;
   else
-    return i * j != 0;
+    return (i * j != 0) * k;
 }
 
 /* { dg-final { scan-tree-dump-times "Splitting edge" 2 "hardcbr" } } */
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/abs-2.c b/gcc/testsuite/gcc.dg/tree-ssa/abs-2.c
new file mode 100644
index 00000000000..328b1802541
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tree-ssa/abs-2.c
@@ -0,0 +1,20 @@
+/* PR tree-optimization/49959 */
+/* { dg-do compile } */
+/* { dg-options "-O -fdump-tree-phiopt1-details" } */
+
+#define ABS(X)    (((X)>0)?(X):-(X))
+unsigned long
+test_abs(int *cur)
+{
+  unsigned long sad = 0;
+  if (cur[0] > 0)
+    sad = cur[0];
+  else
+    sad = -cur[0];
+  return sad;
+}
+
+/* We should figure out that test_abs has an ABS_EXPR in it. */
+/* { dg-final { scan-tree-dump " = ABS_EXPR" "phiopt1"} } */
+/* { dg-final { scan-tree-dump-times "changed to factor conversion out from" 1 "phiopt1"} } */
+
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr103771.c b/gcc/testsuite/gcc.dg/tree-ssa/pr103771.c
new file mode 100644
index 00000000000..97c9db846cb
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tree-ssa/pr103771.c
@@ -0,0 +1,18 @@
+/* { dg-do compile } */
+/* { dg-options "-O3 -fdump-tree-phiopt1-details" } */
+/* { dg-final { scan-tree-dump-times "changed to factor conversion out from COND_EXPR." 1 "phiopt1" } } */
+
+typedef unsigned char uint8_t;
+
+static uint8_t x264_clip_uint8 (int x)
+{
+  return x & (~255) ? (-x) >> 31 : x;
+}
+
+void
+mc_weight (uint8_t* __restrict dst, uint8_t* __restrict src,
+	   int i_width,int i_scale)
+{
+  for(int x = 0; x < i_width; x++)
+    dst[x] = x264_clip_uint8 (src[x] * i_scale);
+}
diff --git a/gcc/tree-ssa-phiopt.cc b/gcc/tree-ssa-phiopt.cc
index f14b7e8b7e6..41fea78dc8d 100644
--- a/gcc/tree-ssa-phiopt.cc
+++ b/gcc/tree-ssa-phiopt.cc
@@ -4087,7 +4087,7 @@ pass_phiopt::execute (function *)
 
       gphi *newphi;
       if (single_pred_p (bb1)
-	  && !diamond_p
+	  && EDGE_COUNT (merge->preds) == 2
 	  && (newphi = factor_out_conditional_conversion (e1, e2, phi,
 							  arg0, arg1,
 							  cond_stmt)))
-- 
2.31.1