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