[6/6] PHIOPT: Add support for diamond shaped bb to match_simplify_replacement

Message ID 20230422220921.452264-7-apinski@marvell.com
State Accepted
Headers
Series Improve PHIOPT match and simplify for diamond shaped bbs |

Checks

Context Check Description
snail/gcc-patch-check success Github commit url

Commit Message

Andrew Pinski April 22, 2023, 10:09 p.m. UTC
  This adds diamond shaped form of basic blocks to match_simplify_replacement.
This is the patch is the start of removing/moving all
of what minmax_replacement does to match.pd to reduce the code duplication.

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

Note phi-opt-{23,24}.c testcase had an incorrect xfail as there should
have been 2 if still because f4/f5 would not be transformed as -ABS is
not allowable during early phi-opt.

gcc/ChangeLog:

	* tree-ssa-phiopt.cc (match_simplify_replacement): Add new arguments
	and support diamond shaped basic block form.
	(tree_ssa_phiopt_worker): Update call to match_simplify_replacement

gcc/testsuite/ChangeLog:

	* gcc.dg/tree-ssa/phi-opt-23.c: Update testcase.
	* gcc.dg/tree-ssa/phi-opt-24.c: Likewise.
---
 gcc/testsuite/gcc.dg/tree-ssa/phi-opt-23.c |  4 +--
 gcc/testsuite/gcc.dg/tree-ssa/phi-opt-24.c |  4 +--
 gcc/tree-ssa-phiopt.cc                     | 37 ++++++++++++++++++----
 3 files changed, 35 insertions(+), 10 deletions(-)
  

Comments

Richard Biener April 24, 2023, 12:07 p.m. UTC | #1
On Sun, Apr 23, 2023 at 12:13 AM Andrew Pinski via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> This adds diamond shaped form of basic blocks to match_simplify_replacement.
> This is the patch is the start of removing/moving all
> of what minmax_replacement does to match.pd to reduce the code duplication.
>
> OK? Bootstrapped and tested on x86_64-linux-gnu with no regressions.

OK.

thanks,
Richard.

> Note phi-opt-{23,24}.c testcase had an incorrect xfail as there should
> have been 2 if still because f4/f5 would not be transformed as -ABS is
> not allowable during early phi-opt.
>
> gcc/ChangeLog:
>
>         * tree-ssa-phiopt.cc (match_simplify_replacement): Add new arguments
>         and support diamond shaped basic block form.
>         (tree_ssa_phiopt_worker): Update call to match_simplify_replacement
>
> gcc/testsuite/ChangeLog:
>
>         * gcc.dg/tree-ssa/phi-opt-23.c: Update testcase.
>         * gcc.dg/tree-ssa/phi-opt-24.c: Likewise.
> ---
>  gcc/testsuite/gcc.dg/tree-ssa/phi-opt-23.c |  4 +--
>  gcc/testsuite/gcc.dg/tree-ssa/phi-opt-24.c |  4 +--
>  gcc/tree-ssa-phiopt.cc                     | 37 ++++++++++++++++++----
>  3 files changed, 35 insertions(+), 10 deletions(-)
>
> diff --git a/gcc/testsuite/gcc.dg/tree-ssa/phi-opt-23.c b/gcc/testsuite/gcc.dg/tree-ssa/phi-opt-23.c
> index ff658cd16a7..86aab955d5e 100644
> --- a/gcc/testsuite/gcc.dg/tree-ssa/phi-opt-23.c
> +++ b/gcc/testsuite/gcc.dg/tree-ssa/phi-opt-23.c
> @@ -38,7 +38,7 @@ int f5(int A)
>    return -A;
>  }
>
> -/* These should be optimized in phiopt1 but is confused by predicts. */
> -/* { dg-final { scan-tree-dump-not "if" "phiopt1" { xfail *-*-* } } } */
> +/* f4 and f5 are not allowed to be optimized in early phi-opt. */
> +/* { dg-final { scan-tree-dump-times "if" 2 "phiopt1" } } */
>  /* { dg-final { scan-tree-dump-not "if" "phiopt2" } } */
>
> diff --git a/gcc/testsuite/gcc.dg/tree-ssa/phi-opt-24.c b/gcc/testsuite/gcc.dg/tree-ssa/phi-opt-24.c
> index eb89decb4bf..bd8308efa0e 100644
> --- a/gcc/testsuite/gcc.dg/tree-ssa/phi-opt-24.c
> +++ b/gcc/testsuite/gcc.dg/tree-ssa/phi-opt-24.c
> @@ -38,7 +38,7 @@ float f5(float A)
>    return -A;
>  }
>
> -/* These should be optimized in phiopt1 but is confused by predicts. */
> -/* { dg-final { scan-tree-dump-not "if" "phiopt1" { xfail *-*-* } } } */
> +/* f4 and f5 are not allowed to be optimized in early phi-opt. */
> +/* { dg-final { scan-tree-dump-times "if" 2 "phiopt1" } } */
>  /* { dg-final { scan-tree-dump-not "if" "phiopt2" } } */
>
> diff --git a/gcc/tree-ssa-phiopt.cc b/gcc/tree-ssa-phiopt.cc
> index ffd6a4e6f35..757e44692ed 100644
> --- a/gcc/tree-ssa-phiopt.cc
> +++ b/gcc/tree-ssa-phiopt.cc
> @@ -57,8 +57,8 @@ along with GCC; see the file COPYING3.  If not see
>
>  static bool two_value_replacement (basic_block, basic_block, edge, gphi *,
>                                    tree, tree);
> -static bool match_simplify_replacement (basic_block, basic_block,
> -                                       edge, edge, gphi *, tree, tree, bool);
> +static bool match_simplify_replacement (basic_block, basic_block, basic_block,
> +                                       edge, edge, gphi *, tree, tree, bool, bool);
>  static gphi *factor_out_conditional_conversion (edge, edge, gphi *, tree, tree,
>                                                 gimple *);
>  static int value_replacement (basic_block, basic_block,
> @@ -304,9 +304,8 @@ tree_ssa_phiopt_worker (bool do_store_elim, bool do_hoist_loads, bool early_p)
>           && !diamond_p
>           && two_value_replacement (bb, bb1, e2, phi, arg0, arg1))
>         cfgchanged = true;
> -      else if (!diamond_p
> -              && match_simplify_replacement (bb, bb1, e1, e2, phi,
> -                                             arg0, arg1, early_p))
> +      else if (match_simplify_replacement (bb, bb1, bb2, e1, e2, phi,
> +                                          arg0, arg1, early_p, diamond_p))
>         cfgchanged = true;
>        else if (!early_p
>                && !diamond_p
> @@ -1026,8 +1025,10 @@ empty_bb_or_one_feeding_into_p (basic_block bb,
>
>  static bool
>  match_simplify_replacement (basic_block cond_bb, basic_block middle_bb,
> +                           basic_block middle_bb_alt,
>                             edge e0, edge e1, gphi *phi,
> -                           tree arg0, tree arg1, bool early_p)
> +                           tree arg0, tree arg1, bool early_p,
> +                           bool threeway_p)
>  {
>    gimple *stmt;
>    gimple_stmt_iterator gsi;
> @@ -1035,6 +1036,7 @@ match_simplify_replacement (basic_block cond_bb, basic_block middle_bb,
>    gimple_seq seq = NULL;
>    tree result;
>    gimple *stmt_to_move = NULL;
> +  gimple *stmt_to_move_alt = NULL;
>    auto_bitmap inserted_exprs;
>
>    /* Special case A ? B : B as this will always simplify to B. */
> @@ -1046,6 +1048,12 @@ match_simplify_replacement (basic_block cond_bb, basic_block middle_bb,
>    if (!empty_bb_or_one_feeding_into_p (middle_bb, phi, stmt_to_move))
>      return false;
>
> +  if (threeway_p
> +      && middle_bb != middle_bb_alt
> +      && !empty_bb_or_one_feeding_into_p (middle_bb_alt, phi,
> +                                         stmt_to_move_alt))
> +    return false;
> +
>      /* At this point we know we have a GIMPLE_COND with two successors.
>       One successor is BB, the other successor is an empty block which
>       falls through into BB.
> @@ -1110,6 +1118,23 @@ match_simplify_replacement (basic_block cond_bb, basic_block middle_bb,
>        reset_flow_sensitive_info (name);
>      }
>
> +  if (stmt_to_move_alt)
> +    {
> +      if (dump_file && (dump_flags & TDF_DETAILS))
> +       {
> +         fprintf (dump_file, "statement un-sinked:\n");
> +         print_gimple_stmt (dump_file, stmt_to_move_alt, 0,
> +                          TDF_VOPS|TDF_MEMSYMS);
> +       }
> +
> +      tree name = gimple_get_lhs (stmt_to_move_alt);
> +      // Mark the name to be renamed if there is one.
> +      bitmap_set_bit (inserted_exprs, SSA_NAME_VERSION (name));
> +      gimple_stmt_iterator gsi1 = gsi_for_stmt (stmt_to_move_alt);
> +      gsi_move_before (&gsi1, &gsi);
> +      reset_flow_sensitive_info (name);
> +    }
> +
>    replace_phi_edge_with_variable (cond_bb, e1, phi, result, inserted_exprs);
>
>    /* Add Statistic here even though replace_phi_edge_with_variable already
> --
> 2.39.1
>
  

Patch

diff --git a/gcc/testsuite/gcc.dg/tree-ssa/phi-opt-23.c b/gcc/testsuite/gcc.dg/tree-ssa/phi-opt-23.c
index ff658cd16a7..86aab955d5e 100644
--- a/gcc/testsuite/gcc.dg/tree-ssa/phi-opt-23.c
+++ b/gcc/testsuite/gcc.dg/tree-ssa/phi-opt-23.c
@@ -38,7 +38,7 @@  int f5(int A)
   return -A;
 }
 
-/* These should be optimized in phiopt1 but is confused by predicts. */
-/* { dg-final { scan-tree-dump-not "if" "phiopt1" { xfail *-*-* } } } */
+/* f4 and f5 are not allowed to be optimized in early phi-opt. */
+/* { dg-final { scan-tree-dump-times "if" 2 "phiopt1" } } */
 /* { dg-final { scan-tree-dump-not "if" "phiopt2" } } */
 
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/phi-opt-24.c b/gcc/testsuite/gcc.dg/tree-ssa/phi-opt-24.c
index eb89decb4bf..bd8308efa0e 100644
--- a/gcc/testsuite/gcc.dg/tree-ssa/phi-opt-24.c
+++ b/gcc/testsuite/gcc.dg/tree-ssa/phi-opt-24.c
@@ -38,7 +38,7 @@  float f5(float A)
   return -A;
 }
 
-/* These should be optimized in phiopt1 but is confused by predicts. */
-/* { dg-final { scan-tree-dump-not "if" "phiopt1" { xfail *-*-* } } } */
+/* f4 and f5 are not allowed to be optimized in early phi-opt. */
+/* { dg-final { scan-tree-dump-times "if" 2 "phiopt1" } } */
 /* { dg-final { scan-tree-dump-not "if" "phiopt2" } } */
 
diff --git a/gcc/tree-ssa-phiopt.cc b/gcc/tree-ssa-phiopt.cc
index ffd6a4e6f35..757e44692ed 100644
--- a/gcc/tree-ssa-phiopt.cc
+++ b/gcc/tree-ssa-phiopt.cc
@@ -57,8 +57,8 @@  along with GCC; see the file COPYING3.  If not see
 
 static bool two_value_replacement (basic_block, basic_block, edge, gphi *,
 				   tree, tree);
-static bool match_simplify_replacement (basic_block, basic_block,
-					edge, edge, gphi *, tree, tree, bool);
+static bool match_simplify_replacement (basic_block, basic_block, basic_block,
+					edge, edge, gphi *, tree, tree, bool, bool);
 static gphi *factor_out_conditional_conversion (edge, edge, gphi *, tree, tree,
 						gimple *);
 static int value_replacement (basic_block, basic_block,
@@ -304,9 +304,8 @@  tree_ssa_phiopt_worker (bool do_store_elim, bool do_hoist_loads, bool early_p)
 	  && !diamond_p
 	  && two_value_replacement (bb, bb1, e2, phi, arg0, arg1))
 	cfgchanged = true;
-      else if (!diamond_p
-	       && match_simplify_replacement (bb, bb1, e1, e2, phi,
-					      arg0, arg1, early_p))
+      else if (match_simplify_replacement (bb, bb1, bb2, e1, e2, phi,
+					   arg0, arg1, early_p, diamond_p))
 	cfgchanged = true;
       else if (!early_p
 	       && !diamond_p
@@ -1026,8 +1025,10 @@  empty_bb_or_one_feeding_into_p (basic_block bb,
 
 static bool
 match_simplify_replacement (basic_block cond_bb, basic_block middle_bb,
+			    basic_block middle_bb_alt,
 			    edge e0, edge e1, gphi *phi,
-			    tree arg0, tree arg1, bool early_p)
+			    tree arg0, tree arg1, bool early_p,
+			    bool threeway_p)
 {
   gimple *stmt;
   gimple_stmt_iterator gsi;
@@ -1035,6 +1036,7 @@  match_simplify_replacement (basic_block cond_bb, basic_block middle_bb,
   gimple_seq seq = NULL;
   tree result;
   gimple *stmt_to_move = NULL;
+  gimple *stmt_to_move_alt = NULL;
   auto_bitmap inserted_exprs;
 
   /* Special case A ? B : B as this will always simplify to B. */
@@ -1046,6 +1048,12 @@  match_simplify_replacement (basic_block cond_bb, basic_block middle_bb,
   if (!empty_bb_or_one_feeding_into_p (middle_bb, phi, stmt_to_move))
     return false;
 
+  if (threeway_p
+      && middle_bb != middle_bb_alt
+      && !empty_bb_or_one_feeding_into_p (middle_bb_alt, phi,
+					  stmt_to_move_alt))
+    return false;
+
     /* At this point we know we have a GIMPLE_COND with two successors.
      One successor is BB, the other successor is an empty block which
      falls through into BB.
@@ -1110,6 +1118,23 @@  match_simplify_replacement (basic_block cond_bb, basic_block middle_bb,
       reset_flow_sensitive_info (name);
     }
 
+  if (stmt_to_move_alt)
+    {
+      if (dump_file && (dump_flags & TDF_DETAILS))
+	{
+	  fprintf (dump_file, "statement un-sinked:\n");
+	  print_gimple_stmt (dump_file, stmt_to_move_alt, 0,
+			   TDF_VOPS|TDF_MEMSYMS);
+	}
+
+      tree name = gimple_get_lhs (stmt_to_move_alt);
+      // Mark the name to be renamed if there is one.
+      bitmap_set_bit (inserted_exprs, SSA_NAME_VERSION (name));
+      gimple_stmt_iterator gsi1 = gsi_for_stmt (stmt_to_move_alt);
+      gsi_move_before (&gsi1, &gsi);
+      reset_flow_sensitive_info (name);
+    }
+
   replace_phi_edge_with_variable (cond_bb, e1, phi, result, inserted_exprs);
 
   /* Add Statistic here even though replace_phi_edge_with_variable already