* [PATCH 1/4] Add dump prints when execute_fixup_cfg removes a write only var store.
2021-10-19 4:54 [PATCH 0/4] Fix PR tree-opt/102703 apinski
@ 2021-10-19 4:54 ` apinski
2021-10-19 23:10 ` Jeff Law
2021-10-19 4:54 ` [PATCH 2/4] Remove outdated comment about execute_fixup_cfg apinski
` (2 subsequent siblings)
3 siblings, 1 reply; 12+ messages in thread
From: apinski @ 2021-10-19 4:54 UTC (permalink / raw)
To: gcc-patches; +Cc: Andrew Pinski
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
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH 1/4] Add dump prints when execute_fixup_cfg removes a write only var store.
2021-10-19 4:54 ` [PATCH 1/4] Add dump prints when execute_fixup_cfg removes a write only var store apinski
@ 2021-10-19 23:10 ` Jeff Law
0 siblings, 0 replies; 12+ messages in thread
From: Jeff Law @ 2021-10-19 23:10 UTC (permalink / raw)
To: apinski, gcc-patches
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 2/4] Remove outdated comment about execute_fixup_cfg
2021-10-19 4:54 [PATCH 0/4] Fix PR tree-opt/102703 apinski
2021-10-19 4:54 ` [PATCH 1/4] Add dump prints when execute_fixup_cfg removes a write only var store apinski
@ 2021-10-19 4:54 ` apinski
2021-10-19 23:10 ` Jeff Law
2021-10-19 4:54 ` [PATCH 3/4] Factor out removal of write only stores from execute_fixup_cfg apinski
2021-10-19 4:54 ` [PATCH 4/4] Improve maybe_remove_writeonly_store to do a simple DCE for defining statement apinski
3 siblings, 1 reply; 12+ messages in thread
From: apinski @ 2021-10-19 4:54 UTC (permalink / raw)
To: gcc-patches; +Cc: Andrew Pinski
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
^ permalink raw reply [flat|nested] 12+ messages in thread* [PATCH 3/4] Factor out removal of write only stores from execute_fixup_cfg
2021-10-19 4:54 [PATCH 0/4] Fix PR tree-opt/102703 apinski
2021-10-19 4:54 ` [PATCH 1/4] Add dump prints when execute_fixup_cfg removes a write only var store apinski
2021-10-19 4:54 ` [PATCH 2/4] Remove outdated comment about execute_fixup_cfg apinski
@ 2021-10-19 4:54 ` apinski
2021-10-19 23:11 ` Jeff Law
2021-10-19 4:54 ` [PATCH 4/4] Improve maybe_remove_writeonly_store to do a simple DCE for defining statement apinski
3 siblings, 1 reply; 12+ messages in thread
From: apinski @ 2021-10-19 4:54 UTC (permalink / raw)
To: gcc-patches; +Cc: Andrew Pinski
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
^ permalink raw reply [flat|nested] 12+ messages in thread* [PATCH 4/4] Improve maybe_remove_writeonly_store to do a simple DCE for defining statement
2021-10-19 4:54 [PATCH 0/4] Fix PR tree-opt/102703 apinski
` (2 preceding siblings ...)
2021-10-19 4:54 ` [PATCH 3/4] Factor out removal of write only stores from execute_fixup_cfg apinski
@ 2021-10-19 4:54 ` apinski
2021-10-19 23:13 ` Jeff Law
3 siblings, 1 reply; 12+ messages in thread
From: apinski @ 2021-10-19 4:54 UTC (permalink / raw)
To: gcc-patches; +Cc: Andrew Pinski
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
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH 4/4] Improve maybe_remove_writeonly_store to do a simple DCE for defining statement
2021-10-19 4:54 ` [PATCH 4/4] Improve maybe_remove_writeonly_store to do a simple DCE for defining statement apinski
@ 2021-10-19 23:13 ` Jeff Law
2021-10-20 6:58 ` Richard Biener
0 siblings, 1 reply; 12+ messages in thread
From: Jeff Law @ 2021-10-19 23:13 UTC (permalink / raw)
To: apinski, gcc-patches
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 4/4] Improve maybe_remove_writeonly_store to do a simple DCE for defining statement
2021-10-19 23:13 ` Jeff Law
@ 2021-10-20 6:58 ` Richard Biener
2021-10-20 19:53 ` Jeff Law
0 siblings, 1 reply; 12+ messages in thread
From: Richard Biener @ 2021-10-20 6:58 UTC (permalink / raw)
To: Jeff Law; +Cc: Andrew Pinski, GCC Patches
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
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 4/4] Improve maybe_remove_writeonly_store to do a simple DCE for defining statement
2021-10-20 6:58 ` Richard Biener
@ 2021-10-20 19:53 ` Jeff Law
2021-10-20 19:57 ` Andrew Pinski
0 siblings, 1 reply; 12+ messages in thread
From: Jeff Law @ 2021-10-20 19:53 UTC (permalink / raw)
To: Richard Biener; +Cc: Andrew Pinski, GCC Patches
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 4/4] Improve maybe_remove_writeonly_store to do a simple DCE for defining statement
2021-10-20 19:53 ` Jeff Law
@ 2021-10-20 19:57 ` Andrew Pinski
0 siblings, 0 replies; 12+ messages in thread
From: Andrew Pinski @ 2021-10-20 19:57 UTC (permalink / raw)
To: Jeff Law; +Cc: Richard Biener, Andrew Pinski, GCC Patches
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
^ permalink raw reply [flat|nested] 12+ messages in thread