On Fri, 11 Aug 2023 at 15:50, Martin Jambor wrote: > Hello, > > On Fri, Aug 11 2023, Christophe Lyon wrote: > > Hi Martin, > > > > > > On Fri, 4 Aug 2023 at 18:26, Martin Jambor wrote: > > > >> Hello, > >> > >> On Wed, Aug 02 2023, Richard Biener wrote: > >> > On Mon, Jul 31, 2023 at 7:05 PM Martin Jambor > 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 > >> > >> 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 > >> > >> 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 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 *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 > >> > >> >