| Message ID | ri6bk8r5kfi.fsf@virgil.suse.cz |
|---|---|
| State | New |
| Headers | show |
| Series | ipa: Avoid excessive removing of SSAs (PR 113757) | expand |
Hello, and ping please. Martin On Thu, Feb 08 2024, Martin Jambor wrote: > Hi, > > PR 113757 shows that the code which was meant to debug-reset and > remove SSAs defined by LHSs of calls redirected to > __builtin_unreachable can trigger also when speculative > devirtualization creates a call to a noreturn function (and since it > is noreturn, it does not bother dealing with its return value). > > What is more, it seems that the code handling this case is not really > necessary. I feel slightly idiotic about this because I have a > feeling that I added it because of a failing test-case but I can > neither find the testcase nor a reason why the code in > cgraph_edge::redirect_call_stmt_to_callee would not be sufficient (it > turns the SSA name into a default-def, a bit like IPA-SRA, but any > code dominated by a call to a noreturn is not dangerous when it comes > to its side-effects). So this patch just removes the handling. > > Bootstrapped and tested on x86_64-linux and ppc64le-linux. I have also > LTO-bootstrapped and LTO-profilebootstrapped the patch on x86_64-linux. > > OK for master? > > Thanks, > > Martin > > > gcc/ChangeLog: > > 2024-02-07 Martin Jambor <mjambor@suse.cz> > > PR ipa/113757 > * tree-inline.cc (redirect_all_calls): Remove code adding SSAs to > id->killed_new_ssa_names. > > gcc/testsuite/ChangeLog: > > 2024-02-07 Martin Jambor <mjambor@suse.cz> > > PR ipa/113757 > * g++.dg/ipa/pr113757.C: New test. > --- > gcc/testsuite/g++.dg/ipa/pr113757.C | 14 ++++++++++++++ > gcc/tree-inline.cc | 14 ++------------ > 2 files changed, 16 insertions(+), 12 deletions(-) > create mode 100644 gcc/testsuite/g++.dg/ipa/pr113757.C > > diff --git a/gcc/testsuite/g++.dg/ipa/pr113757.C b/gcc/testsuite/g++.dg/ipa/pr113757.C > new file mode 100644 > index 00000000000..885d4010a10 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/ipa/pr113757.C > @@ -0,0 +1,14 @@ > +// { dg-do compile } > +// { dg-options "-O2 -fPIC" } > +// { dg-require-effective-target fpic } > + > +long size(); > +struct ll { virtual int hh(); }; > +ll *slice_owner; > +int ll::hh() { __builtin_exit(0); } > +int nn() { > + if (size()) > + return 0; > + return slice_owner->hh(); > +} > +int (*a)() = nn; > diff --git a/gcc/tree-inline.cc b/gcc/tree-inline.cc > index 75c10eb7dfc..cac41b4f031 100644 > --- a/gcc/tree-inline.cc > +++ b/gcc/tree-inline.cc > @@ -2984,23 +2984,13 @@ redirect_all_calls (copy_body_data * id, basic_block bb) > gimple *stmt = gsi_stmt (si); > if (is_gimple_call (stmt)) > { > - tree old_lhs = gimple_call_lhs (stmt); > struct cgraph_edge *edge = id->dst_node->get_edge (stmt); > if (edge) > { > if (!id->killed_new_ssa_names) > id->killed_new_ssa_names = new hash_set<tree> (16); > - gimple *new_stmt > - = cgraph_edge::redirect_call_stmt_to_callee (edge, > - id->killed_new_ssa_names); > - if (old_lhs > - && TREE_CODE (old_lhs) == SSA_NAME > - && !gimple_call_lhs (new_stmt)) > - /* In case of IPA-SRA removing the LHS, the name should have > - been already added to the hash. But in case of redirecting > - to builtin_unreachable it was not and the name still should > - be pruned from debug statements. */ > - id->killed_new_ssa_names->add (old_lhs); > + cgraph_edge::redirect_call_stmt_to_callee (edge, > + id->killed_new_ssa_names); > > if (stmt == last && id->call_stmt && maybe_clean_eh_stmt (stmt)) > gimple_purge_dead_eh_edges (bb); > -- > 2.43.0
> On Thu, Feb 08 2024, Martin Jambor wrote: > > Hi, > > > > PR 113757 shows that the code which was meant to debug-reset and > > remove SSAs defined by LHSs of calls redirected to > > __builtin_unreachable can trigger also when speculative > > devirtualization creates a call to a noreturn function (and since it > > is noreturn, it does not bother dealing with its return value). > > > > What is more, it seems that the code handling this case is not really > > necessary. I feel slightly idiotic about this because I have a > > feeling that I added it because of a failing test-case but I can > > neither find the testcase nor a reason why the code in > > cgraph_edge::redirect_call_stmt_to_callee would not be sufficient (it > > turns the SSA name into a default-def, a bit like IPA-SRA, but any > > code dominated by a call to a noreturn is not dangerous when it comes > > to its side-effects). So this patch just removes the handling. > > > > Bootstrapped and tested on x86_64-linux and ppc64le-linux. I have also > > LTO-bootstrapped and LTO-profilebootstrapped the patch on x86_64-linux. > > > > OK for master? > > > > Thanks, > > > > Martin > > > > > > gcc/ChangeLog: > > > > 2024-02-07 Martin Jambor <mjambor@suse.cz> > > > > PR ipa/113757 > > * tree-inline.cc (redirect_all_calls): Remove code adding SSAs to > > id->killed_new_ssa_names. > > > > gcc/testsuite/ChangeLog: > > > > 2024-02-07 Martin Jambor <mjambor@suse.cz> > > > > PR ipa/113757 > > * g++.dg/ipa/pr113757.C: New test. OK, thanks! Honza > > --- > > gcc/testsuite/g++.dg/ipa/pr113757.C | 14 ++++++++++++++ > > gcc/tree-inline.cc | 14 ++------------ > > 2 files changed, 16 insertions(+), 12 deletions(-) > > create mode 100644 gcc/testsuite/g++.dg/ipa/pr113757.C > > > > diff --git a/gcc/testsuite/g++.dg/ipa/pr113757.C b/gcc/testsuite/g++.dg/ipa/pr113757.C > > new file mode 100644 > > index 00000000000..885d4010a10 > > --- /dev/null > > +++ b/gcc/testsuite/g++.dg/ipa/pr113757.C > > @@ -0,0 +1,14 @@ > > +// { dg-do compile } > > +// { dg-options "-O2 -fPIC" } > > +// { dg-require-effective-target fpic } > > + > > +long size(); > > +struct ll { virtual int hh(); }; > > +ll *slice_owner; > > +int ll::hh() { __builtin_exit(0); } > > +int nn() { > > + if (size()) > > + return 0; > > + return slice_owner->hh(); > > +} > > +int (*a)() = nn; > > diff --git a/gcc/tree-inline.cc b/gcc/tree-inline.cc > > index 75c10eb7dfc..cac41b4f031 100644 > > --- a/gcc/tree-inline.cc > > +++ b/gcc/tree-inline.cc > > @@ -2984,23 +2984,13 @@ redirect_all_calls (copy_body_data * id, basic_block bb) > > gimple *stmt = gsi_stmt (si); > > if (is_gimple_call (stmt)) > > { > > - tree old_lhs = gimple_call_lhs (stmt); > > struct cgraph_edge *edge = id->dst_node->get_edge (stmt); > > if (edge) > > { > > if (!id->killed_new_ssa_names) > > id->killed_new_ssa_names = new hash_set<tree> (16); > > - gimple *new_stmt > > - = cgraph_edge::redirect_call_stmt_to_callee (edge, > > - id->killed_new_ssa_names); > > - if (old_lhs > > - && TREE_CODE (old_lhs) == SSA_NAME > > - && !gimple_call_lhs (new_stmt)) > > - /* In case of IPA-SRA removing the LHS, the name should have > > - been already added to the hash. But in case of redirecting > > - to builtin_unreachable it was not and the name still should > > - be pruned from debug statements. */ > > - id->killed_new_ssa_names->add (old_lhs); > > + cgraph_edge::redirect_call_stmt_to_callee (edge, > > + id->killed_new_ssa_names); > > > > if (stmt == last && id->call_stmt && maybe_clean_eh_stmt (stmt)) > > gimple_purge_dead_eh_edges (bb); > > -- > > 2.43.0
diff --git a/gcc/testsuite/g++.dg/ipa/pr113757.C b/gcc/testsuite/g++.dg/ipa/pr113757.C new file mode 100644 index 00000000000..885d4010a10 --- /dev/null +++ b/gcc/testsuite/g++.dg/ipa/pr113757.C @@ -0,0 +1,14 @@ +// { dg-do compile } +// { dg-options "-O2 -fPIC" } +// { dg-require-effective-target fpic } + +long size(); +struct ll { virtual int hh(); }; +ll *slice_owner; +int ll::hh() { __builtin_exit(0); } +int nn() { + if (size()) + return 0; + return slice_owner->hh(); +} +int (*a)() = nn; diff --git a/gcc/tree-inline.cc b/gcc/tree-inline.cc index 75c10eb7dfc..cac41b4f031 100644 --- a/gcc/tree-inline.cc +++ b/gcc/tree-inline.cc @@ -2984,23 +2984,13 @@ redirect_all_calls (copy_body_data * id, basic_block bb) gimple *stmt = gsi_stmt (si); if (is_gimple_call (stmt)) { - tree old_lhs = gimple_call_lhs (stmt); struct cgraph_edge *edge = id->dst_node->get_edge (stmt); if (edge) { if (!id->killed_new_ssa_names) id->killed_new_ssa_names = new hash_set<tree> (16); - gimple *new_stmt - = cgraph_edge::redirect_call_stmt_to_callee (edge, - id->killed_new_ssa_names); - if (old_lhs - && TREE_CODE (old_lhs) == SSA_NAME - && !gimple_call_lhs (new_stmt)) - /* In case of IPA-SRA removing the LHS, the name should have - been already added to the hash. But in case of redirecting - to builtin_unreachable it was not and the name still should - be pruned from debug statements. */ - id->killed_new_ssa_names->add (old_lhs); + cgraph_edge::redirect_call_stmt_to_callee (edge, + id->killed_new_ssa_names); if (stmt == last && id->call_stmt && maybe_clean_eh_stmt (stmt)) gimple_purge_dead_eh_edges (bb);