Re: [PATCH] ipa-sra: Don't consider CLOBBERS as writes preventing splitting

[Richard Biener via Gcc-patches]

> Am 04.08.2023 um 18:26 schrieb Martin Jambor <mjam...@suse.cz>:
> 
> 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?

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