public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
From: Andrew Pinski <apinski@marvell.com>
To: <gcc-patches@gcc.gnu.org>
Cc: Andrew Pinski <apinski@marvell.com>
Subject: [PATCH 2/6] PHIOPT: Cleanup tree_ssa_phiopt_worker code
Date: Sat, 22 Apr 2023 15:09:17 -0700	[thread overview]
Message-ID: <20230422220921.452264-3-apinski@marvell.com> (raw)
In-Reply-To: <20230422220921.452264-1-apinski@marvell.com>

This patch cleans up tree_ssa_phiopt_worker by merging
common code. Making do_store_elim handled earlier.
Note this does not change any overall logic of the code,
just moves code around enough to be able to do this.
This will make it easier to move code around even more
and a few other fixes I have.
Plus I think all of the do_store_elim code really
should move to its own function as how much code is shared
is now obvious not much.

OK? Bootstrapped and tested on x86_64-linux-gnu.

gcc/ChangeLog:

	* tree-ssa-phiopt.cc (tree_ssa_phiopt_worker): Rearrange
	code for better code readability.
---
 gcc/tree-ssa-phiopt.cc | 212 +++++++++++++++++++++--------------------
 1 file changed, 107 insertions(+), 105 deletions(-)

diff --git a/gcc/tree-ssa-phiopt.cc b/gcc/tree-ssa-phiopt.cc
index 296ba646e62..05f19825ce9 100644
--- a/gcc/tree-ssa-phiopt.cc
+++ b/gcc/tree-ssa-phiopt.cc
@@ -175,10 +175,14 @@ tree_ssa_phiopt_worker (bool do_store_elim, bool do_hoist_loads, bool early_p)
 	  std::swap (bb1, bb2);
 	  std::swap (e1, e2);
 	}
-      else if (EDGE_SUCC (bb1, 0)->dest == EDGE_SUCC (bb2, 0)->dest)
+      else if (EDGE_SUCC (bb1, 0)->dest == EDGE_SUCC (bb2, 0)->dest
+	       && single_succ_p (bb2))
 	{
 	  diamond_p = true;
 	  e2 = EDGE_SUCC (bb2, 0);
+	  /* Make sure bb2 is just a fall through. */
+	  if ((e2->flags & EDGE_FALLTHRU) == 0)
+	    continue;
 	}
       else
 	continue;
@@ -190,46 +194,23 @@ tree_ssa_phiopt_worker (bool do_store_elim, bool do_hoist_loads, bool early_p)
 	  || (e1->flags & EDGE_FALLTHRU) == 0)
 	continue;
 
-      if (do_store_elim && diamond_p)
+      if (do_store_elim)
 	{
-	  basic_block bb3 = e1->dest;
-
-	  if (!single_succ_p (bb2)
-	      || (e2->flags & EDGE_FALLTHRU) == 0
-	      || EDGE_COUNT (bb3->preds) != 2)
-	    continue;
-	  if (cond_if_else_store_replacement (bb1, bb2, bb3))
-	    cfgchanged = true;
-	  continue;
-	}
-      else if (do_hoist_loads && diamond_p)
-	{
-	  basic_block bb3 = e1->dest;
-
-	  if (!FLOAT_TYPE_P (TREE_TYPE (gimple_cond_lhs (cond_stmt)))
-	      && single_succ_p (bb2)
-	      && single_pred_p (bb1)
-	      && single_pred_p (bb2)
-	      && EDGE_COUNT (bb->succs) == 2
-	      && EDGE_COUNT (bb3->preds) == 2
-	      /* If one edge or the other is dominant, a conditional move
-		 is likely to perform worse than the well-predicted branch.  */
-	      && !predictable_edge_p (EDGE_SUCC (bb, 0))
-	      && !predictable_edge_p (EDGE_SUCC (bb, 1)))
-	    hoist_adjacent_loads (bb, bb1, bb2, bb3);
-	  continue;
-	}
-
-      if (diamond_p)
-	{
-	  if (!single_pred_p (bb1)
-	      || !single_pred_p (bb2)
-	      || !single_succ_p (bb2))
-	    continue;
-	}
+	  if (diamond_p)
+	    {
+	      basic_block bb3 = e1->dest;
+
+	      /* Only handle sinking of store from 2 bbs only,
+		 The middle bbs don't need to come from the
+		 if always since we are sinking rather than
+		 hoisting. */
+	      if (EDGE_COUNT (bb3->preds) != 2)
+		continue;
+	      if (cond_if_else_store_replacement (bb1, bb2, bb3))
+		cfgchanged = true;
+	      continue;
+	    }
 
-      if (do_store_elim && !diamond_p)
-	{
 	  /* Also make sure that bb1 only have one predecessor and that it
 	     is bb.  */
 	  if (!single_pred_p (bb1)
@@ -243,85 +224,106 @@ tree_ssa_phiopt_worker (bool do_store_elim, bool do_hoist_loads, bool early_p)
 	    continue;
 	  if (cond_store_replacement (bb1, bb2, e1, e2, nontrap))
 	    cfgchanged = true;
+	  continue;
 	}
-      else
+
+      if (diamond_p)
 	{
-	  gimple_stmt_iterator gsi;
-	  bool candorest = true;
+	  basic_block bb3 = e1->dest;
+
+	  if (!single_pred_p (bb1)
+	      || !single_pred_p (bb2))
+	    continue;
 
-	  /* Check that we're looking for nested phis.  */
-	  basic_block merge = diamond_p ? EDGE_SUCC (bb2, 0)->dest : bb2;
-	  gimple_seq phis = phi_nodes (merge);
+	  if (do_hoist_loads
+	      && !FLOAT_TYPE_P (TREE_TYPE (gimple_cond_lhs (cond_stmt)))
+	      && EDGE_COUNT (bb->succs) == 2
+	      && EDGE_COUNT (bb3->preds) == 2
+	      /* If one edge or the other is dominant, a conditional move
+		 is likely to perform worse than the well-predicted branch.  */
+	      && !predictable_edge_p (EDGE_SUCC (bb, 0))
+	      && !predictable_edge_p (EDGE_SUCC (bb, 1)))
+	    {
+	      hoist_adjacent_loads (bb, bb1, bb2, bb3);
+	      continue;
+	    }
+	}
 
-	  /* Value replacement can work with more than one PHI
-	     so try that first. */
-	  if (!early_p && !diamond_p)
-	    for (gsi = gsi_start (phis); !gsi_end_p (gsi); gsi_next (&gsi))
+      gimple_stmt_iterator gsi;
+      bool candorest = true;
+
+      /* Check that we're looking for nested phis.  */
+      basic_block merge = diamond_p ? EDGE_SUCC (bb2, 0)->dest : bb2;
+      gimple_seq phis = phi_nodes (merge);
+
+      /* Value replacement can work with more than one PHI
+	 so try that first. */
+      if (!early_p && !diamond_p)
+	for (gsi = gsi_start (phis); !gsi_end_p (gsi); gsi_next (&gsi))
+	  {
+	    phi = as_a <gphi *> (gsi_stmt (gsi));
+	    arg0 = gimple_phi_arg_def (phi, e1->dest_idx);
+	    arg1 = gimple_phi_arg_def (phi, e2->dest_idx);
+	    if (value_replacement (bb, bb1, e1, e2, phi, arg0, arg1) == 2)
 	      {
-		phi = as_a <gphi *> (gsi_stmt (gsi));
-		arg0 = gimple_phi_arg_def (phi, e1->dest_idx);
-		arg1 = gimple_phi_arg_def (phi, e2->dest_idx);
-		if (value_replacement (bb, bb1, e1, e2, phi, arg0, arg1) == 2)
-		  {
-		    candorest = false;
-		    cfgchanged = true;
-		    break;
-		  }
+		candorest = false;
+		cfgchanged = true;
+		break;
 	      }
+	  }
 
-	  if (!candorest)
-	    continue;
+      if (!candorest)
+	continue;
 
-	  phi = single_non_singleton_phi_for_edges (phis, e1, e2);
-	  if (!phi)
-	    continue;
+      phi = single_non_singleton_phi_for_edges (phis, e1, e2);
+      if (!phi)
+	continue;
+
+      arg0 = gimple_phi_arg_def (phi, e1->dest_idx);
+      arg1 = gimple_phi_arg_def (phi, e2->dest_idx);
 
+      /* Something is wrong if we cannot find the arguments in the PHI
+	  node.  */
+      gcc_assert (arg0 != NULL_TREE && arg1 != NULL_TREE);
+
+      gphi *newphi;
+      if (single_pred_p (bb1)
+	  && !diamond_p
+	  && (newphi = factor_out_conditional_conversion (e1, e2, phi,
+							  arg0, arg1,
+							  cond_stmt)))
+	{
+	  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);
-
-	  /* Something is wrong if we cannot find the arguments in the PHI
-	     node.  */
 	  gcc_assert (arg0 != NULL_TREE && arg1 != NULL_TREE);
-
-	  gphi *newphi;
-	  if (single_pred_p (bb1)
-	      && !diamond_p
-	      && (newphi = factor_out_conditional_conversion (e1, e2, phi,
-							      arg0, arg1,
-							      cond_stmt)))
-	    {
-	      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);
-	    }
-
-	  /* Do the replacement of conditional if it can be done.  */
-	  if (!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))
-	    cfgchanged = true;
-	  else if (!early_p
-		   && !diamond_p
-		   && single_pred_p (bb1)
-		   && cond_removal_in_builtin_zero_pattern (bb, bb1, e1, e2,
-							    phi, arg0, arg1))
-	    cfgchanged = true;
-	  else if (minmax_replacement (bb, bb1, bb2, e1, e2, phi, arg0, arg1,
-				       diamond_p))
-	    cfgchanged = true;
-	  else if (single_pred_p (bb1)
-		   && !diamond_p
-		   && spaceship_replacement (bb, bb1, e1, e2, phi, arg0, arg1))
-	    cfgchanged = true;
 	}
+
+      /* Do the replacement of conditional if it can be done.  */
+      if (!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))
+	cfgchanged = true;
+      else if (!early_p
+	       && !diamond_p
+	       && single_pred_p (bb1)
+	       && cond_removal_in_builtin_zero_pattern (bb, bb1, e1, e2,
+							phi, arg0, arg1))
+	cfgchanged = true;
+      else if (minmax_replacement (bb, bb1, bb2, e1, e2, phi, arg0, arg1,
+				   diamond_p))
+	cfgchanged = true;
+      else if (single_pred_p (bb1)
+	       && !diamond_p
+	       && spaceship_replacement (bb, bb1, e1, e2, phi, arg0, arg1))
+	cfgchanged = true;
     }
 
   free (bb_order);
-- 
2.39.1


  parent reply	other threads:[~2023-04-22 22:09 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-04-22 22:09 [PATCH 0/6] Improve PHIOPT match and simplify for diamond shaped bbs Andrew Pinski
2023-04-22 22:09 ` [PATCH 1/6] PHIOPT: Move check on diamond bb to tree_ssa_phiopt_worker from minmax_replacement Andrew Pinski
2023-04-24 12:01   ` Richard Biener
2023-04-22 22:09 ` Andrew Pinski [this message]
2023-04-24 12:06   ` [PATCH 2/6] PHIOPT: Cleanup tree_ssa_phiopt_worker code Richard Biener
2023-04-22 22:09 ` [PATCH 3/6] PHIOPT: Allow other diamond uses when do_hoist_loads is true Andrew Pinski
2023-04-24 12:06   ` Richard Biener
2023-04-22 22:09 ` [PATCH 4/6] PHIOPT: Factor out some code from match_simplify_replacement Andrew Pinski
2023-04-24 12:02   ` Richard Biener
2023-04-22 22:09 ` [PATCH 5/6] PHIOPT: Ignore predicates for match-and-simplify phi-opt Andrew Pinski
2023-04-24 12:05   ` Richard Biener
2023-04-22 22:09 ` [PATCH 6/6] PHIOPT: Add support for diamond shaped bb to match_simplify_replacement Andrew Pinski
2023-04-24 12:07   ` Richard Biener

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20230422220921.452264-3-apinski@marvell.com \
    --to=apinski@marvell.com \
    --cc=gcc-patches@gcc.gnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).