* [PATCH] ipa-sra: Don't consider CLOBBERS as writes preventing splitting @ 2023-07-31 17:04 Martin Jambor 2023-08-02 8:03 ` Richard Biener 0 siblings, 1 reply; 8+ messages in thread From: Martin Jambor @ 2023-07-31 17:04 UTC (permalink / raw) To: GCC Patches; +Cc: Jan Hubicka Hi, when IPA-SRA detects whether a parameter passed by reference is written to, it does not special case CLOBBERs which means it often bails out unnecessarily, especially when dealing with C++ destructors. Fixed by the obvious continue in the two relevant loops. The (slightly) more complex testcases in the PR need surprisingly more effort but the simple one can be fixed now easily by this patch and I'll work on the others incrementally. Bootstrapped and currently undergoing testsuite run on x86_64-linux. OK if it passes too? Thanks, Martin gcc/ChangeLog: 2023-07-31 Martin Jambor <mjambor@suse.cz> PR ipa/110378 * ipa-sra.cc (isra_track_scalar_value_uses): Ignore clobbers. (ptr_parm_has_nonarg_uses): Likewise. gcc/testsuite/ChangeLog: 2023-07-31 Martin Jambor <mjambor@suse.cz> PR ipa/110378 * g++.dg/ipa/pr110378-1.C: New test. --- gcc/ipa-sra.cc | 6 ++-- gcc/testsuite/g++.dg/ipa/pr110378-1.C | 47 +++++++++++++++++++++++++++ 2 files changed, 51 insertions(+), 2 deletions(-) create mode 100644 gcc/testsuite/g++.dg/ipa/pr110378-1.C diff --git a/gcc/ipa-sra.cc b/gcc/ipa-sra.cc index c35e03b7abd..edba364f56e 100644 --- a/gcc/ipa-sra.cc +++ b/gcc/ipa-sra.cc @@ -898,7 +898,8 @@ isra_track_scalar_value_uses (function *fun, cgraph_node *node, tree name, FOR_EACH_IMM_USE_STMT (stmt, imm_iter, name) { - if (is_gimple_debug (stmt)) + if (is_gimple_debug (stmt) + || gimple_clobber_p (stmt)) continue; /* TODO: We could handle at least const builtin functions like arithmetic @@ -1056,7 +1057,8 @@ ptr_parm_has_nonarg_uses (cgraph_node *node, function *fun, tree parm, unsigned uses_ok = 0; use_operand_p use_p; - if (is_gimple_debug (stmt)) + if (is_gimple_debug (stmt) + || gimple_clobber_p (stmt)) continue; if (gimple_assign_single_p (stmt)) diff --git a/gcc/testsuite/g++.dg/ipa/pr110378-1.C b/gcc/testsuite/g++.dg/ipa/pr110378-1.C new file mode 100644 index 00000000000..aabe326b8b2 --- /dev/null +++ b/gcc/testsuite/g++.dg/ipa/pr110378-1.C @@ -0,0 +1,47 @@ +/* { dg-do compile } */ +/* { dg-options "-O2 -fdump-ipa-sra -fdump-tree-optimized-slim" } */ + +/* Test that even though destructors end with clobbering all of *this, it + should not prevent IPA-SRA. */ + +namespace { + + class foo + { + public: + int *a; + foo(int c) + { + a = new int[c]; + a[0] = 4; + } + __attribute__((noinline)) ~foo(); + int f () + { + return a[0] + 1; + } + }; + + volatile int v1 = 4; + + __attribute__((noinline)) foo::~foo() + { + delete[] a; + return; + } + + +} + +volatile int v2 = 20; + +int test (void) +{ + foo shouldnotexist(v2); + v2 = shouldnotexist.f(); + return 0; +} + + +/* { dg-final { scan-ipa-dump "Will split parameter 0" "sra" } } */ +/* { dg-final { scan-tree-dump-not "shouldnotexist" "optimized" } } */ -- 2.41.0 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] ipa-sra: Don't consider CLOBBERS as writes preventing splitting 2023-07-31 17:04 [PATCH] ipa-sra: Don't consider CLOBBERS as writes preventing splitting Martin Jambor @ 2023-08-02 8:03 ` Richard Biener 2023-08-03 10:19 ` Jan Hubicka 2023-08-04 16:26 ` Martin Jambor 0 siblings, 2 replies; 8+ messages in thread From: Richard Biener @ 2023-08-02 8:03 UTC (permalink / raw) To: Martin Jambor; +Cc: GCC Patches, Jan Hubicka On Mon, Jul 31, 2023 at 7:05 PM Martin Jambor <mjambor@suse.cz> wrote: > > Hi, > > when IPA-SRA detects whether a parameter passed by reference is > written to, it does not special case CLOBBERs which means it often > bails out unnecessarily, especially when dealing with C++ destructors. > Fixed by the obvious continue in the two relevant loops. > > The (slightly) more complex testcases in the PR need surprisingly more > effort but the simple one can be fixed now easily by this patch and I'll > work on the others incrementally. > > Bootstrapped and currently undergoing testsuite run on x86_64-linux. OK > if it passes too? LGTM, btw - how are the clobbers handled during transform? > Thanks, > > Martin > > > > > gcc/ChangeLog: > > 2023-07-31 Martin Jambor <mjambor@suse.cz> > > PR ipa/110378 > * ipa-sra.cc (isra_track_scalar_value_uses): Ignore clobbers. > (ptr_parm_has_nonarg_uses): Likewise. > > gcc/testsuite/ChangeLog: > > 2023-07-31 Martin Jambor <mjambor@suse.cz> > > PR ipa/110378 > * g++.dg/ipa/pr110378-1.C: New test. > --- > gcc/ipa-sra.cc | 6 ++-- > gcc/testsuite/g++.dg/ipa/pr110378-1.C | 47 +++++++++++++++++++++++++++ > 2 files changed, 51 insertions(+), 2 deletions(-) > create mode 100644 gcc/testsuite/g++.dg/ipa/pr110378-1.C > > diff --git a/gcc/ipa-sra.cc b/gcc/ipa-sra.cc > index c35e03b7abd..edba364f56e 100644 > --- a/gcc/ipa-sra.cc > +++ b/gcc/ipa-sra.cc > @@ -898,7 +898,8 @@ isra_track_scalar_value_uses (function *fun, cgraph_node *node, tree name, > > FOR_EACH_IMM_USE_STMT (stmt, imm_iter, name) > { > - if (is_gimple_debug (stmt)) > + if (is_gimple_debug (stmt) > + || gimple_clobber_p (stmt)) > continue; > > /* TODO: We could handle at least const builtin functions like arithmetic > @@ -1056,7 +1057,8 @@ ptr_parm_has_nonarg_uses (cgraph_node *node, function *fun, tree parm, > unsigned uses_ok = 0; > use_operand_p use_p; > > - if (is_gimple_debug (stmt)) > + if (is_gimple_debug (stmt) > + || gimple_clobber_p (stmt)) > continue; > > if (gimple_assign_single_p (stmt)) > diff --git a/gcc/testsuite/g++.dg/ipa/pr110378-1.C b/gcc/testsuite/g++.dg/ipa/pr110378-1.C > new file mode 100644 > index 00000000000..aabe326b8b2 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/ipa/pr110378-1.C > @@ -0,0 +1,47 @@ > +/* { dg-do compile } */ > +/* { dg-options "-O2 -fdump-ipa-sra -fdump-tree-optimized-slim" } */ > + > +/* Test that even though destructors end with clobbering all of *this, it > + should not prevent IPA-SRA. */ > + > +namespace { > + > + class foo > + { > + public: > + int *a; > + foo(int c) > + { > + a = new int[c]; > + a[0] = 4; > + } > + __attribute__((noinline)) ~foo(); > + int f () > + { > + return a[0] + 1; > + } > + }; > + > + volatile int v1 = 4; > + > + __attribute__((noinline)) foo::~foo() > + { > + delete[] a; > + return; > + } > + > + > +} > + > +volatile int v2 = 20; > + > +int test (void) > +{ > + foo shouldnotexist(v2); > + v2 = shouldnotexist.f(); > + return 0; > +} > + > + > +/* { dg-final { scan-ipa-dump "Will split parameter 0" "sra" } } */ > +/* { dg-final { scan-tree-dump-not "shouldnotexist" "optimized" } } */ > -- > 2.41.0 > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] ipa-sra: Don't consider CLOBBERS as writes preventing splitting 2023-08-02 8:03 ` Richard Biener @ 2023-08-03 10:19 ` Jan Hubicka 2023-08-04 16:26 ` Martin Jambor 1 sibling, 0 replies; 8+ messages in thread From: Jan Hubicka @ 2023-08-03 10:19 UTC (permalink / raw) To: Richard Biener; +Cc: Martin Jambor, GCC Patches > On Mon, Jul 31, 2023 at 7:05 PM Martin Jambor <mjambor@suse.cz> wrote: > > > > Hi, > > > > when IPA-SRA detects whether a parameter passed by reference is > > written to, it does not special case CLOBBERs which means it often > > bails out unnecessarily, especially when dealing with C++ destructors. > > Fixed by the obvious continue in the two relevant loops. > > > > The (slightly) more complex testcases in the PR need surprisingly more > > effort but the simple one can be fixed now easily by this patch and I'll > > work on the others incrementally. > > > > Bootstrapped and currently undergoing testsuite run on x86_64-linux. OK > > if it passes too? > > LGTM, btw - how are the clobbers handled during transform? Looks good to me too. I was also wondering if we want to preserve something about the clobber. If SRA fully suceeds it would not be needed but if the original location is not fully SRAed we may theoretically lose information. We put additonal clobber after destructor call, so one would need to wrap it in non-dstructor and be sure that ipa-modref understands the clobber in order to obtain a testcase. Honza ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] ipa-sra: Don't consider CLOBBERS as writes preventing splitting 2023-08-02 8:03 ` Richard Biener 2023-08-03 10:19 ` Jan Hubicka @ 2023-08-04 16:26 ` Martin Jambor 2023-08-04 17:57 ` Richard Biener 2023-08-11 13:04 ` Christophe Lyon 1 sibling, 2 replies; 8+ messages in thread From: Martin Jambor @ 2023-08-04 16:26 UTC (permalink / raw) To: Richard Biener; +Cc: GCC Patches, Jan Hubicka Hello, On Wed, Aug 02 2023, Richard Biener wrote: > On Mon, Jul 31, 2023 at 7:05 PM Martin Jambor <mjambor@suse.cz> wrote: >> >> Hi, >> >> when IPA-SRA detects whether a parameter passed by reference is >> written to, it does not special case CLOBBERs which means it often >> bails out unnecessarily, especially when dealing with C++ destructors. >> Fixed by the obvious continue in the two relevant loops. >> >> The (slightly) more complex testcases in the PR need surprisingly more >> effort but the simple one can be fixed now easily by this patch and I'll >> work on the others incrementally. >> >> Bootstrapped and currently undergoing testsuite run on x86_64-linux. OK >> if it passes too? > > LGTM, btw - how are the clobbers handled during transform? it turns out your question is spot on. I assumed that the mini-DCE that I implemented into IPA-SRA transform would delete but I had a closer look and it is not invoked on split parameters,only on removed ones. What was actually happening is that the parameter got remapped to a default definition of a replacement VAR_DECL and we were thus gimple-clobbering a pointer pointing to nowhere. The clobber then got DSEd and so I originally did not notice looking at the optimized dump. Still that is of course not ideal and so I added a simple function removing clobbers when splitting. I as considering adding that functionality to ipa_param_body_adjustments::mark_dead_statements but that would make the function harder to read without much gain. So thanks again for the remark. The following passes bootstrap and testing on x86_64-linux. I am running LTO bootstrap now. OK if it passes? Martin When IPA-SRA detects whether a parameter passed by reference is written to, it does not special case CLOBBERs which means it often bails out unnecessarily, especially when dealing with C++ destructors. Fixed by the obvious continue in the two relevant loops and by adding a simple function that marks the clobbers in the transformation code as statements to be removed. gcc/ChangeLog: 2023-08-04 Martin Jambor <mjambor@suse.cz> PR ipa/110378 * ipa-param-manipulation.h (class ipa_param_body_adjustments): New members get_ddef_if_exists_and_is_used and mark_clobbers_dead. * ipa-sra.cc (isra_track_scalar_value_uses): Ignore clobbers. (ptr_parm_has_nonarg_uses): Likewise. * ipa-param-manipulation.cc (ipa_param_body_adjustments::get_ddef_if_exists_and_is_used): New. (ipa_param_body_adjustments::mark_dead_statements): Move initial checks to get_ddef_if_exists_and_is_used. (ipa_param_body_adjustments::mark_clobbers_dead): New. (ipa_param_body_adjustments::common_initialization): Call mark_clobbers_dead when splitting. gcc/testsuite/ChangeLog: 2023-07-31 Martin Jambor <mjambor@suse.cz> PR ipa/110378 * g++.dg/ipa/pr110378-1.C: New test. --- gcc/ipa-param-manipulation.cc | 44 +++++++++++++++++++++--- gcc/ipa-param-manipulation.h | 2 ++ gcc/ipa-sra.cc | 6 ++-- gcc/testsuite/g++.dg/ipa/pr110378-1.C | 48 +++++++++++++++++++++++++++ 4 files changed, 94 insertions(+), 6 deletions(-) create mode 100644 gcc/testsuite/g++.dg/ipa/pr110378-1.C diff --git a/gcc/ipa-param-manipulation.cc b/gcc/ipa-param-manipulation.cc index a286af7f5d9..4a185ddbdf4 100644 --- a/gcc/ipa-param-manipulation.cc +++ b/gcc/ipa-param-manipulation.cc @@ -1072,6 +1072,20 @@ ipa_param_body_adjustments::carry_over_param (tree t) return new_parm; } +/* If DECL is a gimple register that has a default definition SSA name and that + has some uses, return the default definition, otherwise return NULL_TREE. */ + +tree +ipa_param_body_adjustments::get_ddef_if_exists_and_is_used (tree decl) +{ + if (!is_gimple_reg (decl)) + return NULL_TREE; + tree ddef = ssa_default_def (m_id->src_cfun, decl); + if (!ddef || has_zero_uses (ddef)) + return NULL_TREE; + return ddef; +} + /* 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. Push all removed SSA names that @@ -1084,10 +1098,8 @@ 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)) + tree parm_ddef = get_ddef_if_exists_and_is_used (dead_param); + if (!parm_ddef) return; auto_vec<tree, 4> stack; @@ -1169,6 +1181,28 @@ ipa_param_body_adjustments::mark_dead_statements (tree dead_param, m_dead_ssa_debug_equiv.put (parm_ddef, dp_ddecl); } +/* Put all clobbers of of dereference of default definition of PARAM into + m_dead_stmts. */ + +void +ipa_param_body_adjustments::mark_clobbers_dead (tree param) +{ + if (!is_gimple_reg (param)) + return; + tree ddef = get_ddef_if_exists_and_is_used (param); + if (!ddef) + return; + + imm_use_iterator imm_iter; + use_operand_p use_p; + FOR_EACH_IMM_USE_FAST (use_p, imm_iter, ddef) + { + gimple *stmt = USE_STMT (use_p); + if (gimple_clobber_p (stmt)) + m_dead_stmts.add (stmt); + } +} + /* Callback to walk_tree. If REMAP is an SSA_NAME that is present in hash_map passed in DATA, replace it with unshared version of what it was mapped to. If an SSA argument would be remapped to NULL, the whole operation needs to @@ -1504,6 +1538,8 @@ ipa_param_body_adjustments::common_initialization (tree old_fndecl, that will guide what not to copy to the new body. */ if (!split[i]) mark_dead_statements (m_oparms[i], &ssas_to_process_debug); + else + mark_clobbers_dead (m_oparms[i]); if (MAY_HAVE_DEBUG_STMTS && is_gimple_reg (m_oparms[i])) m_reset_debug_decls.safe_push (m_oparms[i]); diff --git a/gcc/ipa-param-manipulation.h b/gcc/ipa-param-manipulation.h index 9798eedf0b6..d6a26e9ad36 100644 --- a/gcc/ipa-param-manipulation.h +++ b/gcc/ipa-param-manipulation.h @@ -378,7 +378,9 @@ private: bool modify_call_stmt (gcall **stmt_p, gimple *orig_stmt); bool modify_cfun_body (); void reset_debug_stmts (); + tree get_ddef_if_exists_and_is_used (tree decl); void mark_dead_statements (tree dead_param, vec<tree> *debugstack); + void mark_clobbers_dead (tree dead_param); bool prepare_debug_expressions (tree dead_ssa); /* Declaration of the function that is being transformed. */ diff --git a/gcc/ipa-sra.cc b/gcc/ipa-sra.cc index c35e03b7abd..edba364f56e 100644 --- a/gcc/ipa-sra.cc +++ b/gcc/ipa-sra.cc @@ -898,7 +898,8 @@ isra_track_scalar_value_uses (function *fun, cgraph_node *node, tree name, FOR_EACH_IMM_USE_STMT (stmt, imm_iter, name) { - if (is_gimple_debug (stmt)) + if (is_gimple_debug (stmt) + || gimple_clobber_p (stmt)) continue; /* TODO: We could handle at least const builtin functions like arithmetic @@ -1056,7 +1057,8 @@ ptr_parm_has_nonarg_uses (cgraph_node *node, function *fun, tree parm, unsigned uses_ok = 0; use_operand_p use_p; - if (is_gimple_debug (stmt)) + if (is_gimple_debug (stmt) + || gimple_clobber_p (stmt)) continue; if (gimple_assign_single_p (stmt)) diff --git a/gcc/testsuite/g++.dg/ipa/pr110378-1.C b/gcc/testsuite/g++.dg/ipa/pr110378-1.C new file mode 100644 index 00000000000..fda7699795a --- /dev/null +++ b/gcc/testsuite/g++.dg/ipa/pr110378-1.C @@ -0,0 +1,48 @@ +/* { dg-do compile } */ +/* { dg-options "-O2 -fdump-ipa-sra -fdump-tree-optimized-slim" } */ + +/* Test that even though destructors end with clobbering all of *this, it + should not prevent IPA-SRA. */ + +namespace { + + class foo + { + public: + short move_offset_of_a; + int *a; + foo(int c) + { + a = new int[c]; + a[0] = 4; + } + __attribute__((noinline)) ~foo(); + int f () + { + return a[0] + 1; + } + }; + + volatile int v1 = 4; + + __attribute__((noinline)) foo::~foo() + { + delete[] a; + return; + } + + +} + +volatile int v2 = 20; + +int test (void) +{ + foo shouldnotexist(v2); + v2 = shouldnotexist.f(); + return 0; +} + + +/* { dg-final { scan-ipa-dump "Will split parameter 0" "sra" } } */ +/* { dg-final { scan-tree-dump-not "shouldnotexist" "optimized" } } */ -- 2.41.0 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] ipa-sra: Don't consider CLOBBERS as writes preventing splitting 2023-08-04 16:26 ` Martin Jambor @ 2023-08-04 17:57 ` Richard Biener 2023-08-11 13:04 ` Christophe Lyon 1 sibling, 0 replies; 8+ messages in thread From: Richard Biener @ 2023-08-04 17:57 UTC (permalink / raw) To: Martin Jambor; +Cc: GCC Patches, Jan Hubicka > Am 04.08.2023 um 18:26 schrieb Martin Jambor <mjambor@suse.cz>: > > Hello, > >> On Wed, Aug 02 2023, Richard Biener wrote: >>> On Mon, Jul 31, 2023 at 7:05 PM Martin Jambor <mjambor@suse.cz> wrote: >>> >>> Hi, >>> >>> when IPA-SRA detects whether a parameter passed by reference is >>> written to, it does not special case CLOBBERs which means it often >>> bails out unnecessarily, especially when dealing with C++ destructors. >>> Fixed by the obvious continue in the two relevant loops. >>> >>> The (slightly) more complex testcases in the PR need surprisingly more >>> effort but the simple one can be fixed now easily by this patch and I'll >>> work on the others incrementally. >>> >>> Bootstrapped and currently undergoing testsuite run on x86_64-linux. OK >>> if it passes too? >> >> LGTM, btw - how are the clobbers handled during transform? > > it turns out your question is spot on. I assumed that the mini-DCE that > I implemented into IPA-SRA transform would delete but I had a closer > look and it is not invoked on split parameters,only on removed ones. > What was actually happening is that the parameter got remapped to a > default definition of a replacement VAR_DECL and we were thus > gimple-clobbering a pointer pointing to nowhere. The clobber then got > DSEd and so I originally did not notice looking at the optimized dump. > > Still that is of course not ideal and so I added a simple function > removing clobbers when splitting. I as considering adding that > functionality to ipa_param_body_adjustments::mark_dead_statements but > that would make the function harder to read without much gain. > > So thanks again for the remark. The following passes bootstrap and > testing on x86_64-linux. I am running LTO bootstrap now. OK if it > passes? Ok Richard > Martin > > > > When IPA-SRA detects whether a parameter passed by reference is > written to, it does not special case CLOBBERs which means it often > bails out unnecessarily, especially when dealing with C++ destructors. > Fixed by the obvious continue in the two relevant loops and by adding > a simple function that marks the clobbers in the transformation code > as statements to be removed. > > gcc/ChangeLog: > > 2023-08-04 Martin Jambor <mjambor@suse.cz> > > PR ipa/110378 > * ipa-param-manipulation.h (class ipa_param_body_adjustments): New > members get_ddef_if_exists_and_is_used and mark_clobbers_dead. > * ipa-sra.cc (isra_track_scalar_value_uses): Ignore clobbers. > (ptr_parm_has_nonarg_uses): Likewise. > * ipa-param-manipulation.cc > (ipa_param_body_adjustments::get_ddef_if_exists_and_is_used): New. > (ipa_param_body_adjustments::mark_dead_statements): Move initial > checks to get_ddef_if_exists_and_is_used. > (ipa_param_body_adjustments::mark_clobbers_dead): New. > (ipa_param_body_adjustments::common_initialization): Call > mark_clobbers_dead when splitting. > > gcc/testsuite/ChangeLog: > > 2023-07-31 Martin Jambor <mjambor@suse.cz> > > PR ipa/110378 > * g++.dg/ipa/pr110378-1.C: New test. > --- > gcc/ipa-param-manipulation.cc | 44 +++++++++++++++++++++--- > gcc/ipa-param-manipulation.h | 2 ++ > gcc/ipa-sra.cc | 6 ++-- > gcc/testsuite/g++.dg/ipa/pr110378-1.C | 48 +++++++++++++++++++++++++++ > 4 files changed, 94 insertions(+), 6 deletions(-) > create mode 100644 gcc/testsuite/g++.dg/ipa/pr110378-1.C > > diff --git a/gcc/ipa-param-manipulation.cc b/gcc/ipa-param-manipulation.cc > index a286af7f5d9..4a185ddbdf4 100644 > --- a/gcc/ipa-param-manipulation.cc > +++ b/gcc/ipa-param-manipulation.cc > @@ -1072,6 +1072,20 @@ ipa_param_body_adjustments::carry_over_param (tree t) > return new_parm; > } > > +/* If DECL is a gimple register that has a default definition SSA name and that > + has some uses, return the default definition, otherwise return NULL_TREE. */ > + > +tree > +ipa_param_body_adjustments::get_ddef_if_exists_and_is_used (tree decl) > +{ > + if (!is_gimple_reg (decl)) > + return NULL_TREE; > + tree ddef = ssa_default_def (m_id->src_cfun, decl); > + if (!ddef || has_zero_uses (ddef)) > + return NULL_TREE; > + return ddef; > +} > + > /* 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. Push all removed SSA names that > @@ -1084,10 +1098,8 @@ 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)) > + tree parm_ddef = get_ddef_if_exists_and_is_used (dead_param); > + if (!parm_ddef) > return; > > auto_vec<tree, 4> stack; > @@ -1169,6 +1181,28 @@ ipa_param_body_adjustments::mark_dead_statements (tree dead_param, > m_dead_ssa_debug_equiv.put (parm_ddef, dp_ddecl); > } > > +/* Put all clobbers of of dereference of default definition of PARAM into > + m_dead_stmts. */ > + > +void > +ipa_param_body_adjustments::mark_clobbers_dead (tree param) > +{ > + if (!is_gimple_reg (param)) > + return; > + tree ddef = get_ddef_if_exists_and_is_used (param); > + if (!ddef) > + return; > + > + imm_use_iterator imm_iter; > + use_operand_p use_p; > + FOR_EACH_IMM_USE_FAST (use_p, imm_iter, ddef) > + { > + gimple *stmt = USE_STMT (use_p); > + if (gimple_clobber_p (stmt)) > + m_dead_stmts.add (stmt); > + } > +} > + > /* Callback to walk_tree. If REMAP is an SSA_NAME that is present in hash_map > passed in DATA, replace it with unshared version of what it was mapped to. > If an SSA argument would be remapped to NULL, the whole operation needs to > @@ -1504,6 +1538,8 @@ ipa_param_body_adjustments::common_initialization (tree old_fndecl, > that will guide what not to copy to the new body. */ > if (!split[i]) > mark_dead_statements (m_oparms[i], &ssas_to_process_debug); > + else > + mark_clobbers_dead (m_oparms[i]); > if (MAY_HAVE_DEBUG_STMTS > && is_gimple_reg (m_oparms[i])) > m_reset_debug_decls.safe_push (m_oparms[i]); > diff --git a/gcc/ipa-param-manipulation.h b/gcc/ipa-param-manipulation.h > index 9798eedf0b6..d6a26e9ad36 100644 > --- a/gcc/ipa-param-manipulation.h > +++ b/gcc/ipa-param-manipulation.h > @@ -378,7 +378,9 @@ private: > bool modify_call_stmt (gcall **stmt_p, gimple *orig_stmt); > bool modify_cfun_body (); > void reset_debug_stmts (); > + tree get_ddef_if_exists_and_is_used (tree decl); > void mark_dead_statements (tree dead_param, vec<tree> *debugstack); > + void mark_clobbers_dead (tree dead_param); > bool prepare_debug_expressions (tree dead_ssa); > > /* Declaration of the function that is being transformed. */ > diff --git a/gcc/ipa-sra.cc b/gcc/ipa-sra.cc > index c35e03b7abd..edba364f56e 100644 > --- a/gcc/ipa-sra.cc > +++ b/gcc/ipa-sra.cc > @@ -898,7 +898,8 @@ isra_track_scalar_value_uses (function *fun, cgraph_node *node, tree name, > > FOR_EACH_IMM_USE_STMT (stmt, imm_iter, name) > { > - if (is_gimple_debug (stmt)) > + if (is_gimple_debug (stmt) > + || gimple_clobber_p (stmt)) > continue; > > /* TODO: We could handle at least const builtin functions like arithmetic > @@ -1056,7 +1057,8 @@ ptr_parm_has_nonarg_uses (cgraph_node *node, function *fun, tree parm, > unsigned uses_ok = 0; > use_operand_p use_p; > > - if (is_gimple_debug (stmt)) > + if (is_gimple_debug (stmt) > + || gimple_clobber_p (stmt)) > continue; > > if (gimple_assign_single_p (stmt)) > diff --git a/gcc/testsuite/g++.dg/ipa/pr110378-1.C b/gcc/testsuite/g++.dg/ipa/pr110378-1.C > new file mode 100644 > index 00000000000..fda7699795a > --- /dev/null > +++ b/gcc/testsuite/g++.dg/ipa/pr110378-1.C > @@ -0,0 +1,48 @@ > +/* { dg-do compile } */ > +/* { dg-options "-O2 -fdump-ipa-sra -fdump-tree-optimized-slim" } */ > + > +/* Test that even though destructors end with clobbering all of *this, it > + should not prevent IPA-SRA. */ > + > +namespace { > + > + class foo > + { > + public: > + short move_offset_of_a; > + int *a; > + foo(int c) > + { > + a = new int[c]; > + a[0] = 4; > + } > + __attribute__((noinline)) ~foo(); > + int f () > + { > + return a[0] + 1; > + } > + }; > + > + volatile int v1 = 4; > + > + __attribute__((noinline)) foo::~foo() > + { > + delete[] a; > + return; > + } > + > + > +} > + > +volatile int v2 = 20; > + > +int test (void) > +{ > + foo shouldnotexist(v2); > + v2 = shouldnotexist.f(); > + return 0; > +} > + > + > +/* { dg-final { scan-ipa-dump "Will split parameter 0" "sra" } } */ > +/* { dg-final { scan-tree-dump-not "shouldnotexist" "optimized" } } */ > -- > 2.41.0 > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] ipa-sra: Don't consider CLOBBERS as writes preventing splitting 2023-08-04 16:26 ` Martin Jambor 2023-08-04 17:57 ` Richard Biener @ 2023-08-11 13:04 ` Christophe Lyon 2023-08-11 13:50 ` Martin Jambor 1 sibling, 1 reply; 8+ messages in thread From: Christophe Lyon @ 2023-08-11 13:04 UTC (permalink / raw) To: Martin Jambor; +Cc: Richard Biener, GCC Patches, Jan Hubicka [-- Attachment #1: Type: text/plain, Size: 9740 bytes --] Hi Martin, On Fri, 4 Aug 2023 at 18:26, Martin Jambor <mjambor@suse.cz> wrote: > Hello, > > On Wed, Aug 02 2023, Richard Biener wrote: > > On Mon, Jul 31, 2023 at 7:05 PM Martin Jambor <mjambor@suse.cz> wrote: > >> > >> Hi, > >> > >> when IPA-SRA detects whether a parameter passed by reference is > >> written to, it does not special case CLOBBERs which means it often > >> bails out unnecessarily, especially when dealing with C++ destructors. > >> Fixed by the obvious continue in the two relevant loops. > >> > >> The (slightly) more complex testcases in the PR need surprisingly more > >> effort but the simple one can be fixed now easily by this patch and I'll > >> work on the others incrementally. > >> > >> Bootstrapped and currently undergoing testsuite run on x86_64-linux. OK > >> if it passes too? > > > > LGTM, btw - how are the clobbers handled during transform? > > it turns out your question is spot on. I assumed that the mini-DCE that > I implemented into IPA-SRA transform would delete but I had a closer > look and it is not invoked on split parameters,only on removed ones. > What was actually happening is that the parameter got remapped to a > default definition of a replacement VAR_DECL and we were thus > gimple-clobbering a pointer pointing to nowhere. The clobber then got > DSEd and so I originally did not notice looking at the optimized dump. > > Still that is of course not ideal and so I added a simple function > removing clobbers when splitting. I as considering adding that > functionality to ipa_param_body_adjustments::mark_dead_statements but > that would make the function harder to read without much gain. > > So thanks again for the remark. The following passes bootstrap and > testing on x86_64-linux. I am running LTO bootstrap now. OK if it > passes? > > Martin > > > > When IPA-SRA detects whether a parameter passed by reference is > written to, it does not special case CLOBBERs which means it often > bails out unnecessarily, especially when dealing with C++ destructors. > Fixed by the obvious continue in the two relevant loops and by adding > a simple function that marks the clobbers in the transformation code > as statements to be removed. > > Not sure if you noticed: I updated bugzilla because the new test fails on arm, and I attached pr110378-1.C.083i.sra there, to help you debug. Thanks, Christophe gcc/ChangeLog: > > 2023-08-04 Martin Jambor <mjambor@suse.cz> > > PR ipa/110378 > * ipa-param-manipulation.h (class ipa_param_body_adjustments): New > members get_ddef_if_exists_and_is_used and mark_clobbers_dead. > * ipa-sra.cc (isra_track_scalar_value_uses): Ignore clobbers. > (ptr_parm_has_nonarg_uses): Likewise. > * ipa-param-manipulation.cc > (ipa_param_body_adjustments::get_ddef_if_exists_and_is_used): New. > (ipa_param_body_adjustments::mark_dead_statements): Move initial > checks to get_ddef_if_exists_and_is_used. > (ipa_param_body_adjustments::mark_clobbers_dead): New. > (ipa_param_body_adjustments::common_initialization): Call > mark_clobbers_dead when splitting. > > gcc/testsuite/ChangeLog: > > 2023-07-31 Martin Jambor <mjambor@suse.cz> > > PR ipa/110378 > * g++.dg/ipa/pr110378-1.C: New test. > --- > gcc/ipa-param-manipulation.cc | 44 +++++++++++++++++++++--- > gcc/ipa-param-manipulation.h | 2 ++ > gcc/ipa-sra.cc | 6 ++-- > gcc/testsuite/g++.dg/ipa/pr110378-1.C | 48 +++++++++++++++++++++++++++ > 4 files changed, 94 insertions(+), 6 deletions(-) > create mode 100644 gcc/testsuite/g++.dg/ipa/pr110378-1.C > > diff --git a/gcc/ipa-param-manipulation.cc b/gcc/ipa-param-manipulation.cc > index a286af7f5d9..4a185ddbdf4 100644 > --- a/gcc/ipa-param-manipulation.cc > +++ b/gcc/ipa-param-manipulation.cc > @@ -1072,6 +1072,20 @@ ipa_param_body_adjustments::carry_over_param (tree > t) > return new_parm; > } > > +/* If DECL is a gimple register that has a default definition SSA name > and that > + has some uses, return the default definition, otherwise return > NULL_TREE. */ > + > +tree > +ipa_param_body_adjustments::get_ddef_if_exists_and_is_used (tree decl) > +{ > + if (!is_gimple_reg (decl)) > + return NULL_TREE; > + tree ddef = ssa_default_def (m_id->src_cfun, decl); > + if (!ddef || has_zero_uses (ddef)) > + return NULL_TREE; > + return ddef; > +} > + > /* 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. Push all removed SSA names > that > @@ -1084,10 +1098,8 @@ 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)) > + tree parm_ddef = get_ddef_if_exists_and_is_used (dead_param); > + if (!parm_ddef) > return; > > auto_vec<tree, 4> stack; > @@ -1169,6 +1181,28 @@ ipa_param_body_adjustments::mark_dead_statements > (tree dead_param, > m_dead_ssa_debug_equiv.put (parm_ddef, dp_ddecl); > } > > +/* Put all clobbers of of dereference of default definition of PARAM into > + m_dead_stmts. */ > + > +void > +ipa_param_body_adjustments::mark_clobbers_dead (tree param) > +{ > + if (!is_gimple_reg (param)) > + return; > + tree ddef = get_ddef_if_exists_and_is_used (param); > + if (!ddef) > + return; > + > + imm_use_iterator imm_iter; > + use_operand_p use_p; > + FOR_EACH_IMM_USE_FAST (use_p, imm_iter, ddef) > + { > + gimple *stmt = USE_STMT (use_p); > + if (gimple_clobber_p (stmt)) > + m_dead_stmts.add (stmt); > + } > +} > + > /* Callback to walk_tree. If REMAP is an SSA_NAME that is present in > hash_map > passed in DATA, replace it with unshared version of what it was mapped > to. > If an SSA argument would be remapped to NULL, the whole operation > needs to > @@ -1504,6 +1538,8 @@ ipa_param_body_adjustments::common_initialization > (tree old_fndecl, > that will guide what not to copy to the new body. */ > if (!split[i]) > mark_dead_statements (m_oparms[i], &ssas_to_process_debug); > + else > + mark_clobbers_dead (m_oparms[i]); > if (MAY_HAVE_DEBUG_STMTS > && is_gimple_reg (m_oparms[i])) > m_reset_debug_decls.safe_push (m_oparms[i]); > diff --git a/gcc/ipa-param-manipulation.h b/gcc/ipa-param-manipulation.h > index 9798eedf0b6..d6a26e9ad36 100644 > --- a/gcc/ipa-param-manipulation.h > +++ b/gcc/ipa-param-manipulation.h > @@ -378,7 +378,9 @@ private: > bool modify_call_stmt (gcall **stmt_p, gimple *orig_stmt); > bool modify_cfun_body (); > void reset_debug_stmts (); > + tree get_ddef_if_exists_and_is_used (tree decl); > void mark_dead_statements (tree dead_param, vec<tree> *debugstack); > + void mark_clobbers_dead (tree dead_param); > bool prepare_debug_expressions (tree dead_ssa); > > /* Declaration of the function that is being transformed. */ > diff --git a/gcc/ipa-sra.cc b/gcc/ipa-sra.cc > index c35e03b7abd..edba364f56e 100644 > --- a/gcc/ipa-sra.cc > +++ b/gcc/ipa-sra.cc > @@ -898,7 +898,8 @@ isra_track_scalar_value_uses (function *fun, > cgraph_node *node, tree name, > > FOR_EACH_IMM_USE_STMT (stmt, imm_iter, name) > { > - if (is_gimple_debug (stmt)) > + if (is_gimple_debug (stmt) > + || gimple_clobber_p (stmt)) > continue; > > /* TODO: We could handle at least const builtin functions like > arithmetic > @@ -1056,7 +1057,8 @@ ptr_parm_has_nonarg_uses (cgraph_node *node, > function *fun, tree parm, > unsigned uses_ok = 0; > use_operand_p use_p; > > - if (is_gimple_debug (stmt)) > + if (is_gimple_debug (stmt) > + || gimple_clobber_p (stmt)) > continue; > > if (gimple_assign_single_p (stmt)) > diff --git a/gcc/testsuite/g++.dg/ipa/pr110378-1.C > b/gcc/testsuite/g++.dg/ipa/pr110378-1.C > new file mode 100644 > index 00000000000..fda7699795a > --- /dev/null > +++ b/gcc/testsuite/g++.dg/ipa/pr110378-1.C > @@ -0,0 +1,48 @@ > +/* { dg-do compile } */ > +/* { dg-options "-O2 -fdump-ipa-sra -fdump-tree-optimized-slim" } */ > + > +/* Test that even though destructors end with clobbering all of *this, it > + should not prevent IPA-SRA. */ > + > +namespace { > + > + class foo > + { > + public: > + short move_offset_of_a; > + int *a; > + foo(int c) > + { > + a = new int[c]; > + a[0] = 4; > + } > + __attribute__((noinline)) ~foo(); > + int f () > + { > + return a[0] + 1; > + } > + }; > + > + volatile int v1 = 4; > + > + __attribute__((noinline)) foo::~foo() > + { > + delete[] a; > + return; > + } > + > + > +} > + > +volatile int v2 = 20; > + > +int test (void) > +{ > + foo shouldnotexist(v2); > + v2 = shouldnotexist.f(); > + return 0; > +} > + > + > +/* { dg-final { scan-ipa-dump "Will split parameter 0" "sra" } } */ > +/* { dg-final { scan-tree-dump-not "shouldnotexist" "optimized" } } */ > -- > 2.41.0 > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] ipa-sra: Don't consider CLOBBERS as writes preventing splitting 2023-08-11 13:04 ` Christophe Lyon @ 2023-08-11 13:50 ` Martin Jambor 2023-08-11 14:19 ` Christophe Lyon 0 siblings, 1 reply; 8+ messages in thread From: Martin Jambor @ 2023-08-11 13:50 UTC (permalink / raw) To: Christophe Lyon; +Cc: Richard Biener, GCC Patches, Jan Hubicka Hello, On Fri, Aug 11 2023, Christophe Lyon wrote: > Hi Martin, > > > On Fri, 4 Aug 2023 at 18:26, Martin Jambor <mjambor@suse.cz> wrote: > >> Hello, >> >> On Wed, Aug 02 2023, Richard Biener wrote: >> > On Mon, Jul 31, 2023 at 7:05 PM Martin Jambor <mjambor@suse.cz> wrote: >> >> >> >> Hi, >> >> >> >> when IPA-SRA detects whether a parameter passed by reference is >> >> written to, it does not special case CLOBBERs which means it often >> >> bails out unnecessarily, especially when dealing with C++ destructors. >> >> Fixed by the obvious continue in the two relevant loops. >> >> >> >> The (slightly) more complex testcases in the PR need surprisingly more >> >> effort but the simple one can be fixed now easily by this patch and I'll >> >> work on the others incrementally. >> >> >> >> Bootstrapped and currently undergoing testsuite run on x86_64-linux. OK >> >> if it passes too? >> > >> > LGTM, btw - how are the clobbers handled during transform? >> >> it turns out your question is spot on. I assumed that the mini-DCE that >> I implemented into IPA-SRA transform would delete but I had a closer >> look and it is not invoked on split parameters,only on removed ones. >> What was actually happening is that the parameter got remapped to a >> default definition of a replacement VAR_DECL and we were thus >> gimple-clobbering a pointer pointing to nowhere. The clobber then got >> DSEd and so I originally did not notice looking at the optimized dump. >> >> Still that is of course not ideal and so I added a simple function >> removing clobbers when splitting. I as considering adding that >> functionality to ipa_param_body_adjustments::mark_dead_statements but >> that would make the function harder to read without much gain. >> >> So thanks again for the remark. The following passes bootstrap and >> testing on x86_64-linux. I am running LTO bootstrap now. OK if it >> passes? >> >> Martin >> >> >> >> When IPA-SRA detects whether a parameter passed by reference is >> written to, it does not special case CLOBBERs which means it often >> bails out unnecessarily, especially when dealing with C++ destructors. >> Fixed by the obvious continue in the two relevant loops and by adding >> a simple function that marks the clobbers in the transformation code >> as statements to be removed. >> >> > Not sure if you noticed: I updated bugzilla because the new test fails on > arm, and I attached pr110378-1.C.083i.sra there, to help you debug. > I am aware and have actually started looking at the issue a while ago. Sorry, I'm only slowly making my way through my TODO list. The difference on 32bit ARM is that the destructor return this pointer, which means that IPA-SRA cannot just split the loaded bit - without any follow-up IPA analysis that the return value is unused which it does not take into account this way. But now that we remove useless returns before splitting it should be doable. Meanwhile, is there a dejagnu target macro for architectures with destructors returning value so that we could xfail the test there? Thanks for bringing my attention to this. Martin > Thanks, > > Christophe > > gcc/ChangeLog: >> >> 2023-08-04 Martin Jambor <mjambor@suse.cz> >> >> PR ipa/110378 >> * ipa-param-manipulation.h (class ipa_param_body_adjustments): New >> members get_ddef_if_exists_and_is_used and mark_clobbers_dead. >> * ipa-sra.cc (isra_track_scalar_value_uses): Ignore clobbers. >> (ptr_parm_has_nonarg_uses): Likewise. >> * ipa-param-manipulation.cc >> (ipa_param_body_adjustments::get_ddef_if_exists_and_is_used): New. >> (ipa_param_body_adjustments::mark_dead_statements): Move initial >> checks to get_ddef_if_exists_and_is_used. >> (ipa_param_body_adjustments::mark_clobbers_dead): New. >> (ipa_param_body_adjustments::common_initialization): Call >> mark_clobbers_dead when splitting. >> >> gcc/testsuite/ChangeLog: >> >> 2023-07-31 Martin Jambor <mjambor@suse.cz> >> >> PR ipa/110378 >> * g++.dg/ipa/pr110378-1.C: New test. >> --- >> gcc/ipa-param-manipulation.cc | 44 +++++++++++++++++++++--- >> gcc/ipa-param-manipulation.h | 2 ++ >> gcc/ipa-sra.cc | 6 ++-- >> gcc/testsuite/g++.dg/ipa/pr110378-1.C | 48 +++++++++++++++++++++++++++ >> 4 files changed, 94 insertions(+), 6 deletions(-) >> create mode 100644 gcc/testsuite/g++.dg/ipa/pr110378-1.C >> >> diff --git a/gcc/ipa-param-manipulation.cc b/gcc/ipa-param-manipulation.cc >> index a286af7f5d9..4a185ddbdf4 100644 >> --- a/gcc/ipa-param-manipulation.cc >> +++ b/gcc/ipa-param-manipulation.cc >> @@ -1072,6 +1072,20 @@ ipa_param_body_adjustments::carry_over_param (tree >> t) >> return new_parm; >> } >> >> +/* If DECL is a gimple register that has a default definition SSA name >> and that >> + has some uses, return the default definition, otherwise return >> NULL_TREE. */ >> + >> +tree >> +ipa_param_body_adjustments::get_ddef_if_exists_and_is_used (tree decl) >> +{ >> + if (!is_gimple_reg (decl)) >> + return NULL_TREE; >> + tree ddef = ssa_default_def (m_id->src_cfun, decl); >> + if (!ddef || has_zero_uses (ddef)) >> + return NULL_TREE; >> + return ddef; >> +} >> + >> /* 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. Push all removed SSA names >> that >> @@ -1084,10 +1098,8 @@ 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)) >> + tree parm_ddef = get_ddef_if_exists_and_is_used (dead_param); >> + if (!parm_ddef) >> return; >> >> auto_vec<tree, 4> stack; >> @@ -1169,6 +1181,28 @@ ipa_param_body_adjustments::mark_dead_statements >> (tree dead_param, >> m_dead_ssa_debug_equiv.put (parm_ddef, dp_ddecl); >> } >> >> +/* Put all clobbers of of dereference of default definition of PARAM into >> + m_dead_stmts. */ >> + >> +void >> +ipa_param_body_adjustments::mark_clobbers_dead (tree param) >> +{ >> + if (!is_gimple_reg (param)) >> + return; >> + tree ddef = get_ddef_if_exists_and_is_used (param); >> + if (!ddef) >> + return; >> + >> + imm_use_iterator imm_iter; >> + use_operand_p use_p; >> + FOR_EACH_IMM_USE_FAST (use_p, imm_iter, ddef) >> + { >> + gimple *stmt = USE_STMT (use_p); >> + if (gimple_clobber_p (stmt)) >> + m_dead_stmts.add (stmt); >> + } >> +} >> + >> /* Callback to walk_tree. If REMAP is an SSA_NAME that is present in >> hash_map >> passed in DATA, replace it with unshared version of what it was mapped >> to. >> If an SSA argument would be remapped to NULL, the whole operation >> needs to >> @@ -1504,6 +1538,8 @@ ipa_param_body_adjustments::common_initialization >> (tree old_fndecl, >> that will guide what not to copy to the new body. */ >> if (!split[i]) >> mark_dead_statements (m_oparms[i], &ssas_to_process_debug); >> + else >> + mark_clobbers_dead (m_oparms[i]); >> if (MAY_HAVE_DEBUG_STMTS >> && is_gimple_reg (m_oparms[i])) >> m_reset_debug_decls.safe_push (m_oparms[i]); >> diff --git a/gcc/ipa-param-manipulation.h b/gcc/ipa-param-manipulation.h >> index 9798eedf0b6..d6a26e9ad36 100644 >> --- a/gcc/ipa-param-manipulation.h >> +++ b/gcc/ipa-param-manipulation.h >> @@ -378,7 +378,9 @@ private: >> bool modify_call_stmt (gcall **stmt_p, gimple *orig_stmt); >> bool modify_cfun_body (); >> void reset_debug_stmts (); >> + tree get_ddef_if_exists_and_is_used (tree decl); >> void mark_dead_statements (tree dead_param, vec<tree> *debugstack); >> + void mark_clobbers_dead (tree dead_param); >> bool prepare_debug_expressions (tree dead_ssa); >> >> /* Declaration of the function that is being transformed. */ >> diff --git a/gcc/ipa-sra.cc b/gcc/ipa-sra.cc >> index c35e03b7abd..edba364f56e 100644 >> --- a/gcc/ipa-sra.cc >> +++ b/gcc/ipa-sra.cc >> @@ -898,7 +898,8 @@ isra_track_scalar_value_uses (function *fun, >> cgraph_node *node, tree name, >> >> FOR_EACH_IMM_USE_STMT (stmt, imm_iter, name) >> { >> - if (is_gimple_debug (stmt)) >> + if (is_gimple_debug (stmt) >> + || gimple_clobber_p (stmt)) >> continue; >> >> /* TODO: We could handle at least const builtin functions like >> arithmetic >> @@ -1056,7 +1057,8 @@ ptr_parm_has_nonarg_uses (cgraph_node *node, >> function *fun, tree parm, >> unsigned uses_ok = 0; >> use_operand_p use_p; >> >> - if (is_gimple_debug (stmt)) >> + if (is_gimple_debug (stmt) >> + || gimple_clobber_p (stmt)) >> continue; >> >> if (gimple_assign_single_p (stmt)) >> diff --git a/gcc/testsuite/g++.dg/ipa/pr110378-1.C >> b/gcc/testsuite/g++.dg/ipa/pr110378-1.C >> new file mode 100644 >> index 00000000000..fda7699795a >> --- /dev/null >> +++ b/gcc/testsuite/g++.dg/ipa/pr110378-1.C >> @@ -0,0 +1,48 @@ >> +/* { dg-do compile } */ >> +/* { dg-options "-O2 -fdump-ipa-sra -fdump-tree-optimized-slim" } */ >> + >> +/* Test that even though destructors end with clobbering all of *this, it >> + should not prevent IPA-SRA. */ >> + >> +namespace { >> + >> + class foo >> + { >> + public: >> + short move_offset_of_a; >> + int *a; >> + foo(int c) >> + { >> + a = new int[c]; >> + a[0] = 4; >> + } >> + __attribute__((noinline)) ~foo(); >> + int f () >> + { >> + return a[0] + 1; >> + } >> + }; >> + >> + volatile int v1 = 4; >> + >> + __attribute__((noinline)) foo::~foo() >> + { >> + delete[] a; >> + return; >> + } >> + >> + >> +} >> + >> +volatile int v2 = 20; >> + >> +int test (void) >> +{ >> + foo shouldnotexist(v2); >> + v2 = shouldnotexist.f(); >> + return 0; >> +} >> + >> + >> +/* { dg-final { scan-ipa-dump "Will split parameter 0" "sra" } } */ >> +/* { dg-final { scan-tree-dump-not "shouldnotexist" "optimized" } } */ >> -- >> 2.41.0 >> >> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] ipa-sra: Don't consider CLOBBERS as writes preventing splitting 2023-08-11 13:50 ` Martin Jambor @ 2023-08-11 14:19 ` Christophe Lyon 0 siblings, 0 replies; 8+ messages in thread From: Christophe Lyon @ 2023-08-11 14:19 UTC (permalink / raw) To: Martin Jambor; +Cc: Richard Biener, GCC Patches, Jan Hubicka [-- Attachment #1: Type: text/plain, Size: 11615 bytes --] On Fri, 11 Aug 2023 at 15:50, Martin Jambor <mjambor@suse.cz> wrote: > Hello, > > On Fri, Aug 11 2023, Christophe Lyon wrote: > > Hi Martin, > > > > > > On Fri, 4 Aug 2023 at 18:26, Martin Jambor <mjambor@suse.cz> wrote: > > > >> Hello, > >> > >> On Wed, Aug 02 2023, Richard Biener wrote: > >> > On Mon, Jul 31, 2023 at 7:05 PM Martin Jambor <mjambor@suse.cz> > wrote: > >> >> > >> >> Hi, > >> >> > >> >> when IPA-SRA detects whether a parameter passed by reference is > >> >> written to, it does not special case CLOBBERs which means it often > >> >> bails out unnecessarily, especially when dealing with C++ > destructors. > >> >> Fixed by the obvious continue in the two relevant loops. > >> >> > >> >> The (slightly) more complex testcases in the PR need surprisingly > more > >> >> effort but the simple one can be fixed now easily by this patch and > I'll > >> >> work on the others incrementally. > >> >> > >> >> Bootstrapped and currently undergoing testsuite run on > x86_64-linux. OK > >> >> if it passes too? > >> > > >> > LGTM, btw - how are the clobbers handled during transform? > >> > >> it turns out your question is spot on. I assumed that the mini-DCE that > >> I implemented into IPA-SRA transform would delete but I had a closer > >> look and it is not invoked on split parameters,only on removed ones. > >> What was actually happening is that the parameter got remapped to a > >> default definition of a replacement VAR_DECL and we were thus > >> gimple-clobbering a pointer pointing to nowhere. The clobber then got > >> DSEd and so I originally did not notice looking at the optimized dump. > >> > >> Still that is of course not ideal and so I added a simple function > >> removing clobbers when splitting. I as considering adding that > >> functionality to ipa_param_body_adjustments::mark_dead_statements but > >> that would make the function harder to read without much gain. > >> > >> So thanks again for the remark. The following passes bootstrap and > >> testing on x86_64-linux. I am running LTO bootstrap now. OK if it > >> passes? > >> > >> Martin > >> > >> > >> > >> When IPA-SRA detects whether a parameter passed by reference is > >> written to, it does not special case CLOBBERs which means it often > >> bails out unnecessarily, especially when dealing with C++ destructors. > >> Fixed by the obvious continue in the two relevant loops and by adding > >> a simple function that marks the clobbers in the transformation code > >> as statements to be removed. > >> > >> > > Not sure if you noticed: I updated bugzilla because the new test fails on > > arm, and I attached pr110378-1.C.083i.sra there, to help you debug. > > > > I am aware and have actually started looking at the issue a while ago. > Sorry, I'm only slowly making my way through my TODO list. > No worries, thanks for confirming you are aware of the problem ;-) > > The difference on 32bit ARM is that the destructor return this pointer, > which means that IPA-SRA cannot just split the loaded bit - without any > follow-up IPA analysis that the return value is unused which it does not > take into account this way. But now that we remove useless returns > before splitting it should be doable. > > Meanwhile, is there a dejagnu target macro for architectures with > destructors returning value so that we could xfail the test there? > I'm not aware of any at quick glance > > Thanks for bringing my attention to this. > > Martin > > Thanks, Christophe > > > > Thanks, > > > > Christophe > > > > gcc/ChangeLog: > >> > >> 2023-08-04 Martin Jambor <mjambor@suse.cz> > >> > >> PR ipa/110378 > >> * ipa-param-manipulation.h (class ipa_param_body_adjustments): > New > >> members get_ddef_if_exists_and_is_used and mark_clobbers_dead. > >> * ipa-sra.cc (isra_track_scalar_value_uses): Ignore clobbers. > >> (ptr_parm_has_nonarg_uses): Likewise. > >> * ipa-param-manipulation.cc > >> (ipa_param_body_adjustments::get_ddef_if_exists_and_is_used): > New. > >> (ipa_param_body_adjustments::mark_dead_statements): Move initial > >> checks to get_ddef_if_exists_and_is_used. > >> (ipa_param_body_adjustments::mark_clobbers_dead): New. > >> (ipa_param_body_adjustments::common_initialization): Call > >> mark_clobbers_dead when splitting. > >> > >> gcc/testsuite/ChangeLog: > >> > >> 2023-07-31 Martin Jambor <mjambor@suse.cz> > >> > >> PR ipa/110378 > >> * g++.dg/ipa/pr110378-1.C: New test. > >> --- > >> gcc/ipa-param-manipulation.cc | 44 +++++++++++++++++++++--- > >> gcc/ipa-param-manipulation.h | 2 ++ > >> gcc/ipa-sra.cc | 6 ++-- > >> gcc/testsuite/g++.dg/ipa/pr110378-1.C | 48 +++++++++++++++++++++++++++ > >> 4 files changed, 94 insertions(+), 6 deletions(-) > >> create mode 100644 gcc/testsuite/g++.dg/ipa/pr110378-1.C > >> > >> diff --git a/gcc/ipa-param-manipulation.cc > b/gcc/ipa-param-manipulation.cc > >> index a286af7f5d9..4a185ddbdf4 100644 > >> --- a/gcc/ipa-param-manipulation.cc > >> +++ b/gcc/ipa-param-manipulation.cc > >> @@ -1072,6 +1072,20 @@ ipa_param_body_adjustments::carry_over_param > (tree > >> t) > >> return new_parm; > >> } > >> > >> +/* If DECL is a gimple register that has a default definition SSA name > >> and that > >> + has some uses, return the default definition, otherwise return > >> NULL_TREE. */ > >> + > >> +tree > >> +ipa_param_body_adjustments::get_ddef_if_exists_and_is_used (tree decl) > >> +{ > >> + if (!is_gimple_reg (decl)) > >> + return NULL_TREE; > >> + tree ddef = ssa_default_def (m_id->src_cfun, decl); > >> + if (!ddef || has_zero_uses (ddef)) > >> + return NULL_TREE; > >> + return ddef; > >> +} > >> + > >> /* 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. Push all removed SSA > names > >> that > >> @@ -1084,10 +1098,8 @@ 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)) > >> + tree parm_ddef = get_ddef_if_exists_and_is_used (dead_param); > >> + if (!parm_ddef) > >> return; > >> > >> auto_vec<tree, 4> stack; > >> @@ -1169,6 +1181,28 @@ ipa_param_body_adjustments::mark_dead_statements > >> (tree dead_param, > >> m_dead_ssa_debug_equiv.put (parm_ddef, dp_ddecl); > >> } > >> > >> +/* Put all clobbers of of dereference of default definition of PARAM > into > >> + m_dead_stmts. */ > >> + > >> +void > >> +ipa_param_body_adjustments::mark_clobbers_dead (tree param) > >> +{ > >> + if (!is_gimple_reg (param)) > >> + return; > >> + tree ddef = get_ddef_if_exists_and_is_used (param); > >> + if (!ddef) > >> + return; > >> + > >> + imm_use_iterator imm_iter; > >> + use_operand_p use_p; > >> + FOR_EACH_IMM_USE_FAST (use_p, imm_iter, ddef) > >> + { > >> + gimple *stmt = USE_STMT (use_p); > >> + if (gimple_clobber_p (stmt)) > >> + m_dead_stmts.add (stmt); > >> + } > >> +} > >> + > >> /* Callback to walk_tree. If REMAP is an SSA_NAME that is present in > >> hash_map > >> passed in DATA, replace it with unshared version of what it was > mapped > >> to. > >> If an SSA argument would be remapped to NULL, the whole operation > >> needs to > >> @@ -1504,6 +1538,8 @@ ipa_param_body_adjustments::common_initialization > >> (tree old_fndecl, > >> that will guide what not to copy to the new body. */ > >> if (!split[i]) > >> mark_dead_statements (m_oparms[i], > &ssas_to_process_debug); > >> + else > >> + mark_clobbers_dead (m_oparms[i]); > >> if (MAY_HAVE_DEBUG_STMTS > >> && is_gimple_reg (m_oparms[i])) > >> m_reset_debug_decls.safe_push (m_oparms[i]); > >> diff --git a/gcc/ipa-param-manipulation.h b/gcc/ipa-param-manipulation.h > >> index 9798eedf0b6..d6a26e9ad36 100644 > >> --- a/gcc/ipa-param-manipulation.h > >> +++ b/gcc/ipa-param-manipulation.h > >> @@ -378,7 +378,9 @@ private: > >> bool modify_call_stmt (gcall **stmt_p, gimple *orig_stmt); > >> bool modify_cfun_body (); > >> void reset_debug_stmts (); > >> + tree get_ddef_if_exists_and_is_used (tree decl); > >> void mark_dead_statements (tree dead_param, vec<tree> *debugstack); > >> + void mark_clobbers_dead (tree dead_param); > >> bool prepare_debug_expressions (tree dead_ssa); > >> > >> /* Declaration of the function that is being transformed. */ > >> diff --git a/gcc/ipa-sra.cc b/gcc/ipa-sra.cc > >> index c35e03b7abd..edba364f56e 100644 > >> --- a/gcc/ipa-sra.cc > >> +++ b/gcc/ipa-sra.cc > >> @@ -898,7 +898,8 @@ isra_track_scalar_value_uses (function *fun, > >> cgraph_node *node, tree name, > >> > >> FOR_EACH_IMM_USE_STMT (stmt, imm_iter, name) > >> { > >> - if (is_gimple_debug (stmt)) > >> + if (is_gimple_debug (stmt) > >> + || gimple_clobber_p (stmt)) > >> continue; > >> > >> /* TODO: We could handle at least const builtin functions like > >> arithmetic > >> @@ -1056,7 +1057,8 @@ ptr_parm_has_nonarg_uses (cgraph_node *node, > >> function *fun, tree parm, > >> unsigned uses_ok = 0; > >> use_operand_p use_p; > >> > >> - if (is_gimple_debug (stmt)) > >> + if (is_gimple_debug (stmt) > >> + || gimple_clobber_p (stmt)) > >> continue; > >> > >> if (gimple_assign_single_p (stmt)) > >> diff --git a/gcc/testsuite/g++.dg/ipa/pr110378-1.C > >> b/gcc/testsuite/g++.dg/ipa/pr110378-1.C > >> new file mode 100644 > >> index 00000000000..fda7699795a > >> --- /dev/null > >> +++ b/gcc/testsuite/g++.dg/ipa/pr110378-1.C > >> @@ -0,0 +1,48 @@ > >> +/* { dg-do compile } */ > >> +/* { dg-options "-O2 -fdump-ipa-sra -fdump-tree-optimized-slim" } */ > >> + > >> +/* Test that even though destructors end with clobbering all of *this, > it > >> + should not prevent IPA-SRA. */ > >> + > >> +namespace { > >> + > >> + class foo > >> + { > >> + public: > >> + short move_offset_of_a; > >> + int *a; > >> + foo(int c) > >> + { > >> + a = new int[c]; > >> + a[0] = 4; > >> + } > >> + __attribute__((noinline)) ~foo(); > >> + int f () > >> + { > >> + return a[0] + 1; > >> + } > >> + }; > >> + > >> + volatile int v1 = 4; > >> + > >> + __attribute__((noinline)) foo::~foo() > >> + { > >> + delete[] a; > >> + return; > >> + } > >> + > >> + > >> +} > >> + > >> +volatile int v2 = 20; > >> + > >> +int test (void) > >> +{ > >> + foo shouldnotexist(v2); > >> + v2 = shouldnotexist.f(); > >> + return 0; > >> +} > >> + > >> + > >> +/* { dg-final { scan-ipa-dump "Will split parameter 0" "sra" } } */ > >> +/* { dg-final { scan-tree-dump-not "shouldnotexist" "optimized" } } */ > >> -- > >> 2.41.0 > >> > >> > ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2023-08-11 14:19 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-07-31 17:04 [PATCH] ipa-sra: Don't consider CLOBBERS as writes preventing splitting Martin Jambor 2023-08-02 8:03 ` Richard Biener 2023-08-03 10:19 ` Jan Hubicka 2023-08-04 16:26 ` Martin Jambor 2023-08-04 17:57 ` Richard Biener 2023-08-11 13:04 ` Christophe Lyon 2023-08-11 13:50 ` Martin Jambor 2023-08-11 14:19 ` Christophe Lyon
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).