[PATCH 0/4] Fix PR tree-opt/102703

[PATCH 0/4] Fix PR tree-opt/102703

[apinski]

From: Andrew Pinski <apinski@marvell.com>

This patch series fixes PR tree-opt/102703 by
improving the code which will delete write only stores to also
delete the phi node (if it was a phi node) that was used to define
the write.
We need to some factoring out of the code to make it easier
to understand and less indention.

Andrew Pinski (4):
  Add dump prints when execute_fixup_cfg removes a write only var store.
  Remove outdated comment about execute_fixup_cfg
  Factor out removal of write only stores from execute_fixup_cfg
  Improve maybe_remove_writeonly_store to do a simple DCE for defining
    statement

 gcc/tree-cfg.c | 95 ++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 73 insertions(+), 22 deletions(-)

-- 
2.17.1

[PATCH 1/4] Add dump prints when execute_fixup_cfg removes a write only var store.

[apinski]

From: Andrew Pinski <apinski@marvell.com>

While debugging PR 102703, I found it was hard to figure out where
the store was being removed as there was no pass which was outputting
why the store was removed.
This adds to execute_fixup_cfg the output.
Also note most of removals happen when execute_fixup_cfg is called
from the inliner.

gcc/ChangeLog:

	* tree-cfg.c (execute_fixup_cfg): Output when the statement
	is removed when it is a write only var.
---
 gcc/tree-cfg.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c
index 4b4b0b52d9a..b78e4564e4d 100644
--- a/gcc/tree-cfg.c
+++ b/gcc/tree-cfg.c
@@ -9737,6 +9737,13 @@ execute_fixup_cfg (void)
 		  && (TREE_STATIC (lhs) || DECL_EXTERNAL (lhs))
 		  && varpool_node::get (lhs)->writeonly)
 		{
+		  if (dump_file && (dump_flags & TDF_DETAILS))
+		    {
+		      fprintf (dump_file, "Removing statement, writes"
+		               " to write only var:\n");
+		      print_gimple_stmt (dump_file, stmt, 0,
+					 TDF_VOPS|TDF_MEMSYMS);
+		    }
 		  unlink_stmt_vdef (stmt);
 		  gsi_remove (&gsi, true);
 		  release_defs (stmt);
-- 
2.17.1

[PATCH 2/4] Remove outdated comment about execute_fixup_cfg

[apinski]

From: Andrew Pinski <apinski@marvell.com>

The comment about execute_fixup_cfg not being able to
run as a standalone pass is not true for a long time
now.  It has been a standalone pass for a while now.

gcc/ChangeLog:

	* tree-cfg.c (execute_fixup_cfg): Remove comment
	about standalone pass.
---
 gcc/tree-cfg.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c
index b78e4564e4d..c20fc4980c6 100644
--- a/gcc/tree-cfg.c
+++ b/gcc/tree-cfg.c
@@ -9669,10 +9669,7 @@ make_pass_warn_unused_result (gcc::context *ctxt)
 /* IPA passes, compilation of earlier functions or inlining
    might have changed some properties, such as marked functions nothrow,
    pure, const or noreturn.
-   Remove redundant edges and basic blocks, and create new ones if necessary.
-
-   This pass can't be executed as stand alone pass from pass manager, because
-   in between inlining and this fixup the verify_flow_info would fail.  */
+   Remove redundant edges and basic blocks, and create new ones if necessary. */
 
 unsigned int
 execute_fixup_cfg (void)
-- 
2.17.1

[PATCH 3/4] Factor out removal of write only stores from execute_fixup_cfg

[apinski]

From: Andrew Pinski <apinski@marvell.com>

To make it easier to fix PR 102703, factoring this code out
to its own function makes it easier to read and less indentions
too.

gcc/ChangeLog:

	* tree-cfg.c (maybe_remove_writeonly_store): New function
	factored out from ...
	(execute_fixup_cfg): Here. Call maybe_remove_writeonly_store.
---
 gcc/tree-cfg.c | 62 ++++++++++++++++++++++++++++++--------------------
 1 file changed, 37 insertions(+), 25 deletions(-)

diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c
index c20fc4980c6..dbbf6beb6e4 100644
--- a/gcc/tree-cfg.c
+++ b/gcc/tree-cfg.c
@@ -9666,6 +9666,38 @@ make_pass_warn_unused_result (gcc::context *ctxt)
   return new pass_warn_unused_result (ctxt);
 }
 
+/* Maybe Remove stores to variables we marked write-only.
+   Return true if a store was removed. */
+static bool
+maybe_remove_writeonly_store (gimple_stmt_iterator &gsi, gimple *stmt)
+{
+  /* Keep access when store has side effect, i.e. in case when source
+     is volatile.  */  
+  if (!gimple_store_p (stmt)
+      || gimple_has_side_effects (stmt)
+      || optimize_debug)
+    return false;
+
+  tree lhs = get_base_address (gimple_get_lhs (stmt));
+
+  if (!VAR_P (lhs)
+      || (!TREE_STATIC (lhs) && !DECL_EXTERNAL (lhs))
+      || !varpool_node::get (lhs)->writeonly)
+    return false;
+
+  if (dump_file && (dump_flags & TDF_DETAILS))
+    {
+      fprintf (dump_file, "Removing statement, writes"
+	       " to write only var:\n");
+      print_gimple_stmt (dump_file, stmt, 0,
+			 TDF_VOPS|TDF_MEMSYMS);
+    }
+  unlink_stmt_vdef (stmt);
+  gsi_remove (&gsi, true);
+  release_defs (stmt);
+  return true;
+}
+
 /* IPA passes, compilation of earlier functions or inlining
    might have changed some properties, such as marked functions nothrow,
    pure, const or noreturn.
@@ -9721,33 +9753,13 @@ execute_fixup_cfg (void)
 		todo |= TODO_cleanup_cfg;
 	     }
 
-	  /* Remove stores to variables we marked write-only.
-	     Keep access when store has side effect, i.e. in case when source
-	     is volatile.  */
-	  if (gimple_store_p (stmt)
-	      && !gimple_has_side_effects (stmt)
-	      && !optimize_debug)
+	  /* Remove stores to variables we marked write-only. */
+	  if (maybe_remove_writeonly_store (gsi, stmt))
 	    {
-	      tree lhs = get_base_address (gimple_get_lhs (stmt));
-
-	      if (VAR_P (lhs)
-		  && (TREE_STATIC (lhs) || DECL_EXTERNAL (lhs))
-		  && varpool_node::get (lhs)->writeonly)
-		{
-		  if (dump_file && (dump_flags & TDF_DETAILS))
-		    {
-		      fprintf (dump_file, "Removing statement, writes"
-		               " to write only var:\n");
-		      print_gimple_stmt (dump_file, stmt, 0,
-					 TDF_VOPS|TDF_MEMSYMS);
-		    }
-		  unlink_stmt_vdef (stmt);
-		  gsi_remove (&gsi, true);
-		  release_defs (stmt);
-	          todo |= TODO_update_ssa | TODO_cleanup_cfg;
-	          continue;
-		}
+	      todo |= TODO_update_ssa | TODO_cleanup_cfg;
+	      continue;
 	    }
+
 	  /* For calls we can simply remove LHS when it is known
 	     to be write-only.  */
 	  if (is_gimple_call (stmt)
-- 
2.17.1

[PATCH 4/4] Improve maybe_remove_writeonly_store to do a simple DCE for defining statement

[apinski]

From: Andrew Pinski <apinski@marvell.com>

Instead of putting a full blow DCE after execute_fixup_cfg, it makes sense
to try to remove the defining statement for the store that is being removed.
Right now we only handle PHI node statements as there needs no extra checks
except for it is only used once in the store statement.

gcc/ChangeLog:

	* tree-cfg.c (maybe_remove_writeonly_store): Remove defining
	(PHI) statement of the store if possible.
---
 gcc/tree-cfg.c | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c
index dbbf6beb6e4..d9efdc220ca 100644
--- a/gcc/tree-cfg.c
+++ b/gcc/tree-cfg.c
@@ -9692,6 +9692,41 @@ maybe_remove_writeonly_store (gimple_stmt_iterator &gsi, gimple *stmt)
       print_gimple_stmt (dump_file, stmt, 0,
 			 TDF_VOPS|TDF_MEMSYMS);
     }
+
+  /* Remove the statement defining the rhs if it was only
+     used by this statement. */
+  if (gimple_assign_single_p (stmt))
+    {
+      tree rhs = gimple_assign_rhs1 (stmt);
+      gimple *use_stmt;
+      use_operand_p use_p;
+      gimple *stmt1;
+
+
+      if (TREE_CODE (rhs) == SSA_NAME
+	  && single_imm_use (rhs, &use_p, &use_stmt)
+	  && (stmt1 = SSA_NAME_DEF_STMT (rhs))
+	  /* For now only handle PHI nodes.
+	     FIXME: this should handle more. */
+	  && gimple_code (stmt1) == GIMPLE_PHI)
+	{
+	  if (dump_file && (dump_flags & TDF_DETAILS))
+	    {
+	      fprintf (dump_file, "Removing defining statement:\n");
+	      print_gimple_stmt (dump_file, stmt1, 0,
+				 TDF_VOPS|TDF_MEMSYMS);
+	    }
+	  gimple_stmt_iterator gsi_for_def;
+	  gsi_for_def = gsi_for_stmt (stmt1);
+	  if (gimple_code (stmt1) == GIMPLE_PHI)
+	    remove_phi_node (&gsi_for_def, true);
+	  else
+	    {
+	      gsi_remove (&gsi_for_def, true);
+	      release_defs (stmt1);
+	    }
+	}
+    }
   unlink_stmt_vdef (stmt);
   gsi_remove (&gsi, true);
   release_defs (stmt);
-- 
2.17.1

Re: [PATCH 1/4] Add dump prints when execute_fixup_cfg removes a write only var store.

[Jeff Law]

On 10/18/2021 10:54 PM, apinski--- via Gcc-patches wrote:
> From: Andrew Pinski <apinski@marvell.com>
>
> While debugging PR 102703, I found it was hard to figure out where
> the store was being removed as there was no pass which was outputting
> why the store was removed.
> This adds to execute_fixup_cfg the output.
> Also note most of removals happen when execute_fixup_cfg is called
> from the inliner.
>
> gcc/ChangeLog:
>
> 	* tree-cfg.c (execute_fixup_cfg): Output when the statement
> 	is removed when it is a write only var.
OK
jeff

Re: [PATCH 2/4] Remove outdated comment about execute_fixup_cfg

[Jeff Law]

On 10/18/2021 10:54 PM, apinski--- via Gcc-patches wrote:
> From: Andrew Pinski <apinski@marvell.com>
>
> The comment about execute_fixup_cfg not being able to
> run as a standalone pass is not true for a long time
> now.  It has been a standalone pass for a while now.
>
> gcc/ChangeLog:
>
> 	* tree-cfg.c (execute_fixup_cfg): Remove comment
> 	about standalone pass.
OK
jeff

Re: [PATCH 3/4] Factor out removal of write only stores from execute_fixup_cfg

[Jeff Law]

On 10/18/2021 10:54 PM, apinski--- via Gcc-patches wrote:
> From: Andrew Pinski <apinski@marvell.com>
>
> To make it easier to fix PR 102703, factoring this code out
> to its own function makes it easier to read and less indentions
> too.
>
> gcc/ChangeLog:
>
> 	* tree-cfg.c (maybe_remove_writeonly_store): New function
> 	factored out from ...
> 	(execute_fixup_cfg): Here. Call maybe_remove_writeonly_store.
OK
jeff

Re: [PATCH 4/4] Improve maybe_remove_writeonly_store to do a simple DCE for defining statement

[Jeff Law]

On 10/18/2021 10:54 PM, apinski--- via Gcc-patches wrote:
> From: Andrew Pinski <apinski@marvell.com>
>
> Instead of putting a full blow DCE after execute_fixup_cfg, it makes sense
> to try to remove the defining statement for the store that is being removed.
> Right now we only handle PHI node statements as there needs no extra checks
> except for it is only used once in the store statement.
>
> gcc/ChangeLog:
>
> 	* tree-cfg.c (maybe_remove_writeonly_store): Remove defining
> 	(PHI) statement of the store if possible.
This is the only part that I consider at all controversial.

Is the case you're trying to handle such that you have to eliminate the 
PHI immediately and can't wait until the next DCE pass?

If so and we want to go this direction, should we pull this out into a 
little routine?   I'm a bit surprised we don't already have one or more 
that do basically the same thing.

Jeff

Re: [PATCH 4/4] Improve maybe_remove_writeonly_store to do a simple DCE for defining statement

[Richard Biener]

On Wed, Oct 20, 2021 at 1:14 AM Jeff Law via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
>
>
> On 10/18/2021 10:54 PM, apinski--- via Gcc-patches wrote:
> > From: Andrew Pinski <apinski@marvell.com>
> >
> > Instead of putting a full blow DCE after execute_fixup_cfg, it makes sense
> > to try to remove the defining statement for the store that is being removed.
> > Right now we only handle PHI node statements as there needs no extra checks
> > except for it is only used once in the store statement.
> >
> > gcc/ChangeLog:
> >
> >       * tree-cfg.c (maybe_remove_writeonly_store): Remove defining
> >       (PHI) statement of the store if possible.
> This is the only part that I consider at all controversial.
>
> Is the case you're trying to handle such that you have to eliminate the
> PHI immediately and can't wait until the next DCE pass?
>
> If so and we want to go this direction, should we pull this out into a
> little routine?   I'm a bit surprised we don't already have one or more
> that do basically the same thing.

We have simple_dce_from_worklist for this which you'd seed with
the SSA rhs of the removed stores.

Richard.

>
> Jeff
>

Re: [PATCH 4/4] Improve maybe_remove_writeonly_store to do a simple DCE for defining statement

[Jeff Law]

On 10/20/2021 12:58 AM, Richard Biener wrote:
> On Wed, Oct 20, 2021 at 1:14 AM Jeff Law via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
>>
>>
>> On 10/18/2021 10:54 PM, apinski--- via Gcc-patches wrote:
>>> From: Andrew Pinski <apinski@marvell.com>
>>>
>>> Instead of putting a full blow DCE after execute_fixup_cfg, it makes sense
>>> to try to remove the defining statement for the store that is being removed.
>>> Right now we only handle PHI node statements as there needs no extra checks
>>> except for it is only used once in the store statement.
>>>
>>> gcc/ChangeLog:
>>>
>>>        * tree-cfg.c (maybe_remove_writeonly_store): Remove defining
>>>        (PHI) statement of the store if possible.
>> This is the only part that I consider at all controversial.
>>
>> Is the case you're trying to handle such that you have to eliminate the
>> PHI immediately and can't wait until the next DCE pass?
>>
>> If so and we want to go this direction, should we pull this out into a
>> little routine?   I'm a bit surprised we don't already have one or more
>> that do basically the same thing.
> We have simple_dce_from_worklist for this which you'd seed with
> the SSA rhs of the removed stores.
Yea, that seems like a better routine to use.  Andrew, can you try that?
Jeff

Re: [PATCH 4/4] Improve maybe_remove_writeonly_store to do a simple DCE for defining statement

[Andrew Pinski]

On Wed, Oct 20, 2021 at 12:54 PM Jeff Law via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
>
>
> On 10/20/2021 12:58 AM, Richard Biener wrote:
> > On Wed, Oct 20, 2021 at 1:14 AM Jeff Law via Gcc-patches
> > <gcc-patches@gcc.gnu.org> wrote:
> >>
> >>
> >> On 10/18/2021 10:54 PM, apinski--- via Gcc-patches wrote:
> >>> From: Andrew Pinski <apinski@marvell.com>
> >>>
> >>> Instead of putting a full blow DCE after execute_fixup_cfg, it makes sense
> >>> to try to remove the defining statement for the store that is being removed.
> >>> Right now we only handle PHI node statements as there needs no extra checks
> >>> except for it is only used once in the store statement.
> >>>
> >>> gcc/ChangeLog:
> >>>
> >>>        * tree-cfg.c (maybe_remove_writeonly_store): Remove defining
> >>>        (PHI) statement of the store if possible.
> >> This is the only part that I consider at all controversial.
> >>
> >> Is the case you're trying to handle such that you have to eliminate the
> >> PHI immediately and can't wait until the next DCE pass?
> >>
> >> If so and we want to go this direction, should we pull this out into a
> >> little routine?   I'm a bit surprised we don't already have one or more
> >> that do basically the same thing.
> > We have simple_dce_from_worklist for this which you'd seed with
> > the SSA rhs of the removed stores.
> Yea, that seems like a better routine to use.  Andrew, can you try that?

Yes that is a better routine to use, the patch is in testing right
now.  I should be able to submit it in a few hours.

Thanks,
Andrew

> Jeff