fab: Cleanup eh after optimize_memcpy [PR116601]
Checks
| Context |
Check |
Description |
| linaro-tcwg-bot/tcwg_gcc_build--master-arm |
success
|
Build passed
|
| linaro-tcwg-bot/tcwg_gcc_build--master-aarch64 |
success
|
Build passed
|
| linaro-tcwg-bot/tcwg_gcc_check--master-arm |
success
|
Test passed
|
Commit Message
When optimize_memcpy was added in r7-5443-g7b45d0dfeb5f85,
a path was added such that a statement was turned into a non-throwing
statement and maybe_clean_or_replace_eh_stmt/gimple_purge_dead_eh_edges
would not be called for that statement.
This adds these calls to that path.
Bootstrapped and tested on x86_64-linux-gnu with no regressions.
Ok? For the trunk, 14, 13 and 12 branches?
PR tree-optimization/116601
gcc/ChangeLog:
* tree-ssa-ccp.cc (pass_fold_builtins::execute): Cleanup eh
after optimize_memcpy on a mem statement.
gcc/testsuite/ChangeLog:
* g++.dg/torture/except-2.C: New test.
Signed-off-by: Andrew Pinski <quic_apinski@quicinc.com>
---
gcc/testsuite/g++.dg/torture/except-2.C | 18 ++++++++++++++++++
gcc/tree-ssa-ccp.cc | 11 +++++++++--
2 files changed, 27 insertions(+), 2 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/torture/except-2.C
Comments
On Thu, Sep 5, 2024 at 8:25 AM Andrew Pinski <quic_apinski@quicinc.com> wrote:
>
> When optimize_memcpy was added in r7-5443-g7b45d0dfeb5f85,
> a path was added such that a statement was turned into a non-throwing
> statement and maybe_clean_or_replace_eh_stmt/gimple_purge_dead_eh_edges
> would not be called for that statement.
> This adds these calls to that path.
>
> Bootstrapped and tested on x86_64-linux-gnu with no regressions.
>
> Ok? For the trunk, 14, 13 and 12 branches?
I wonder if this can be somehow integrated better with the existing
old_stmt = stmt;
stmt = gsi_stmt (i);
update_stmt (stmt);
if (maybe_clean_or_replace_eh_stmt (old_stmt, stmt)
&& gimple_purge_dead_eh_edges (bb))
cfg_changed = true;
which frankly looks odd - update_stmt shouldn't ever change stmt. Maybe
moving the old_stmt assign before the switch works?
> PR tree-optimization/116601
>
> gcc/ChangeLog:
>
> * tree-ssa-ccp.cc (pass_fold_builtins::execute): Cleanup eh
> after optimize_memcpy on a mem statement.
>
> gcc/testsuite/ChangeLog:
>
> * g++.dg/torture/except-2.C: New test.
>
> Signed-off-by: Andrew Pinski <quic_apinski@quicinc.com>
> ---
> gcc/testsuite/g++.dg/torture/except-2.C | 18 ++++++++++++++++++
> gcc/tree-ssa-ccp.cc | 11 +++++++++--
> 2 files changed, 27 insertions(+), 2 deletions(-)
> create mode 100644 gcc/testsuite/g++.dg/torture/except-2.C
>
> diff --git a/gcc/testsuite/g++.dg/torture/except-2.C b/gcc/testsuite/g++.dg/torture/except-2.C
> new file mode 100644
> index 00000000000..d896937a118
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/torture/except-2.C
> @@ -0,0 +1,18 @@
> +// { dg-do compile }
> +// { dg-additional-options "-fexceptions -fnon-call-exceptions" }
> +// PR tree-optimization/116601
> +
> +struct RefitOption {
> + char subtype;
> + int string;
> +} n;
> +void h(RefitOption);
> +void k(RefitOption *__val)
> +{
> + try {
> + *__val = RefitOption{};
> + RefitOption __trans_tmp_2 = *__val;
> + h(__trans_tmp_2);
> + }
> + catch(...){}
> +}
> diff --git a/gcc/tree-ssa-ccp.cc b/gcc/tree-ssa-ccp.cc
> index 44711018e0e..3cd385f476b 100644
> --- a/gcc/tree-ssa-ccp.cc
> +++ b/gcc/tree-ssa-ccp.cc
> @@ -4325,8 +4325,15 @@ pass_fold_builtins::execute (function *fun)
> if (gimple_code (stmt) != GIMPLE_CALL)
> {
> if (gimple_assign_load_p (stmt) && gimple_store_p (stmt))
> - optimize_memcpy (&i, gimple_assign_lhs (stmt),
> - gimple_assign_rhs1 (stmt), NULL_TREE);
> + {
> + optimize_memcpy (&i, gimple_assign_lhs (stmt),
> + gimple_assign_rhs1 (stmt), NULL_TREE);
> + old_stmt = stmt;
> + stmt = gsi_stmt (i);
> + if (maybe_clean_or_replace_eh_stmt (old_stmt, stmt)
> + && gimple_purge_dead_eh_edges (bb))
> + cfg_changed = true;
> + }
> gsi_next (&i);
> continue;
> }
> --
> 2.43.0
>
On Thu, Sep 5, 2024 at 12:26 AM Richard Biener
<richard.guenther@gmail.com> wrote:
>
> On Thu, Sep 5, 2024 at 8:25 AM Andrew Pinski <quic_apinski@quicinc.com> wrote:
> >
> > When optimize_memcpy was added in r7-5443-g7b45d0dfeb5f85,
> > a path was added such that a statement was turned into a non-throwing
> > statement and maybe_clean_or_replace_eh_stmt/gimple_purge_dead_eh_edges
> > would not be called for that statement.
> > This adds these calls to that path.
> >
> > Bootstrapped and tested on x86_64-linux-gnu with no regressions.
> >
> > Ok? For the trunk, 14, 13 and 12 branches?
>
> I wonder if this can be somehow integrated better with the existing
>
> old_stmt = stmt;
> stmt = gsi_stmt (i);
> update_stmt (stmt);
>
> if (maybe_clean_or_replace_eh_stmt (old_stmt, stmt)
> && gimple_purge_dead_eh_edges (bb))
> cfg_changed = true;
>
> which frankly looks odd - update_stmt shouldn't ever change stmt. Maybe
> moving the old_stmt assign before the switch works?
I agree it looks odd/wrong. But only moving the assignment before the
switch does not fix this issue since if we don't have a builtin (which
we have in this case, it is a memcpy like statement):
__trans_tmp_2 = MEM[(const struct RefitOption &)__val_5(D)];
I have a set of patches to refactor this code to simplify and fix the
issue with the update_stmt and more (since there are issues with the
atomic replacements too).
>
> > PR tree-optimization/116601
> >
> > gcc/ChangeLog:
> >
> > * tree-ssa-ccp.cc (pass_fold_builtins::execute): Cleanup eh
> > after optimize_memcpy on a mem statement.
> >
> > gcc/testsuite/ChangeLog:
> >
> > * g++.dg/torture/except-2.C: New test.
> >
> > Signed-off-by: Andrew Pinski <quic_apinski@quicinc.com>
> > ---
> > gcc/testsuite/g++.dg/torture/except-2.C | 18 ++++++++++++++++++
> > gcc/tree-ssa-ccp.cc | 11 +++++++++--
> > 2 files changed, 27 insertions(+), 2 deletions(-)
> > create mode 100644 gcc/testsuite/g++.dg/torture/except-2.C
> >
> > diff --git a/gcc/testsuite/g++.dg/torture/except-2.C b/gcc/testsuite/g++.dg/torture/except-2.C
> > new file mode 100644
> > index 00000000000..d896937a118
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/torture/except-2.C
> > @@ -0,0 +1,18 @@
> > +// { dg-do compile }
> > +// { dg-additional-options "-fexceptions -fnon-call-exceptions" }
> > +// PR tree-optimization/116601
> > +
> > +struct RefitOption {
> > + char subtype;
> > + int string;
> > +} n;
> > +void h(RefitOption);
> > +void k(RefitOption *__val)
> > +{
> > + try {
> > + *__val = RefitOption{};
> > + RefitOption __trans_tmp_2 = *__val;
> > + h(__trans_tmp_2);
> > + }
> > + catch(...){}
> > +}
> > diff --git a/gcc/tree-ssa-ccp.cc b/gcc/tree-ssa-ccp.cc
> > index 44711018e0e..3cd385f476b 100644
> > --- a/gcc/tree-ssa-ccp.cc
> > +++ b/gcc/tree-ssa-ccp.cc
> > @@ -4325,8 +4325,15 @@ pass_fold_builtins::execute (function *fun)
> > if (gimple_code (stmt) != GIMPLE_CALL)
> > {
> > if (gimple_assign_load_p (stmt) && gimple_store_p (stmt))
> > - optimize_memcpy (&i, gimple_assign_lhs (stmt),
> > - gimple_assign_rhs1 (stmt), NULL_TREE);
> > + {
> > + optimize_memcpy (&i, gimple_assign_lhs (stmt),
> > + gimple_assign_rhs1 (stmt), NULL_TREE);
> > + old_stmt = stmt;
> > + stmt = gsi_stmt (i);
> > + if (maybe_clean_or_replace_eh_stmt (old_stmt, stmt)
> > + && gimple_purge_dead_eh_edges (bb))
> > + cfg_changed = true;
> > + }
> > gsi_next (&i);
> > continue;
> > }
> > --
> > 2.43.0
> >
On Fri, Sep 6, 2024 at 3:00 AM Andrew Pinski <pinskia@gmail.com> wrote:
>
> On Thu, Sep 5, 2024 at 12:26 AM Richard Biener
> <richard.guenther@gmail.com> wrote:
> >
> > On Thu, Sep 5, 2024 at 8:25 AM Andrew Pinski <quic_apinski@quicinc.com> wrote:
> > >
> > > When optimize_memcpy was added in r7-5443-g7b45d0dfeb5f85,
> > > a path was added such that a statement was turned into a non-throwing
> > > statement and maybe_clean_or_replace_eh_stmt/gimple_purge_dead_eh_edges
> > > would not be called for that statement.
> > > This adds these calls to that path.
> > >
> > > Bootstrapped and tested on x86_64-linux-gnu with no regressions.
> > >
> > > Ok? For the trunk, 14, 13 and 12 branches?
> >
> > I wonder if this can be somehow integrated better with the existing
> >
> > old_stmt = stmt;
> > stmt = gsi_stmt (i);
> > update_stmt (stmt);
> >
> > if (maybe_clean_or_replace_eh_stmt (old_stmt, stmt)
> > && gimple_purge_dead_eh_edges (bb))
> > cfg_changed = true;
> >
> > which frankly looks odd - update_stmt shouldn't ever change stmt. Maybe
> > moving the old_stmt assign before the switch works?
>
> I agree it looks odd/wrong. But only moving the assignment before the
> switch does not fix this issue since if we don't have a builtin (which
> we have in this case, it is a memcpy like statement):
> __trans_tmp_2 = MEM[(const struct RefitOption &)__val_5(D)];
>
> I have a set of patches to refactor this code to simplify and fix the
> issue with the update_stmt and more (since there are issues with the
> atomic replacements too).
If it gets too big for branches consider the original change approved for
backporting (you might want to start with pushing that to trunk and then
refactoring). Looking I think the memcpy handling is simply misplaced
in the switch as it does more than all of the rest.
> >
> > > PR tree-optimization/116601
> > >
> > > gcc/ChangeLog:
> > >
> > > * tree-ssa-ccp.cc (pass_fold_builtins::execute): Cleanup eh
> > > after optimize_memcpy on a mem statement.
> > >
> > > gcc/testsuite/ChangeLog:
> > >
> > > * g++.dg/torture/except-2.C: New test.
> > >
> > > Signed-off-by: Andrew Pinski <quic_apinski@quicinc.com>
> > > ---
> > > gcc/testsuite/g++.dg/torture/except-2.C | 18 ++++++++++++++++++
> > > gcc/tree-ssa-ccp.cc | 11 +++++++++--
> > > 2 files changed, 27 insertions(+), 2 deletions(-)
> > > create mode 100644 gcc/testsuite/g++.dg/torture/except-2.C
> > >
> > > diff --git a/gcc/testsuite/g++.dg/torture/except-2.C b/gcc/testsuite/g++.dg/torture/except-2.C
> > > new file mode 100644
> > > index 00000000000..d896937a118
> > > --- /dev/null
> > > +++ b/gcc/testsuite/g++.dg/torture/except-2.C
> > > @@ -0,0 +1,18 @@
> > > +// { dg-do compile }
> > > +// { dg-additional-options "-fexceptions -fnon-call-exceptions" }
> > > +// PR tree-optimization/116601
> > > +
> > > +struct RefitOption {
> > > + char subtype;
> > > + int string;
> > > +} n;
> > > +void h(RefitOption);
> > > +void k(RefitOption *__val)
> > > +{
> > > + try {
> > > + *__val = RefitOption{};
> > > + RefitOption __trans_tmp_2 = *__val;
> > > + h(__trans_tmp_2);
> > > + }
> > > + catch(...){}
> > > +}
> > > diff --git a/gcc/tree-ssa-ccp.cc b/gcc/tree-ssa-ccp.cc
> > > index 44711018e0e..3cd385f476b 100644
> > > --- a/gcc/tree-ssa-ccp.cc
> > > +++ b/gcc/tree-ssa-ccp.cc
> > > @@ -4325,8 +4325,15 @@ pass_fold_builtins::execute (function *fun)
> > > if (gimple_code (stmt) != GIMPLE_CALL)
> > > {
> > > if (gimple_assign_load_p (stmt) && gimple_store_p (stmt))
> > > - optimize_memcpy (&i, gimple_assign_lhs (stmt),
> > > - gimple_assign_rhs1 (stmt), NULL_TREE);
> > > + {
> > > + optimize_memcpy (&i, gimple_assign_lhs (stmt),
> > > + gimple_assign_rhs1 (stmt), NULL_TREE);
> > > + old_stmt = stmt;
> > > + stmt = gsi_stmt (i);
> > > + if (maybe_clean_or_replace_eh_stmt (old_stmt, stmt)
> > > + && gimple_purge_dead_eh_edges (bb))
> > > + cfg_changed = true;
> > > + }
> > > gsi_next (&i);
> > > continue;
> > > }
> > > --
> > > 2.43.0
> > >
new file mode 100644
@@ -0,0 +1,18 @@
+// { dg-do compile }
+// { dg-additional-options "-fexceptions -fnon-call-exceptions" }
+// PR tree-optimization/116601
+
+struct RefitOption {
+ char subtype;
+ int string;
+} n;
+void h(RefitOption);
+void k(RefitOption *__val)
+{
+ try {
+ *__val = RefitOption{};
+ RefitOption __trans_tmp_2 = *__val;
+ h(__trans_tmp_2);
+ }
+ catch(...){}
+}
@@ -4325,8 +4325,15 @@ pass_fold_builtins::execute (function *fun)
if (gimple_code (stmt) != GIMPLE_CALL)
{
if (gimple_assign_load_p (stmt) && gimple_store_p (stmt))
- optimize_memcpy (&i, gimple_assign_lhs (stmt),
- gimple_assign_rhs1 (stmt), NULL_TREE);
+ {
+ optimize_memcpy (&i, gimple_assign_lhs (stmt),
+ gimple_assign_rhs1 (stmt), NULL_TREE);
+ old_stmt = stmt;
+ stmt = gsi_stmt (i);
+ if (maybe_clean_or_replace_eh_stmt (old_stmt, stmt)
+ && gimple_purge_dead_eh_edges (bb))
+ cfg_changed = true;
+ }
gsi_next (&i);
continue;
}