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

[Martin Jambor <mjambor@suse.cz>]

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