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