Re: [PATCH] ipa: Self-DCE of uses of removed call LHSs (PR 108007)
> Hi,
>
> PR 108007 is another manifestation where we rely on DCE to clean-up
> after IPA-SRA and if the user explicitely switches DCE off, IPA-SRA
> can leave behind statements which are fed uninitialized values and
> trap, even though their results are themselves never used.
>
> I have already fixed this for unused parameters in callees, this bug
> shows that almost the same thing can happen for removed returns, on
> the side of callers. This means that the issue has to be fixed
> elsewhere, in call redirection. This patch adds a function which
> looks for (and through, using a work-list) uses of operations fed
> specific SSA names and removes them all.
>
> That would have been easy if it wasn't for debug statements during
> tree-inline (from which call redirection is also invoked). Debug
> statements are decoupled from the rest at this point and iterating
> over uses of SSAs does not bring them up. During tree-inline they are
> handled especially at the end, I assume in order to make sure that
> relative ordering of UIDs are the same with and without debug info.
>
> This means that during tree-inline we need to make a hash of killed
> SSAs, that we already have in copy_body_data, available to the
> function making the purging. So the patch duly does also that, making
> the interface slightly ugly. Moreover, all newly unused SSA names
> need to be freed and as PR 112616 showed, it must be done in a defined
> order, which is what newly added ipa_release_ssas_in_hash does.
>
> The only difference from the patch which has already been approved in
> September but which I later had to revert is (one function name and)
> that SSAs that are to be released are first put into an auto_vec and
> sorted according their version number to avoid issues like PR 112616.
>
> The patch has passed bootstrap, LTO-bootstrap and profiled-LTO-bootstrap
> and testing on x86_64-linux, bootstrap, LTO-bootstrap and testing on
> ppc64le-linux and bootstrap and LTO-bootstrap on Aarch64, testsuite
> there is still running, OK if it passes?
>
> Thanks
>
> Martin
>
>
> gcc/ChangeLog:
>
> 2024-01-12 Martin Jambor
>
> PR ipa/108007
> PR ipa/112616
> * cgraph.h (cgraph_edge): Add a parameter to
> redirect_call_stmt_to_callee.
> * ipa-param-manipulation.h (ipa_param_adjustments): Add a
> parameter to modify_call.
> (ipa_release_ssas_in_hash): Declare.
> * cgraph.cc (cgraph_edge::redirect_call_stmt_to_callee): New
> parameter killed_ssas, pass it to padjs->modify_call.
> * ipa-param-manipulation.cc (purge_all_uses): New function.
> (ipa_param_adjustments::modify_call): New parameter killed_ssas.
> Instead of substituting uses, invoke purge_all_uses. If
> hash of killed SSAs has not been provided, create a temporary one
> and release SSAs that have been added to it.
> (compare_ssa_versions): New function.
> (ipa_release_ssas_in_hash): Likewise.
> * tree-inline.cc (redirect_all_calls): Create
> id->killed_new_ssa_names earlier, pass it to edge redirection,
> adjust a comment.
> (copy_body): Release SSAs in id->killed_new_ssa_names.
>
> gcc/testsuite/ChangeLog:
>
> 2024-01-15 Martin Jambor
>
> PR ipa/108007
> PR ipa/112616
> * gcc.dg/ipa/pr108007.c: New test.
> * gcc.dg/ipa/pr112616.c: Likewise.
> +/* Remove all statements that use NAME directly or indirectly. KILLED_SSAS
> + contains the SSA_NAMEs that are already being or have been processed and
> new
> + ones need to be added to it. The function only has to process situations
> + handled by ssa_name_only_returned_p in ipa-sra.cc with the exception that
> it
> + can assume it must never reach a use in a return statement. */
> +
> +static void
> +purge_all_uses (tree name, hash_set *killed_ssas)
I think simple_dce_from_worklist does pretty much the same but expects
bitmap instead of hash set.
It checks for some cases when statement is not removable, but these
should all pass if we declared it dead.
The patch looks OK to me, except that keeping this walk at one place may
be nice.
Honza
> +{
> + imm_use_iterator imm_iter;
> + gimple *stmt;
> + auto_vec worklist;
> +
> + worklist.safe_push (name);
> + while (!worklist.is_empty ())
> +{
> + tree cur_name = worklist.pop ();
> + FOR_EACH_IMM_USE_STMT (stmt, imm_iter, cur_name)
> + {
> + if (gimple_debug_bind_p (stmt))
> + {
> + /* When runing within tree-inline, we will never end up here but
> + adding the SSAs to killed_ssas will do the trick in this case
> + and the respective debug statements will get reset. */
> + gimple_debug_bind_reset_value (stmt);
> + update_stmt (stmt);
> + continue;
> + }
> +
> + tree lhs = NULL_TREE;
> + if (is_gimple_assign (stmt))
> + lhs = gimple_assign_lhs (stmt);
> + else if (gimple_code
[PATCH] ipa: Self-DCE of uses of removed call LHSs (PR 108007)
Hi,
PR 108007 is another manifestation where we rely on DCE to clean-up
after IPA-SRA and if the user explicitely switches DCE off, IPA-SRA
can leave behind statements which are fed uninitialized values and
trap, even though their results are themselves never used.
I have already fixed this for unused parameters in callees, this bug
shows that almost the same thing can happen for removed returns, on
the side of callers. This means that the issue has to be fixed
elsewhere, in call redirection. This patch adds a function which
looks for (and through, using a work-list) uses of operations fed
specific SSA names and removes them all.
That would have been easy if it wasn't for debug statements during
tree-inline (from which call redirection is also invoked). Debug
statements are decoupled from the rest at this point and iterating
over uses of SSAs does not bring them up. During tree-inline they are
handled especially at the end, I assume in order to make sure that
relative ordering of UIDs are the same with and without debug info.
This means that during tree-inline we need to make a hash of killed
SSAs, that we already have in copy_body_data, available to the
function making the purging. So the patch duly does also that, making
the interface slightly ugly. Moreover, all newly unused SSA names
need to be freed and as PR 112616 showed, it must be done in a defined
order, which is what newly added ipa_release_ssas_in_hash does.
The only difference from the patch which has already been approved in
September but which I later had to revert is (one function name and)
that SSAs that are to be released are first put into an auto_vec and
sorted according their version number to avoid issues like PR 112616.
The patch has passed bootstrap, LTO-bootstrap and profiled-LTO-bootstrap
and testing on x86_64-linux, bootstrap, LTO-bootstrap and testing on
ppc64le-linux and bootstrap and LTO-bootstrap on Aarch64, testsuite
there is still running, OK if it passes?
Thanks
Martin
gcc/ChangeLog:
2024-01-12 Martin Jambor
PR ipa/108007
PR ipa/112616
* cgraph.h (cgraph_edge): Add a parameter to
redirect_call_stmt_to_callee.
* ipa-param-manipulation.h (ipa_param_adjustments): Add a
parameter to modify_call.
(ipa_release_ssas_in_hash): Declare.
* cgraph.cc (cgraph_edge::redirect_call_stmt_to_callee): New
parameter killed_ssas, pass it to padjs->modify_call.
* ipa-param-manipulation.cc (purge_all_uses): New function.
(ipa_param_adjustments::modify_call): New parameter killed_ssas.
Instead of substituting uses, invoke purge_all_uses. If
hash of killed SSAs has not been provided, create a temporary one
and release SSAs that have been added to it.
(compare_ssa_versions): New function.
(ipa_release_ssas_in_hash): Likewise.
* tree-inline.cc (redirect_all_calls): Create
id->killed_new_ssa_names earlier, pass it to edge redirection,
adjust a comment.
(copy_body): Release SSAs in id->killed_new_ssa_names.
gcc/testsuite/ChangeLog:
2024-01-15 Martin Jambor
PR ipa/108007
PR ipa/112616
* gcc.dg/ipa/pr108007.c: New test.
* gcc.dg/ipa/pr112616.c: Likewise.
---
gcc/cgraph.cc | 10 ++-
gcc/cgraph.h| 9 ++-
gcc/ipa-param-manipulation.cc | 112 ++--
gcc/ipa-param-manipulation.h| 5 +-
gcc/testsuite/gcc.dg/ipa/pr108007.c | 32
gcc/testsuite/gcc.dg/ipa/pr112616.c | 28 +++
gcc/tree-inline.cc | 27 ---
7 files changed, 184 insertions(+), 39 deletions(-)
create mode 100644 gcc/testsuite/gcc.dg/ipa/pr108007.c
create mode 100644 gcc/testsuite/gcc.dg/ipa/pr112616.c
diff --git a/gcc/cgraph.cc b/gcc/cgraph.cc
index d565c005f62..0ac8f73204b 100644
--- a/gcc/cgraph.cc
+++ b/gcc/cgraph.cc
@@ -1403,11 +1403,17 @@ cgraph_edge::redirect_callee (cgraph_node *n)
speculative indirect call, remove "speculative" of the indirect call and
also redirect stmt to it's final direct target.
+ When called from within tree-inline, KILLED_SSAs has to contain the pointer
+ to killed_new_ssa_names within the copy_body_data structure and SSAs
+ discovered to be useless (if LHS is removed) will be added to it, otherwise
+ it needs to be NULL.
+
It is up to caller to iteratively transform each "speculative"
direct call as appropriate. */
gimple *
-cgraph_edge::redirect_call_stmt_to_callee (cgraph_edge *e)
+cgraph_edge::redirect_call_stmt_to_callee (cgraph_edge *e,
+ hash_set *killed_ssas)
{
tree decl = gimple_call_fndecl (e->call_stmt);
gcall *new_stmt;
@@ -1527,7 +1533,7 @@ cgraph_edge::redirect_call_stmt_to_callee (cgraph_edge *e)
remove_stmt_from_eh_lp (e->call_stmt);
tree old_fntype = gimple_call_fntype (e->call_stmt);
- new_stmt =
Re: [PATCH] ipa: Self-DCE of uses of removed call LHSs (PR 108007)
On Wed, Oct 4, 2023 at 5:08 PM Maciej W. Rozycki wrote:
>
> On Tue, 3 Oct 2023, Martin Jambor wrote:
>
> > > SSA graph may be deep so this may cause stack overflow, so I think we
> > > should use worklist here (it is also easy to do).
> > >
> > > OK with that change.
> > > Honza
> >
> > I have just committed the following after a bootstrap and testing on
> > x86_64-linux.
>
> This has regressed the native `powerpc64le-linux-gnu' configuration,
> which doesn't bootstrap here anymore:
>
> Comparing stages 2 and 3
> Bootstrap comparison failure!
> powerpc64le-linux-gnu/libstdc++-v3/src/compatibility-ldbl.o differs
> powerpc64le-linux-gnu/libstdc++-v3/src/.libs/compatibility-ldbl.o differs
>
> I have double-checked this is indeed the offending commit, the compiler
> bootstraps just fine as at commit 7eb5ce7f58ed ("Remove pass counting in
> VRP.").
>
> Shall I file a PR, or can you handle it regardless? Let me know if you
> need anything from me.
It is already filed as https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111688 .
Thanks,
Andrew
>
> Maciej
Re: [PATCH] ipa: Self-DCE of uses of removed call LHSs (PR 108007)
On Tue, 3 Oct 2023, Martin Jambor wrote:
> > SSA graph may be deep so this may cause stack overflow, so I think we
> > should use worklist here (it is also easy to do).
> >
> > OK with that change.
> > Honza
>
> I have just committed the following after a bootstrap and testing on
> x86_64-linux.
This has regressed the native `powerpc64le-linux-gnu' configuration,
which doesn't bootstrap here anymore:
Comparing stages 2 and 3
Bootstrap comparison failure!
powerpc64le-linux-gnu/libstdc++-v3/src/compatibility-ldbl.o differs
powerpc64le-linux-gnu/libstdc++-v3/src/.libs/compatibility-ldbl.o differs
I have double-checked this is indeed the offending commit, the compiler
bootstraps just fine as at commit 7eb5ce7f58ed ("Remove pass counting in
VRP.").
Shall I file a PR, or can you handle it regardless? Let me know if you
need anything from me.
Maciej
Re: [PATCH] ipa: Self-DCE of uses of removed call LHSs (PR 108007)
Hello,
On Mon, Sep 25 2023, Jan Hubicka wrote:
[...]
>> >> +static void
>> >> +purge_transitive_uses (tree name, hash_set *killed_ssas)
>> >> +{
>> >> + imm_use_iterator imm_iter;
>> >> + gimple *stmt;
>> >> +
>> >> + FOR_EACH_IMM_USE_STMT (stmt, imm_iter, name)
>> >> +{
>> >> + if (gimple_debug_bind_p (stmt))
>> >> + {
>> >> + /* When runing within tree-inline, we will never end up here but
>> >> + adding the SSAs to killed_ssas will do the trick in this case and
>> >> + the respective debug statements will get reset. */
>> >> +
>> >> + gimple_debug_bind_reset_value (stmt);
>> >> + update_stmt (stmt);
>> >> + continue;
>> >> + }
>> >> +
>> >> + tree lhs = NULL_TREE;
>> >> + if (is_gimple_assign (stmt))
>> >> + lhs = gimple_assign_lhs (stmt);
>> >> + else if (gimple_code (stmt) == GIMPLE_PHI)
>> >> + lhs = gimple_phi_result (stmt);
>> >> + gcc_assert (lhs
>> >> + && (TREE_CODE (lhs) == SSA_NAME)
>> >> + && !gimple_vdef (stmt));
>> >> +
>> >> + if (!killed_ssas->contains (lhs))
>> >> + {
>> >> + killed_ssas->add (lhs);
>> >> + purge_transitive_uses (lhs, killed_ssas);
>
> SSA graph may be deep so this may cause stack overflow, so I think we
> should use worklist here (it is also easy to do).
>
> OK with that change.
> Honza
I have just committed the following after a bootstrap and testing on
x86_64-linux.
Thanks,
Martin
PR 108007 is another manifestation where we rely on DCE to clean-up
after IPA-SRA and if the user explicitely switches DCE off, IPA-SRA
can leave behind statements which are fed uninitialized values and
trap, even though their results are themselves never used.
I have already fixed this for unused parameters in callees, this bug
shows that almost the same thing can happen for removed returns, on
the side of callers. This means that the issue has to be fixed
elsewhere, in call redirection. This patch adds a function which
looks for (and through, using a work-list) uses of operations fed
specific SSA names and removes them all.
That would have been easy if it wasn't for debug statements during
tree-inline (from which call redirection is also invoked). Debug
statements are decoupled from the rest at this point and iterating
over uses of SSAs does not bring them up. During tree-inline they are
handled especially at the end, I assume in order to make sure that
relative ordering of UIDs are the same with and without debug info.
This means that during tree-inline we need to make a hash of killed
SSAs, that we already have in copy_body_data, available to the
function making the purging. So the patch duly does also that, making
the interface slightly ugly.
gcc/ChangeLog:
2023-09-27 Martin Jambor
PR ipa/108007
* cgraph.h (cgraph_edge): Add a parameter to
redirect_call_stmt_to_callee.
* ipa-param-manipulation.h (ipa_param_adjustments): Add a
parameter to modify_call.
* cgraph.cc (cgraph_edge::redirect_call_stmt_to_callee): New
parameter killed_ssas, pass it to padjs->modify_call.
* ipa-param-manipulation.cc (purge_transitive_uses): New function.
(ipa_param_adjustments::modify_call): New parameter killed_ssas.
Instead of substituting uses, invoke purge_transitive_uses. If
hash of killed SSAs has not been provided, create a temporary one
and release SSAs that have been added to it.
* tree-inline.cc (redirect_all_calls): Create
id->killed_new_ssa_names earlier, pass it to edge redirection,
adjust a comment.
(copy_body): Release SSAs in id->killed_new_ssa_names.
gcc/testsuite/ChangeLog:
2023-05-11 Martin Jambor
PR ipa/108007
* gcc.dg/ipa/pr108007.c: New test.
---
gcc/cgraph.cc | 10 +++-
gcc/cgraph.h| 9 ++-
gcc/ipa-param-manipulation.cc | 88 +
gcc/ipa-param-manipulation.h| 3 +-
gcc/testsuite/gcc.dg/ipa/pr108007.c | 32 +++
gcc/tree-inline.cc | 28 +
6 files changed, 132 insertions(+), 38 deletions(-)
create mode 100644 gcc/testsuite/gcc.dg/ipa/pr108007.c
diff --git a/gcc/cgraph.cc b/gcc/cgraph.cc
index e41e5ad3ae7..b82367ac342 100644
--- a/gcc/cgraph.cc
+++ b/gcc/cgraph.cc
@@ -1403,11 +1403,17 @@ cgraph_edge::redirect_callee (cgraph_node *n)
speculative indirect call, remove "speculative" of the indirect call and
also redirect stmt to it's final direct target.
+ When called from within tree-inline, KILLED_SSAs has to contain the pointer
+ to killed_new_ssa_names within the copy_body_data structure and SSAs
+ discovered to be useless (if LHS is removed) will be added to it, otherwise
+ it needs to be NULL.
+
It is up to caller to iteratively transform each "speculative"
direct call as appropriate. */
gimple *
-cgraph_edge::redirect_call_stmt_to_callee (cgraph_edge *e)
Re: [PATCH] ipa: Self-DCE of uses of removed call LHSs (PR 108007)
> >> PR 108007 is another manifestation where we rely on DCE to clean-up
> >> after IPA-SRA and if the user explicitely switches DCE off, IPA-SRA
> >> can leave behind statements which are fed uninitialized values and
> >> trap, even though their results are themselves never used.
> >>
> >> I have already fixed this for unused parameters in callees, this bug
> >> shows that almost the same thing can happen for removed returns, on
> >> the side of callers. This means that the issue has to be fixed
> >> elsewhere, in call redirection. This patch adds a function which
> >> recursivewly looks for uses of operations fed specific SSA names and
> >> removes them all.
> >>
> >> That would have been easy if it wasn't for debug statements during
> >> tree-inline (from which call redirection is also invoked). Debug
> >> statements are decoupled from the rest at this point and iterating
> >> over uses of SSAs does not bring them up. During tree-inline they are
> >> handled especially at the end, I assume in order to make sure that
> >> relative ordering of UIDs are the same with and without debug info.
> >>
> >> This means that during tree-inline we need to make a hash of killed
> >> SSAs, that we already have in copy_body_data, available to the
> >> function making the purging. So the patch duly does also that, making
> >> the interface slightly ugly.
> >>
> >> Bootstrapped and tested on x86_64-linux. OK for master? (I am not sure
> >> the problem is grave enough to warrant backporting to release branches
> >> but can do that as well if people think I should.)
> >>
> >> Thanks,
> >>
> >> Martin
> >>
> >>
> >> gcc/ChangeLog:
> >>
> >> 2023-05-11 Martin Jambor
> >>
> >>PR ipa/108007
> >>* cgraph.h (cgraph_edge): Add a parameter to
> >>redirect_call_stmt_to_callee.
> >>* ipa-param-manipulation.h (ipa_param_adjustments): Added a
> >>parameter to modify_call.
> >>* cgraph.cc (cgraph_edge::redirect_call_stmt_to_callee): New
> >>parameter killed_ssas, pass it to padjs->modify_call.
> >>* ipa-param-manipulation.cc (purge_transitive_uses): New function.
> >>(ipa_param_adjustments::modify_call): New parameter killed_ssas.
> >>Instead of substitutin uses, invoke purge_transitive_uses. If
> >>hash of killed SSAs has not been provided, create a temporary one
> >>and release SSAs that have been added to it.
> >>* tree-inline.cc (redirect_all_calls): Create
> >>id->killed_new_ssa_names earlier, pass it to edge redirection,
> >>adjust a comment.
> >>(copy_body): Release SSAs in id->killed_new_ssa_names.
> >>
> >> gcc/testsuite/ChangeLog:
> >>
> >> 2023-05-11 Martin Jambor
> >>
> >>PR ipa/108007
> >>* gcc.dg/ipa/pr108007.c: New test.
> >> ---
> >> gcc/cgraph.cc | 10 +++-
> >> gcc/cgraph.h| 9 ++-
> >> gcc/ipa-param-manipulation.cc | 85 +
> >> gcc/ipa-param-manipulation.h| 3 +-
> >> gcc/testsuite/gcc.dg/ipa/pr108007.c | 32 +++
> >> gcc/tree-inline.cc | 28 ++
> >> 6 files changed, 129 insertions(+), 38 deletions(-)
> >> create mode 100644 gcc/testsuite/gcc.dg/ipa/pr108007.c
> >>
> >> +/* Remove all statements that use NAME and transitively those that use the
> >> + result of such statements. KILLED_SSAS contains the SSA_NAMEs that are
> >> + already being or have been processed and new ones need to be added to
> >> it.
> >> + The funtction only has to process situations handled by
> >> + ssa_name_only_returned_p in ipa-sra.cc with the exception that it can
> >> assume
> >> + it must never reach a use in a return statement. */
> >> +
> >> +static void
> >> +purge_transitive_uses (tree name, hash_set *killed_ssas)
> >> +{
> >> + imm_use_iterator imm_iter;
> >> + gimple *stmt;
> >> +
> >> + FOR_EACH_IMM_USE_STMT (stmt, imm_iter, name)
> >> +{
> >> + if (gimple_debug_bind_p (stmt))
> >> + {
> >> +/* When runing within tree-inline, we will never end up here but
> >> + adding the SSAs to killed_ssas will do the trick in this case and
> >> + the respective debug statements will get reset. */
> >> +
> >> +gimple_debug_bind_reset_value (stmt);
> >> +update_stmt (stmt);
> >> +continue;
> >> + }
> >> +
> >> + tree lhs = NULL_TREE;
> >> + if (is_gimple_assign (stmt))
> >> + lhs = gimple_assign_lhs (stmt);
> >> + else if (gimple_code (stmt) == GIMPLE_PHI)
> >> + lhs = gimple_phi_result (stmt);
> >> + gcc_assert (lhs
> >> +&& (TREE_CODE (lhs) == SSA_NAME)
> >> +&& !gimple_vdef (stmt));
> >> +
> >> + if (!killed_ssas->contains (lhs))
> >> + {
> >> +killed_ssas->add (lhs);
> >> +purge_transitive_uses (lhs, killed_ssas);
SSA graph may be deep so this may cause stack overflow, so I think we
should use worklist here (it is also easy to do).
OK with that change.
Honza
Re: [PATCH] ipa: Self-DCE of uses of removed call LHSs (PR 108007)
Hello,
and ping.
Thanks,
Martin
On Fri, Sep 01 2023, Martin Jambor wrote:
> Hello
>
> and ping.
>
> Thanks,
>
> Martin
>
>
> On Fri, May 12 2023, Martin Jambor wrote:
>> Hi,
>>
>> PR 108007 is another manifestation where we rely on DCE to clean-up
>> after IPA-SRA and if the user explicitely switches DCE off, IPA-SRA
>> can leave behind statements which are fed uninitialized values and
>> trap, even though their results are themselves never used.
>>
>> I have already fixed this for unused parameters in callees, this bug
>> shows that almost the same thing can happen for removed returns, on
>> the side of callers. This means that the issue has to be fixed
>> elsewhere, in call redirection. This patch adds a function which
>> recursivewly looks for uses of operations fed specific SSA names and
>> removes them all.
>>
>> That would have been easy if it wasn't for debug statements during
>> tree-inline (from which call redirection is also invoked). Debug
>> statements are decoupled from the rest at this point and iterating
>> over uses of SSAs does not bring them up. During tree-inline they are
>> handled especially at the end, I assume in order to make sure that
>> relative ordering of UIDs are the same with and without debug info.
>>
>> This means that during tree-inline we need to make a hash of killed
>> SSAs, that we already have in copy_body_data, available to the
>> function making the purging. So the patch duly does also that, making
>> the interface slightly ugly.
>>
>> Bootstrapped and tested on x86_64-linux. OK for master? (I am not sure
>> the problem is grave enough to warrant backporting to release branches
>> but can do that as well if people think I should.)
>>
>> Thanks,
>>
>> Martin
>>
>>
>> gcc/ChangeLog:
>>
>> 2023-05-11 Martin Jambor
>>
>> PR ipa/108007
>> * cgraph.h (cgraph_edge): Add a parameter to
>> redirect_call_stmt_to_callee.
>> * ipa-param-manipulation.h (ipa_param_adjustments): Added a
>> parameter to modify_call.
>> * cgraph.cc (cgraph_edge::redirect_call_stmt_to_callee): New
>> parameter killed_ssas, pass it to padjs->modify_call.
>> * ipa-param-manipulation.cc (purge_transitive_uses): New function.
>> (ipa_param_adjustments::modify_call): New parameter killed_ssas.
>> Instead of substitutin uses, invoke purge_transitive_uses. If
>> hash of killed SSAs has not been provided, create a temporary one
>> and release SSAs that have been added to it.
>> * tree-inline.cc (redirect_all_calls): Create
>> id->killed_new_ssa_names earlier, pass it to edge redirection,
>> adjust a comment.
>> (copy_body): Release SSAs in id->killed_new_ssa_names.
>>
>> gcc/testsuite/ChangeLog:
>>
>> 2023-05-11 Martin Jambor
>>
>> PR ipa/108007
>> * gcc.dg/ipa/pr108007.c: New test.
>> ---
>> gcc/cgraph.cc | 10 +++-
>> gcc/cgraph.h| 9 ++-
>> gcc/ipa-param-manipulation.cc | 85 +
>> gcc/ipa-param-manipulation.h| 3 +-
>> gcc/testsuite/gcc.dg/ipa/pr108007.c | 32 +++
>> gcc/tree-inline.cc | 28 ++
>> 6 files changed, 129 insertions(+), 38 deletions(-)
>> create mode 100644 gcc/testsuite/gcc.dg/ipa/pr108007.c
>>
>> diff --git a/gcc/cgraph.cc b/gcc/cgraph.cc
>> index e8f9bec8227..5e923bf0557 100644
>> --- a/gcc/cgraph.cc
>> +++ b/gcc/cgraph.cc
>> @@ -1403,11 +1403,17 @@ cgraph_edge::redirect_callee (cgraph_node *n)
>> speculative indirect call, remove "speculative" of the indirect call and
>> also redirect stmt to it's final direct target.
>>
>> + When called from within tree-inline, KILLED_SSAs has to contain the
>> pointer
>> + to killed_new_ssa_names within the copy_body_data structure and SSAs
>> + discovered to be useless (if LHS is removed) will be added to it,
>> otherwise
>> + it needs to be NULL.
>> +
>> It is up to caller to iteratively transform each "speculative"
>> direct call as appropriate. */
>>
>> gimple *
>> -cgraph_edge::redirect_call_stmt_to_callee (cgraph_edge *e)
>> +cgraph_edge::redirect_call_stmt_to_callee (cgraph_edge *e,
>> + hash_set *killed_ssas)
>> {
>>tree decl = gimple_call_fndecl (e->call_stmt);
>>gcall *new_stmt;
>> @@ -1527,7 +1533,7 @@ cgraph_edge::redirect_call_stmt_to_callee (cgraph_edge
>> *e)
>> remove_stmt_from_eh_lp (e->call_stmt);
>>
>>tree old_fntype = gimple_call_fntype (e->call_stmt);
>> - new_stmt = padjs->modify_call (e, false);
>> + new_stmt = padjs->modify_call (e, false, killed_ssas);
>>cgraph_node *origin = e->callee;
>>while (origin->clone_of)
>> origin = origin->clone_of;
>> diff --git a/gcc/cgraph.h b/gcc/cgraph.h
>> index f5f54769eda..c1a3691b6f5 100644
>> --- a/gcc/cgraph.h
>> +++ b/gcc/cgraph.h
>> @@ -1833,9 +1833,16 @@ public:
>> speculative indirect call, remove "speculative" of the indirect
Re: [PATCH] ipa: Self-DCE of uses of removed call LHSs (PR 108007)
Hello
and ping.
Thanks,
Martin
On Fri, May 12 2023, Martin Jambor wrote:
> Hi,
>
> PR 108007 is another manifestation where we rely on DCE to clean-up
> after IPA-SRA and if the user explicitely switches DCE off, IPA-SRA
> can leave behind statements which are fed uninitialized values and
> trap, even though their results are themselves never used.
>
> I have already fixed this for unused parameters in callees, this bug
> shows that almost the same thing can happen for removed returns, on
> the side of callers. This means that the issue has to be fixed
> elsewhere, in call redirection. This patch adds a function which
> recursivewly looks for uses of operations fed specific SSA names and
> removes them all.
>
> That would have been easy if it wasn't for debug statements during
> tree-inline (from which call redirection is also invoked). Debug
> statements are decoupled from the rest at this point and iterating
> over uses of SSAs does not bring them up. During tree-inline they are
> handled especially at the end, I assume in order to make sure that
> relative ordering of UIDs are the same with and without debug info.
>
> This means that during tree-inline we need to make a hash of killed
> SSAs, that we already have in copy_body_data, available to the
> function making the purging. So the patch duly does also that, making
> the interface slightly ugly.
>
> Bootstrapped and tested on x86_64-linux. OK for master? (I am not sure
> the problem is grave enough to warrant backporting to release branches
> but can do that as well if people think I should.)
>
> Thanks,
>
> Martin
>
>
> gcc/ChangeLog:
>
> 2023-05-11 Martin Jambor
>
> PR ipa/108007
> * cgraph.h (cgraph_edge): Add a parameter to
> redirect_call_stmt_to_callee.
> * ipa-param-manipulation.h (ipa_param_adjustments): Added a
> parameter to modify_call.
> * cgraph.cc (cgraph_edge::redirect_call_stmt_to_callee): New
> parameter killed_ssas, pass it to padjs->modify_call.
> * ipa-param-manipulation.cc (purge_transitive_uses): New function.
> (ipa_param_adjustments::modify_call): New parameter killed_ssas.
> Instead of substitutin uses, invoke purge_transitive_uses. If
> hash of killed SSAs has not been provided, create a temporary one
> and release SSAs that have been added to it.
> * tree-inline.cc (redirect_all_calls): Create
> id->killed_new_ssa_names earlier, pass it to edge redirection,
> adjust a comment.
> (copy_body): Release SSAs in id->killed_new_ssa_names.
>
> gcc/testsuite/ChangeLog:
>
> 2023-05-11 Martin Jambor
>
> PR ipa/108007
> * gcc.dg/ipa/pr108007.c: New test.
> ---
> gcc/cgraph.cc | 10 +++-
> gcc/cgraph.h| 9 ++-
> gcc/ipa-param-manipulation.cc | 85 +
> gcc/ipa-param-manipulation.h| 3 +-
> gcc/testsuite/gcc.dg/ipa/pr108007.c | 32 +++
> gcc/tree-inline.cc | 28 ++
> 6 files changed, 129 insertions(+), 38 deletions(-)
> create mode 100644 gcc/testsuite/gcc.dg/ipa/pr108007.c
>
> diff --git a/gcc/cgraph.cc b/gcc/cgraph.cc
> index e8f9bec8227..5e923bf0557 100644
> --- a/gcc/cgraph.cc
> +++ b/gcc/cgraph.cc
> @@ -1403,11 +1403,17 @@ cgraph_edge::redirect_callee (cgraph_node *n)
> speculative indirect call, remove "speculative" of the indirect call and
> also redirect stmt to it's final direct target.
>
> + When called from within tree-inline, KILLED_SSAs has to contain the
> pointer
> + to killed_new_ssa_names within the copy_body_data structure and SSAs
> + discovered to be useless (if LHS is removed) will be added to it,
> otherwise
> + it needs to be NULL.
> +
> It is up to caller to iteratively transform each "speculative"
> direct call as appropriate. */
>
> gimple *
> -cgraph_edge::redirect_call_stmt_to_callee (cgraph_edge *e)
> +cgraph_edge::redirect_call_stmt_to_callee (cgraph_edge *e,
> +hash_set *killed_ssas)
> {
>tree decl = gimple_call_fndecl (e->call_stmt);
>gcall *new_stmt;
> @@ -1527,7 +1533,7 @@ cgraph_edge::redirect_call_stmt_to_callee (cgraph_edge
> *e)
> remove_stmt_from_eh_lp (e->call_stmt);
>
>tree old_fntype = gimple_call_fntype (e->call_stmt);
> - new_stmt = padjs->modify_call (e, false);
> + new_stmt = padjs->modify_call (e, false, killed_ssas);
>cgraph_node *origin = e->callee;
>while (origin->clone_of)
> origin = origin->clone_of;
> diff --git a/gcc/cgraph.h b/gcc/cgraph.h
> index f5f54769eda..c1a3691b6f5 100644
> --- a/gcc/cgraph.h
> +++ b/gcc/cgraph.h
> @@ -1833,9 +1833,16 @@ public:
> speculative indirect call, remove "speculative" of the indirect call and
> also redirect stmt to it's final direct target.
>
> + When called from within tree-inline, KILLED_SSAs has to contain the
> + pointer to killed_new_ssa_names
Re: [PATCH] ipa: Self-DCE of uses of removed call LHSs (PR 108007)
On 13 June 2023 17:11:13 CEST, Martin Jambor wrote: >Ping. s/funtction/function/ s/runing/running/ > >Thanks,
Re: [PATCH] ipa: Self-DCE of uses of removed call LHSs (PR 108007)
Ping.
Thanks,
Martin
On Fri, May 12 2023, Martin Jambor wrote:
> Hi,
>
> PR 108007 is another manifestation where we rely on DCE to clean-up
> after IPA-SRA and if the user explicitely switches DCE off, IPA-SRA
> can leave behind statements which are fed uninitialized values and
> trap, even though their results are themselves never used.
>
> I have already fixed this for unused parameters in callees, this bug
> shows that almost the same thing can happen for removed returns, on
> the side of callers. This means that the issue has to be fixed
> elsewhere, in call redirection. This patch adds a function which
> recursivewly looks for uses of operations fed specific SSA names and
> removes them all.
>
> That would have been easy if it wasn't for debug statements during
> tree-inline (from which call redirection is also invoked). Debug
> statements are decoupled from the rest at this point and iterating
> over uses of SSAs does not bring them up. During tree-inline they are
> handled especially at the end, I assume in order to make sure that
> relative ordering of UIDs are the same with and without debug info.
>
> This means that during tree-inline we need to make a hash of killed
> SSAs, that we already have in copy_body_data, available to the
> function making the purging. So the patch duly does also that, making
> the interface slightly ugly.
>
> Bootstrapped and tested on x86_64-linux. OK for master? (I am not sure
> the problem is grave enough to warrant backporting to release branches
> but can do that as well if people think I should.)
>
> Thanks,
>
> Martin
>
>
> gcc/ChangeLog:
>
> 2023-05-11 Martin Jambor
>
> PR ipa/108007
> * cgraph.h (cgraph_edge): Add a parameter to
> redirect_call_stmt_to_callee.
> * ipa-param-manipulation.h (ipa_param_adjustments): Added a
> parameter to modify_call.
> * cgraph.cc (cgraph_edge::redirect_call_stmt_to_callee): New
> parameter killed_ssas, pass it to padjs->modify_call.
> * ipa-param-manipulation.cc (purge_transitive_uses): New function.
> (ipa_param_adjustments::modify_call): New parameter killed_ssas.
> Instead of substitutin uses, invoke purge_transitive_uses. If
> hash of killed SSAs has not been provided, create a temporary one
> and release SSAs that have been added to it.
> * tree-inline.cc (redirect_all_calls): Create
> id->killed_new_ssa_names earlier, pass it to edge redirection,
> adjust a comment.
> (copy_body): Release SSAs in id->killed_new_ssa_names.
>
> gcc/testsuite/ChangeLog:
>
> 2023-05-11 Martin Jambor
>
> PR ipa/108007
> * gcc.dg/ipa/pr108007.c: New test.
> ---
> gcc/cgraph.cc | 10 +++-
> gcc/cgraph.h| 9 ++-
> gcc/ipa-param-manipulation.cc | 85 +
> gcc/ipa-param-manipulation.h| 3 +-
> gcc/testsuite/gcc.dg/ipa/pr108007.c | 32 +++
> gcc/tree-inline.cc | 28 ++
> 6 files changed, 129 insertions(+), 38 deletions(-)
> create mode 100644 gcc/testsuite/gcc.dg/ipa/pr108007.c
>
> diff --git a/gcc/cgraph.cc b/gcc/cgraph.cc
> index e8f9bec8227..5e923bf0557 100644
> --- a/gcc/cgraph.cc
> +++ b/gcc/cgraph.cc
> @@ -1403,11 +1403,17 @@ cgraph_edge::redirect_callee (cgraph_node *n)
> speculative indirect call, remove "speculative" of the indirect call and
> also redirect stmt to it's final direct target.
>
> + When called from within tree-inline, KILLED_SSAs has to contain the
> pointer
> + to killed_new_ssa_names within the copy_body_data structure and SSAs
> + discovered to be useless (if LHS is removed) will be added to it,
> otherwise
> + it needs to be NULL.
> +
> It is up to caller to iteratively transform each "speculative"
> direct call as appropriate. */
>
> gimple *
> -cgraph_edge::redirect_call_stmt_to_callee (cgraph_edge *e)
> +cgraph_edge::redirect_call_stmt_to_callee (cgraph_edge *e,
> +hash_set *killed_ssas)
> {
>tree decl = gimple_call_fndecl (e->call_stmt);
>gcall *new_stmt;
> @@ -1527,7 +1533,7 @@ cgraph_edge::redirect_call_stmt_to_callee (cgraph_edge
> *e)
> remove_stmt_from_eh_lp (e->call_stmt);
>
>tree old_fntype = gimple_call_fntype (e->call_stmt);
> - new_stmt = padjs->modify_call (e, false);
> + new_stmt = padjs->modify_call (e, false, killed_ssas);
>cgraph_node *origin = e->callee;
>while (origin->clone_of)
> origin = origin->clone_of;
> diff --git a/gcc/cgraph.h b/gcc/cgraph.h
> index f5f54769eda..c1a3691b6f5 100644
> --- a/gcc/cgraph.h
> +++ b/gcc/cgraph.h
> @@ -1833,9 +1833,16 @@ public:
> speculative indirect call, remove "speculative" of the indirect call and
> also redirect stmt to it's final direct target.
>
> + When called from within tree-inline, KILLED_SSAs has to contain the
> + pointer to killed_new_ssa_names within the
Re: [PATCH] ipa: Self-DCE of uses of removed call LHSs (PR 108007)
On 12 May 2023 14:45:14 CEST, Martin Jambor wrote: >gcc/ChangeLog: > >2023-05-11 Martin Jambor > > PR ipa/108007 > * cgraph.h (cgraph_edge): Add a parameter to > redirect_call_stmt_to_callee. > * ipa-param-manipulation.h (ipa_param_adjustments): Added a > parameter to modify_call. > * cgraph.cc (cgraph_edge::redirect_call_stmt_to_callee): New > parameter killed_ssas, pass it to padjs->modify_call. > * ipa-param-manipulation.cc (purge_transitive_uses): New function. > (ipa_param_adjustments::modify_call): New parameter killed_ssas. > Instead of substitutin uses, invoke purge_transitive_uses. If > hash of killed SSAs has not been provided, create a temporary one > and release SSAs that have been added to it. > * tree-inline.cc (redirect_all_calls): Create > id->killed_new_ssa_names earlier, pass it to edge redirection, > adjust a comment. > (copy_body): Release SSAs in id->killed_new_ssa_names. Nit: Please use present tense in the ChangeLog. s/Added/Add/ And there is a trailing 'g' missing in substitutin thanks,
[PATCH] ipa: Self-DCE of uses of removed call LHSs (PR 108007)
Hi,
PR 108007 is another manifestation where we rely on DCE to clean-up
after IPA-SRA and if the user explicitely switches DCE off, IPA-SRA
can leave behind statements which are fed uninitialized values and
trap, even though their results are themselves never used.
I have already fixed this for unused parameters in callees, this bug
shows that almost the same thing can happen for removed returns, on
the side of callers. This means that the issue has to be fixed
elsewhere, in call redirection. This patch adds a function which
recursivewly looks for uses of operations fed specific SSA names and
removes them all.
That would have been easy if it wasn't for debug statements during
tree-inline (from which call redirection is also invoked). Debug
statements are decoupled from the rest at this point and iterating
over uses of SSAs does not bring them up. During tree-inline they are
handled especially at the end, I assume in order to make sure that
relative ordering of UIDs are the same with and without debug info.
This means that during tree-inline we need to make a hash of killed
SSAs, that we already have in copy_body_data, available to the
function making the purging. So the patch duly does also that, making
the interface slightly ugly.
Bootstrapped and tested on x86_64-linux. OK for master? (I am not sure
the problem is grave enough to warrant backporting to release branches
but can do that as well if people think I should.)
Thanks,
Martin
gcc/ChangeLog:
2023-05-11 Martin Jambor
PR ipa/108007
* cgraph.h (cgraph_edge): Add a parameter to
redirect_call_stmt_to_callee.
* ipa-param-manipulation.h (ipa_param_adjustments): Added a
parameter to modify_call.
* cgraph.cc (cgraph_edge::redirect_call_stmt_to_callee): New
parameter killed_ssas, pass it to padjs->modify_call.
* ipa-param-manipulation.cc (purge_transitive_uses): New function.
(ipa_param_adjustments::modify_call): New parameter killed_ssas.
Instead of substitutin uses, invoke purge_transitive_uses. If
hash of killed SSAs has not been provided, create a temporary one
and release SSAs that have been added to it.
* tree-inline.cc (redirect_all_calls): Create
id->killed_new_ssa_names earlier, pass it to edge redirection,
adjust a comment.
(copy_body): Release SSAs in id->killed_new_ssa_names.
gcc/testsuite/ChangeLog:
2023-05-11 Martin Jambor
PR ipa/108007
* gcc.dg/ipa/pr108007.c: New test.
---
gcc/cgraph.cc | 10 +++-
gcc/cgraph.h| 9 ++-
gcc/ipa-param-manipulation.cc | 85 +
gcc/ipa-param-manipulation.h| 3 +-
gcc/testsuite/gcc.dg/ipa/pr108007.c | 32 +++
gcc/tree-inline.cc | 28 ++
6 files changed, 129 insertions(+), 38 deletions(-)
create mode 100644 gcc/testsuite/gcc.dg/ipa/pr108007.c
diff --git a/gcc/cgraph.cc b/gcc/cgraph.cc
index e8f9bec8227..5e923bf0557 100644
--- a/gcc/cgraph.cc
+++ b/gcc/cgraph.cc
@@ -1403,11 +1403,17 @@ cgraph_edge::redirect_callee (cgraph_node *n)
speculative indirect call, remove "speculative" of the indirect call and
also redirect stmt to it's final direct target.
+ When called from within tree-inline, KILLED_SSAs has to contain the pointer
+ to killed_new_ssa_names within the copy_body_data structure and SSAs
+ discovered to be useless (if LHS is removed) will be added to it, otherwise
+ it needs to be NULL.
+
It is up to caller to iteratively transform each "speculative"
direct call as appropriate. */
gimple *
-cgraph_edge::redirect_call_stmt_to_callee (cgraph_edge *e)
+cgraph_edge::redirect_call_stmt_to_callee (cgraph_edge *e,
+ hash_set *killed_ssas)
{
tree decl = gimple_call_fndecl (e->call_stmt);
gcall *new_stmt;
@@ -1527,7 +1533,7 @@ cgraph_edge::redirect_call_stmt_to_callee (cgraph_edge *e)
remove_stmt_from_eh_lp (e->call_stmt);
tree old_fntype = gimple_call_fntype (e->call_stmt);
- new_stmt = padjs->modify_call (e, false);
+ new_stmt = padjs->modify_call (e, false, killed_ssas);
cgraph_node *origin = e->callee;
while (origin->clone_of)
origin = origin->clone_of;
diff --git a/gcc/cgraph.h b/gcc/cgraph.h
index f5f54769eda..c1a3691b6f5 100644
--- a/gcc/cgraph.h
+++ b/gcc/cgraph.h
@@ -1833,9 +1833,16 @@ public:
speculative indirect call, remove "speculative" of the indirect call and
also redirect stmt to it's final direct target.
+ When called from within tree-inline, KILLED_SSAs has to contain the
+ pointer to killed_new_ssa_names within the copy_body_data structure and
+ SSAs discovered to be useless (if LHS is removed) will be added to it,
+ otherwise it needs to be NULL.
+
It is up to caller to iteratively transform each "speculative"
direct call as appropriate.
