[4/4] Improve maybe_remove_writeonly_store to do a simple DCE for defining statement
Commit Message
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(+)
Comments
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
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
>
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
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
@@ -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);