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.
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
>
>
