Re: [PATCH 1.0/2] ipa-sra: Introduce a mini-DCE to tree-inline.c (PR 93385)
Hi,
On Tue, May 11 2021, Richard Biener wrote:
> On Mon, May 10, 2021 at 8:52 PM Martin Jambor wrote:
[...]
>> @@ -969,6 +969,97 @@ ipa_param_body_adjustments::carry_over_param (tree t)
>>return new_parm;
>> }
>>
>> +/* Return true if BLOCKS_TO_COPY is NULL or if PHI has an argument ARG in
>> + position that corresponds to an edge that is coming from a block that has
>> + the corresponding bit set in BLOCKS_TO_COPY. */
>> +
>> +static bool
>> +phi_arg_will_live_p (gphi *phi, bitmap blocks_to_copy, tree arg)
>> +{
>> + bool arg_will_survive = false;
>> + if (!blocks_to_copy)
>> +arg_will_survive = true;
>> + else
>> +for (unsigned i = 0; i < gimple_phi_num_args (phi); i++)
>> + if (gimple_phi_arg_def (phi, i) == arg
>
> I think this is prone to quadratic, it would be nice to use the
> faster FOR_EACH_IMM_USE_FAST () below and then
> phi_arg_index_from_use () to get to the corresponding edge
> directly - this would remove the loop over PHI args here and
> you can then inline the function below.
I see, thanks, I have changed the code as suggested.
>
>> + && bitmap_bit_p (blocks_to_copy,
>> + gimple_phi_arg_edge (phi, i)->src->index))
>> + {
>> + arg_will_survive = true;
>> + break;
>> + }
>> + return arg_will_survive;
>> +}
>> +
>> +/* Populate m_dead_stmts given that DEAD_PARAM is going to be removed
>> without
>> + any replacement or splitting. REPL is the replacement VAR_SECL to base
>> any
>> + remaining uses of a removed parameter on. */
>> +
>> +void
>> +ipa_param_body_adjustments::mark_dead_statements (tree dead_param)
>> +{
>> + /* Current IPA analyses which remove unused parameters never remove a
>> + non-gimple register ones which have any use except as parameters in
>> other
>> + calls, so we can safely leve them as they are. */
>> + if (!is_gimple_reg (dead_param))
>> +return;
>> + tree parm_ddef = ssa_default_def (m_id->src_cfun, dead_param);
>> + if (!parm_ddef || has_zero_uses (parm_ddef))
>> +return;
>> +
>> + auto_vec stack;
>> + m_dead_ssas.add (parm_ddef);
>> + stack.safe_push (parm_ddef);
>> + while (!stack.is_empty ())
>> +{
>> + tree t = stack.pop ();
>> +
>> + imm_use_iterator imm_iter;
>> + gimple *stmt;
>> +
>> + insert_decl_map (m_id, t, error_mark_node);
>> + FOR_EACH_IMM_USE_STMT (stmt, imm_iter, t)
>> + {
>> + if (is_gimple_call (stmt)
>
> (*) so we just ignore dead calls? That is, there might be
> a const call like
>
>_1 = foo (removed_param_2);
>
> and followup important to be removed uses of _1? Above
> you say that eventual IPA SRA argument removal is dealt
> with but this still leaves the const/pure call case. Unless
> IPA SRA bails on them, of course.
It does, dealing with pure/const functions is only a TODO. This code is
basically only meant to understand what isra_track_scalar_value_uses
can. I was looking at somehow merging them but introducing
callbacks/specialization would make it too complex and ugly for little
gain.
> Can you add a comment like
>
> /* IPA SRA only handles dead arguments in calls when it
>removes those arguments from the called function. The
>IPA machinery arranges for fixup here. */
I added the following comment which I think better describes the
situation:
/* Calls containing dead arguments cannot be deleted,
modify_call_stmt will instead remove just the argument later on.
If isra_track_scalar_value_uses in ipa-sra.c is extended to look
through const functions, we will need to do so here too. */
because arguments are indeed "usually" removed during the final fixup of
calls but this case is actually an exception from that "rule."
>
>> + || (m_id->blocks_to_copy
>> + && !bitmap_bit_p (m_id->blocks_to_copy,
>> + gimple_bb (stmt)->index)))
>> + continue;
>> +
>> + if (is_gimple_debug (stmt))
>> + {
>> + m_dead_stmts.add (stmt);
>> + gcc_assert (gimple_debug_bind_p (stmt));
>> + }
>> + else if (gimple_code (stmt) == GIMPLE_PHI)
>> + {
>> + gphi *phi = as_a (stmt);
>> + if (phi_arg_will_live_p (phi, m_id->blocks_to_copy, t))
>> + {
>> + m_dead_stmts.add (phi);
>> + tree res = gimple_phi_result (phi);
>> + if (!m_dead_ssas.add (res))
>> + stack.safe_push (res);
>> + }
>> + }
>> + else if (is_gimple_assign (stmt))
>> + {
>> + m_dead_stmts.add (stmt);
>> + if (!gimple_clobber_p (stmt))
>> + {
>> + tree lhs = gimple_assign_lhs (stmt);
>> + gcc_assert (TREE_CODE (lhs) == SSA_NAME);
>
> I don't think you can assert this? Well, maybe you can because
> IPA prop would have refused to mark the param dead?
Re: [PATCH 1.0/2] ipa-sra: Introduce a mini-DCE to tree-inline.c (PR 93385)
On Mon, May 10, 2021 at 8:52 PM Martin Jambor wrote:
>
> Hi,
>
> On Mon, May 10 2021, Richard Biener wrote:
> > I've tried to have a look at this patch but it does a lot of IPA specific
> > refactoring(?), so the actual DCE bits are hard to find. Is it possible
> > to split the patch up or is it too entangled?
> >
>
> Yes:
>
> I was asked by Richi to split my fix for PR 93385 for easier review
> into IPA-SRA materialization refactoring and the actual DCE addition.
> This is the second part that actually contains the DCE of statements
> that IPA-SRA should not leave behind because they can have problematic
> side effects, even if they are useless, so that we do not depend on
> tree-dce to remove them for correctness.
>
> The patch fixes the problem by doing a def-use walk when materializing
> clones, marking which statements should not be copied and which
> SSA_NAMEs do not need to be computed because eventually they would be
> DCEd. We do this on the original function body and tree-inline simply
> does not copy statements which are "dead."
>
> The only complication is removing dead argument calls because that
> needs to be communicated to callee redirection code using the
> infrastructure introduced by the previous patch.
>
> I added all testcases of the original patch to this one, although some
> probably test behavior introduced in the previous patch.
>
> The patch is so far only lightly tested but I have verified that
> together with the second one they make up pretty much exactly the
> original one (modulo m_new_call_arg_modification_info) which I did
> bootstrap this morning. I will of course bootstrap it independently
> too.
>
> What do you think?
>
> Martin
>
>
> gcc/ChangeLog:
>
> 2021-05-10 Martin Jambor
>
> PR ipa/93385
> * ipa-param-manipulation.h (class ipa_param_body_adjustments): New
> members m_dead_stmts and m_dead_ssas.
> * ipa-param-manipulation.c (phi_arg_will_live_p): New function.
> (ipa_param_body_adjustments::mark_dead_statements): Likwise.
> (ipa_param_body_adjustments::common_initialization): Call it on
> all removed but not split parameters.
> (ipa_param_body_adjustments::ipa_param_body_adjustments): Initialize
> new mwmbers.
> (ipa_param_body_adjustments::modify_call_stmt): Remove arguments that
> are dead.
> * tree-inline.c (remap_gimple_stmt): Do not copy dead statements,
> reset
> dead debug statements.
> (copy_phis_for_bb): Do not copy dead PHI nodes.
>
> gcc/testsuite/ChangeLog:
>
> 2021-03-22 Martin Jambor
>
> PR ipa/93385
> * gcc.dg/ipa/pr93385.c: New test.
> * gcc.dg/ipa/ipa-sra-23.c: Likewise.
> * gcc.dg/ipa/ipa-sra-24.c: Likewise.
> * g++.dg/ipa/ipa-sra-4.C: Likewise.
> ---
> gcc/ipa-param-manipulation.c | 142 +++---
> gcc/ipa-param-manipulation.h | 6 ++
> gcc/testsuite/g++.dg/ipa/ipa-sra-4.C | 37 +++
> gcc/testsuite/gcc.dg/ipa/ipa-sra-23.c | 24 +
> gcc/testsuite/gcc.dg/ipa/ipa-sra-24.c | 20
> gcc/testsuite/gcc.dg/ipa/pr93385.c| 27 +
> gcc/tree-inline.c | 18 +++-
> 7 files changed, 256 insertions(+), 18 deletions(-)
> create mode 100644 gcc/testsuite/g++.dg/ipa/ipa-sra-4.C
> create mode 100644 gcc/testsuite/gcc.dg/ipa/ipa-sra-23.c
> create mode 100644 gcc/testsuite/gcc.dg/ipa/ipa-sra-24.c
> create mode 100644 gcc/testsuite/gcc.dg/ipa/pr93385.c
>
> diff --git a/gcc/ipa-param-manipulation.c b/gcc/ipa-param-manipulation.c
> index 424b8e5343f..d7d73542856 100644
> --- a/gcc/ipa-param-manipulation.c
> +++ b/gcc/ipa-param-manipulation.c
> @@ -969,6 +969,97 @@ ipa_param_body_adjustments::carry_over_param (tree t)
>return new_parm;
> }
>
> +/* Return true if BLOCKS_TO_COPY is NULL or if PHI has an argument ARG in
> + position that corresponds to an edge that is coming from a block that has
> + the corresponding bit set in BLOCKS_TO_COPY. */
> +
> +static bool
> +phi_arg_will_live_p (gphi *phi, bitmap blocks_to_copy, tree arg)
> +{
> + bool arg_will_survive = false;
> + if (!blocks_to_copy)
> +arg_will_survive = true;
> + else
> +for (unsigned i = 0; i < gimple_phi_num_args (phi); i++)
> + if (gimple_phi_arg_def (phi, i) == arg
I think this is prone to quadratic, it would be nice to use the
faster FOR_EACH_IMM_USE_FAST () below and then
phi_arg_index_from_use () to get to the corresponding edge
directly - this would remove the loop over PHI args here and
you can then inline the function below.
> + && bitmap_bit_p (blocks_to_copy,
> + gimple_phi_arg_edge (phi, i)->src->index))
> + {
> + arg_will_survive = true;
> + break;
> + }
> + return arg_will_survive;
> +}
> +
> +/* Populate m_dead_stmts given that DEAD_PARAM is going to be removed without
> + any replacement or splitting. REPL is the replacement VAR_SECL to base
> any
> +
Re: [PATCH 1.0/2] ipa-sra: Introduce a mini-DCE to tree-inline.c (PR 93385)
Hi,
On Mon, May 10 2021, Richard Biener wrote:
> I've tried to have a look at this patch but it does a lot of IPA specific
> refactoring(?), so the actual DCE bits are hard to find. Is it possible
> to split the patch up or is it too entangled?
>
Yes:
I was asked by Richi to split my fix for PR 93385 for easier review
into IPA-SRA materialization refactoring and the actual DCE addition.
This is the second part that actually contains the DCE of statements
that IPA-SRA should not leave behind because they can have problematic
side effects, even if they are useless, so that we do not depend on
tree-dce to remove them for correctness.
The patch fixes the problem by doing a def-use walk when materializing
clones, marking which statements should not be copied and which
SSA_NAMEs do not need to be computed because eventually they would be
DCEd. We do this on the original function body and tree-inline simply
does not copy statements which are "dead."
The only complication is removing dead argument calls because that
needs to be communicated to callee redirection code using the
infrastructure introduced by the previous patch.
I added all testcases of the original patch to this one, although some
probably test behavior introduced in the previous patch.
The patch is so far only lightly tested but I have verified that
together with the second one they make up pretty much exactly the
original one (modulo m_new_call_arg_modification_info) which I did
bootstrap this morning. I will of course bootstrap it independently
too.
What do you think?
Martin
gcc/ChangeLog:
2021-05-10 Martin Jambor
PR ipa/93385
* ipa-param-manipulation.h (class ipa_param_body_adjustments): New
members m_dead_stmts and m_dead_ssas.
* ipa-param-manipulation.c (phi_arg_will_live_p): New function.
(ipa_param_body_adjustments::mark_dead_statements): Likwise.
(ipa_param_body_adjustments::common_initialization): Call it on
all removed but not split parameters.
(ipa_param_body_adjustments::ipa_param_body_adjustments): Initialize
new mwmbers.
(ipa_param_body_adjustments::modify_call_stmt): Remove arguments that
are dead.
* tree-inline.c (remap_gimple_stmt): Do not copy dead statements, reset
dead debug statements.
(copy_phis_for_bb): Do not copy dead PHI nodes.
gcc/testsuite/ChangeLog:
2021-03-22 Martin Jambor
PR ipa/93385
* gcc.dg/ipa/pr93385.c: New test.
* gcc.dg/ipa/ipa-sra-23.c: Likewise.
* gcc.dg/ipa/ipa-sra-24.c: Likewise.
* g++.dg/ipa/ipa-sra-4.C: Likewise.
---
gcc/ipa-param-manipulation.c | 142 +++---
gcc/ipa-param-manipulation.h | 6 ++
gcc/testsuite/g++.dg/ipa/ipa-sra-4.C | 37 +++
gcc/testsuite/gcc.dg/ipa/ipa-sra-23.c | 24 +
gcc/testsuite/gcc.dg/ipa/ipa-sra-24.c | 20
gcc/testsuite/gcc.dg/ipa/pr93385.c| 27 +
gcc/tree-inline.c | 18 +++-
7 files changed, 256 insertions(+), 18 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/ipa/ipa-sra-4.C
create mode 100644 gcc/testsuite/gcc.dg/ipa/ipa-sra-23.c
create mode 100644 gcc/testsuite/gcc.dg/ipa/ipa-sra-24.c
create mode 100644 gcc/testsuite/gcc.dg/ipa/pr93385.c
diff --git a/gcc/ipa-param-manipulation.c b/gcc/ipa-param-manipulation.c
index 424b8e5343f..d7d73542856 100644
--- a/gcc/ipa-param-manipulation.c
+++ b/gcc/ipa-param-manipulation.c
@@ -969,6 +969,97 @@ ipa_param_body_adjustments::carry_over_param (tree t)
return new_parm;
}
+/* Return true if BLOCKS_TO_COPY is NULL or if PHI has an argument ARG in
+ position that corresponds to an edge that is coming from a block that has
+ the corresponding bit set in BLOCKS_TO_COPY. */
+
+static bool
+phi_arg_will_live_p (gphi *phi, bitmap blocks_to_copy, tree arg)
+{
+ bool arg_will_survive = false;
+ if (!blocks_to_copy)
+arg_will_survive = true;
+ else
+for (unsigned i = 0; i < gimple_phi_num_args (phi); i++)
+ if (gimple_phi_arg_def (phi, i) == arg
+ && bitmap_bit_p (blocks_to_copy,
+ gimple_phi_arg_edge (phi, i)->src->index))
+ {
+ arg_will_survive = true;
+ break;
+ }
+ return arg_will_survive;
+}
+
+/* Populate m_dead_stmts given that DEAD_PARAM is going to be removed without
+ any replacement or splitting. REPL is the replacement VAR_SECL to base any
+ remaining uses of a removed parameter on. */
+
+void
+ipa_param_body_adjustments::mark_dead_statements (tree dead_param)
+{
+ /* Current IPA analyses which remove unused parameters never remove a
+ non-gimple register ones which have any use except as parameters in other
+ calls, so we can safely leve them as they are. */
+ if (!is_gimple_reg (dead_param))
+return;
+ tree parm_ddef = ssa_default_def (m_id->src_cfun, dead_param);
+ if (!parm_ddef || has_zero_uses (parm_ddef))
+return;
+
+ auto_vec stack;
+
