Re: [PATCH] ipa: Self-DCE of uses of removed call LHSs (PR 108007)

[Martin Jambor <mjambor@suse.cz>]

Hello,

and ping.

Thanks,

Martin


On Fri, Sep 01 2023, Martin Jambor wrote:
> Hello
>
> and ping.
>
> Thanks,
>
> Martin
>
>
> On Fri, May 12 2023, Martin Jambor wrote:
>> Hi,
>>
>> PR 108007 is another manifestation where we rely on DCE to clean-up
>> after IPA-SRA and if the user explicitely switches DCE off, IPA-SRA
>> can leave behind statements which are fed uninitialized values and
>> trap, even though their results are themselves never used.
>>
>> I have already fixed this for unused parameters in callees, this bug
>> shows that almost the same thing can happen for removed returns, on
>> the side of callers.  This means that the issue has to be fixed
>> elsewhere, in call redirection.  This patch adds a function which
>> recursivewly looks for uses of operations fed specific SSA names and
>> removes them all.
>>
>> That would have been easy if it wasn't for debug statements during
>> tree-inline (from which call redirection is also invoked).  Debug
>> statements are decoupled from the rest at this point and iterating
>> over uses of SSAs does not bring them up.  During tree-inline they are
>> handled especially at the end, I assume in order to make sure that
>> relative ordering of UIDs are the same with and without debug info.
>>
>> This means that during tree-inline we need to make a hash of killed
>> SSAs, that we already have in copy_body_data, available to the
>> function making the purging.  So the patch duly does also that, making
>> the interface slightly ugly.
>>
>> Bootstrapped and tested on x86_64-linux.  OK for master?  (I am not sure
>> the problem is grave enough to warrant backporting to release branches
>> but can do that as well if people think I should.)
>>
>> Thanks,
>>
>> Martin
>>
>>
>> gcc/ChangeLog:
>>
>> 2023-05-11  Martin Jambor  <mjambor@suse.cz>
>>
>> 	PR ipa/108007
>> 	* cgraph.h (cgraph_edge): Add a parameter to
>> 	redirect_call_stmt_to_callee.
>> 	* ipa-param-manipulation.h (ipa_param_adjustments): Added a
>> 	parameter to modify_call.
>> 	* cgraph.cc (cgraph_edge::redirect_call_stmt_to_callee): New
>> 	parameter killed_ssas, pass it to padjs->modify_call.
>> 	* ipa-param-manipulation.cc (purge_transitive_uses): New function.
>> 	(ipa_param_adjustments::modify_call): New parameter killed_ssas.
>> 	Instead of substitutin uses, invoke purge_transitive_uses.  If
>> 	hash of killed SSAs has not been provided, create a temporary one
>> 	and release SSAs that have been added to it.
>> 	* tree-inline.cc (redirect_all_calls): Create
>> 	id->killed_new_ssa_names earlier, pass it to edge redirection,
>> 	adjust a comment.
>> 	(copy_body): Release SSAs in id->killed_new_ssa_names.
>>
>> gcc/testsuite/ChangeLog:
>>
>> 2023-05-11  Martin Jambor  <mjambor@suse.cz>
>>
>> 	PR ipa/108007
>> 	* gcc.dg/ipa/pr108007.c: New test.
>> ---
>>  gcc/cgraph.cc                       | 10 +++-
>>  gcc/cgraph.h                        |  9 ++-
>>  gcc/ipa-param-manipulation.cc       | 85 +++++++++++++++++++++--------
>>  gcc/ipa-param-manipulation.h        |  3 +-
>>  gcc/testsuite/gcc.dg/ipa/pr108007.c | 32 +++++++++++
>>  gcc/tree-inline.cc                  | 28 ++++++----
>>  6 files changed, 129 insertions(+), 38 deletions(-)
>>  create mode 100644 gcc/testsuite/gcc.dg/ipa/pr108007.c
>>
>> diff --git a/gcc/cgraph.cc b/gcc/cgraph.cc
>> index e8f9bec8227..5e923bf0557 100644
>> --- a/gcc/cgraph.cc
>> +++ b/gcc/cgraph.cc
>> @@ -1403,11 +1403,17 @@ cgraph_edge::redirect_callee (cgraph_node *n)
>>     speculative indirect call, remove "speculative" of the indirect call and
>>     also redirect stmt to it's final direct target.
>>  
>> +   When called from within tree-inline, KILLED_SSAs has to contain the pointer
>> +   to killed_new_ssa_names within the copy_body_data structure and SSAs
>> +   discovered to be useless (if LHS is removed) will be added to it, otherwise
>> +   it needs to be NULL.
>> +
>>     It is up to caller to iteratively transform each "speculative"
>>     direct call as appropriate.  */
>>  
>>  gimple *
>> -cgraph_edge::redirect_call_stmt_to_callee (cgraph_edge *e)
>> +cgraph_edge::redirect_call_stmt_to_callee (cgraph_edge *e,
>> +					   hash_set <tree> *killed_ssas)
>>  {
>>    tree decl = gimple_call_fndecl (e->call_stmt);
>>    gcall *new_stmt;
>> @@ -1527,7 +1533,7 @@ cgraph_edge::redirect_call_stmt_to_callee (cgraph_edge *e)
>>  	remove_stmt_from_eh_lp (e->call_stmt);
>>  
>>        tree old_fntype = gimple_call_fntype (e->call_stmt);
>> -      new_stmt = padjs->modify_call (e, false);
>> +      new_stmt = padjs->modify_call (e, false, killed_ssas);
>>        cgraph_node *origin = e->callee;
>>        while (origin->clone_of)
>>  	origin = origin->clone_of;
>> diff --git a/gcc/cgraph.h b/gcc/cgraph.h
>> index f5f54769eda..c1a3691b6f5 100644
>> --- a/gcc/cgraph.h
>> +++ b/gcc/cgraph.h
>> @@ -1833,9 +1833,16 @@ public:
>>       speculative indirect call, remove "speculative" of the indirect call and
>>       also redirect stmt to it's final direct target.
>>  
>> +     When called from within tree-inline, KILLED_SSAs has to contain the
>> +     pointer to killed_new_ssa_names within the copy_body_data structure and
>> +     SSAs discovered to be useless (if LHS is removed) will be added to it,
>> +     otherwise it needs to be NULL.
>> +
>>       It is up to caller to iteratively transform each "speculative"
>>       direct call as appropriate.  */
>> -  static gimple *redirect_call_stmt_to_callee (cgraph_edge *e);
>> +  static gimple *redirect_call_stmt_to_callee (cgraph_edge *e,
>> +					       hash_set <tree>
>> +					       *killed_ssas = nullptr);
>>  
>>    /* Create clone of edge in the node N represented
>>       by CALL_EXPR the callgraph.  */
>> diff --git a/gcc/ipa-param-manipulation.cc b/gcc/ipa-param-manipulation.cc
>> index a286af7f5d9..bf2adeb4bd6 100644
>> --- a/gcc/ipa-param-manipulation.cc
>> +++ b/gcc/ipa-param-manipulation.cc
>> @@ -593,14 +593,63 @@ isra_get_ref_base_and_offset (tree expr, tree *base_p, unsigned *unit_offset_p)
>>    return true;
>>  }
>>  
>> +/* Remove all statements that use NAME and transitively those that use the
>> +   result of such statements.  KILLED_SSAS contains the SSA_NAMEs that are
>> +   already being or have been processed and new ones need to be added to it.
>> +   The funtction only has to process situations handled by
>> +   ssa_name_only_returned_p in ipa-sra.cc with the exception that it can assume
>> +   it must never reach a use in a return statement.  */
>> +
>> +static void
>> +purge_transitive_uses (tree name, hash_set <tree> *killed_ssas)
>> +{
>> +  imm_use_iterator imm_iter;
>> +  gimple *stmt;
>> +
>> +  FOR_EACH_IMM_USE_STMT (stmt, imm_iter, name)
>> +    {
>> +      if (gimple_debug_bind_p (stmt))
>> +	{
>> +	  /* When runing within tree-inline, we will never end up here but
>> +	     adding the SSAs to killed_ssas will do the trick in this case and
>> +	     the respective debug statements will get reset. */
>> +
>> +	  gimple_debug_bind_reset_value (stmt);
>> +	  update_stmt (stmt);
>> +	  continue;
>> +	}
>> +
>> +      tree lhs = NULL_TREE;
>> +      if (is_gimple_assign (stmt))
>> +	lhs = gimple_assign_lhs (stmt);
>> +      else if (gimple_code (stmt) == GIMPLE_PHI)
>> +	lhs = gimple_phi_result (stmt);
>> +      gcc_assert (lhs
>> +		  && (TREE_CODE (lhs) == SSA_NAME)
>> +		  && !gimple_vdef (stmt));
>> +
>> +      if (!killed_ssas->contains (lhs))
>> +	{
>> +	  killed_ssas->add (lhs);
>> +	  purge_transitive_uses (lhs, killed_ssas);
>> +	  gimple_stmt_iterator gsi = gsi_for_stmt (stmt);
>> +	  gsi_remove (&gsi, true);
>> +	}
>> +    }
>> +}
>> +
>>  /* Modify actual arguments of a function call in statement currently belonging
>>     to CS, and make it call CS->callee->decl.  Return the new statement that
>>     replaced the old one.  When invoked, cfun and current_function_decl have to
>> -   be set to the caller.  */
>> +   be set to the caller.  When called from within tree-inline, KILLED_SSAs has
>> +   to contain the pointer to killed_new_ssa_names within the copy_body_data
>> +   structure and SSAs discovered to be useless (if LHS is removed) will be
>> +   added to it, otherwise it needs to be NULL.  */
>>  
>>  gcall *
>>  ipa_param_adjustments::modify_call (cgraph_edge *cs,
>> -				    bool update_references)
>> +				    bool update_references,
>> +				    hash_set <tree> *killed_ssas)
>>  {
>>    gcall *stmt = cs->call_stmt;
>>    tree callee_decl = cs->callee->decl;
>> @@ -910,32 +959,20 @@ ipa_param_adjustments::modify_call (cgraph_edge *cs,
>>  
>>    gcall *new_stmt = gimple_build_call_vec (callee_decl, vargs);
>>  
>> -  tree ssa_to_remove = NULL;
>> +  hash_set <tree> *ssas_to_remove = NULL;
>>    if (tree lhs = gimple_call_lhs (stmt))
>>      {
>>        if (!m_skip_return)
>>  	gimple_call_set_lhs (new_stmt, lhs);
>>        else if (TREE_CODE (lhs) == SSA_NAME)
>>  	{
>> -	  /* LHS should now by a default-def SSA.  Unfortunately default-def
>> -	     SSA_NAMEs need a backing variable (or at least some code examining
>> -	     SSAs assumes it is non-NULL).  So we either have to re-use the
>> -	     decl we have at hand or introdice a new one.  */
>> -	  tree repl = create_tmp_var (TREE_TYPE (lhs), "removed_return");
>> -	  repl = get_or_create_ssa_default_def (cfun, repl);
>> -	  SSA_NAME_IS_DEFAULT_DEF (repl) = true;
>> -	  imm_use_iterator ui;
>> -	  use_operand_p use_p;
>> -	  gimple *using_stmt;
>> -	  FOR_EACH_IMM_USE_STMT (using_stmt, ui, lhs)
>> +	  if (!killed_ssas)
>>  	    {
>> -	      FOR_EACH_IMM_USE_ON_STMT (use_p, ui)
>> -		{
>> -		  SET_USE (use_p, repl);
>> -		}
>> -	      update_stmt (using_stmt);
>> +	      ssas_to_remove = new hash_set<tree> (8);
>> +	      killed_ssas = ssas_to_remove;
>>  	    }
>> -	  ssa_to_remove = lhs;
>> +	  killed_ssas->add (lhs);
>> +	  purge_transitive_uses (lhs, killed_ssas);
>>  	}
>>      }
>>  
>> @@ -954,8 +991,12 @@ ipa_param_adjustments::modify_call (cgraph_edge *cs,
>>        fprintf (dump_file, "\n");
>>      }
>>    gsi_replace (&gsi, new_stmt, true);
>> -  if (ssa_to_remove)
>> -    release_ssa_name (ssa_to_remove);
>> +  if (ssas_to_remove)
>> +    {
>> +      for (tree sn : *ssas_to_remove)
>> +	release_ssa_name (sn);
>> +      delete ssas_to_remove;
>> +    }
>>    if (update_references)
>>      do
>>        {
>> diff --git a/gcc/ipa-param-manipulation.h b/gcc/ipa-param-manipulation.h
>> index 9798eedf0b6..5b2f90f8307 100644
>> --- a/gcc/ipa-param-manipulation.h
>> +++ b/gcc/ipa-param-manipulation.h
>> @@ -224,7 +224,8 @@ public:
>>  
>>    /* Modify a call statement arguments (and possibly remove the return value)
>>       as described in the data fields of this class.  */
>> -  gcall *modify_call (cgraph_edge *cs, bool update_references);
>> +  gcall *modify_call (cgraph_edge *cs, bool update_references,
>> +		      hash_set <tree> *killed_ssas);
>>    /* Return if the first parameter is left intact.  */
>>    bool first_param_intact_p ();
>>    /* Build a function type corresponding to the modified call.  */
>> diff --git a/gcc/testsuite/gcc.dg/ipa/pr108007.c b/gcc/testsuite/gcc.dg/ipa/pr108007.c
>> new file mode 100644
>> index 00000000000..77fc95975cf
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.dg/ipa/pr108007.c
>> @@ -0,0 +1,32 @@
>> +/* { dg-do run } */
>> +/* { dg-options "-Os -fno-dce -fno-tree-dce -g" } */
>> +
>> +/* This tests that when IPA-SRA removes a LHS of a call statement which, in the
>> +   original source, is fed into a useless operation which however can trap when
>> +   given nonsensical input, that we remove it even when the user has turned off
>> +   normal DCE.  */
>> +
>> +int a, b, d, e, f = 10000000, h;
>> +short c, g;
>> +static int *i() {
>> +  g = f;
>> + L:
>> +  h = e = ~g;
>> +  g = ~f % g & e;
>> +  if (!g)
>> +    goto L;
>> +  c++;
>> +  while (g < 1)
>> +    ;
>> +  return &a;
>> +}
>> +static void k() {
>> +  int *l, m = 2;
>> +  l = i();
>> +  for (; d < 1; d++)
>> +    m |= *l >= b;
>> +}
>> +int main() {
>> +  k();
>> +  return 0;
>> +}
>> diff --git a/gcc/tree-inline.cc b/gcc/tree-inline.cc
>> index 63a19f8d1d8..1c20e9545d1 100644
>> --- a/gcc/tree-inline.cc
>> +++ b/gcc/tree-inline.cc
>> @@ -2982,20 +2982,19 @@ redirect_all_calls (copy_body_data * id, basic_block bb)
>>  	  struct cgraph_edge *edge = id->dst_node->get_edge (stmt);
>>  	  if (edge)
>>  	    {
>> +	      if (!id->killed_new_ssa_names)
>> +		id->killed_new_ssa_names = new hash_set<tree> (16);
>>  	      gimple *new_stmt
>> -		= cgraph_edge::redirect_call_stmt_to_callee (edge);
>> -	      /* If IPA-SRA transformation, run as part of edge redirection,
>> -		 removed the LHS because it is unused, save it to
>> -		 killed_new_ssa_names so that we can prune it from debug
>> -		 statements.  */
>> +		= cgraph_edge::redirect_call_stmt_to_callee (edge,
>> +		    id->killed_new_ssa_names);
>>  	      if (old_lhs
>>  		  && TREE_CODE (old_lhs) == SSA_NAME
>>  		  && !gimple_call_lhs (new_stmt))
>> -		{
>> -		  if (!id->killed_new_ssa_names)
>> -		    id->killed_new_ssa_names = new hash_set<tree> (16);
>> -		  id->killed_new_ssa_names->add (old_lhs);
>> -		}
>> +		/* In case of IPA-SRA removing the LHS, the name should have
>> +		   been already added to the hash.  But in case of redirecting
>> +		   to builtin_unreachable it was not and the name still should
>> +		   be pruned from debug statements.  */
>> +		id->killed_new_ssa_names->add (old_lhs);
>>  
>>  	      if (stmt == last && id->call_stmt && maybe_clean_eh_stmt (stmt))
>>  		gimple_purge_dead_eh_edges (bb);
>> @@ -3322,8 +3321,13 @@ copy_body (copy_body_data *id,
>>    body = copy_cfg_body (id, entry_block_map, exit_block_map,
>>  			new_entry);
>>    copy_debug_stmts (id);
>> -  delete id->killed_new_ssa_names;
>> -  id->killed_new_ssa_names = NULL;
>> +  if (id->killed_new_ssa_names)
>> +    {
>> +      for (tree sn : *id->killed_new_ssa_names)
>> +	release_ssa_name (sn);
>> +      delete id->killed_new_ssa_names;
>> +      id->killed_new_ssa_names = NULL;
>> +    }
>>  
>>    return body;
>>  }
>> -- 
>> 2.40.0