Re: Fix noreturn related LTO ICE

[Richard Guenther <rguenther at suse dot de>]

On Mon, 31 May 2010, Jan Hubicka wrote:

> Hi,
> the attached testcase cause enable checking ICE since with decl merging
> we take priority to noreturn variant of call. Since it has return value
> we end up with noreturn call with LHS set.
> 
> I guess this is generic problem with folding too if we fold in
> originally indirect call to be noreturn call with return value,
> but I did not tried to make a testcase.
> 
> This patch fixes it by extending existing code in split_bbs_on_noreturn_calls
> that does updating after the indirect->direct call promotion (splits bb and
> breaks the edge).  It is not also done as part of execute_fixup_cfg where we
> handle similar case with pure/const/noreturn. Similarly as in the other
> cases, we need to relax verifier to accept this during IPA passes until
> local fixups are done.
> 
> It would work to simply add the statements into MODIFIED_NORETURN_CALLS list as
> we do eslewhere, but I do need to do this at IPA level for local noreturn
> discovery I want to submit next, so it is better to have explicit function for
> that.
> 
> (we can get around pure/const/nothrow discovery since inliner does
> the fixup, but for noreturns we can then early inline unfixed function
> body that leads to ICE)
> 
> Bootstrapped/regtested x86_64-linux, OK?
> 
> Honza
> 
> 	* testsuite/gcc.dg/lto/noreturn-1_1.c: New testcase.
> 	* testsuite/gcc.dg/lto/noreturn-1_0.c: New testcase.
> 
> 	* tree-cfgcleanup.c (fixup_noreturn_call): Break out from ...;
> 	remove return value.
> 	(split_bbs_on_noreturn_calls) .... here.
> 	* tree-optimize.c (execute_fixup_cfg): Fixup noreturn calls too.
> 	* tree-flow.h (fixup_noreturn_call): New.
> 	* tree-cfg.c (verify_call, verify_control_flow): Allow nonfixed
> 	noreturns during IPA passes.
> Index: testsuite/gcc.dg/lto/noreturn-1_1.c
> ===================================================================
> --- testsuite/gcc.dg/lto/noreturn-1_1.c	(revision 0)
> +++ testsuite/gcc.dg/lto/noreturn-1_1.c	(revision 0)
> @@ -0,0 +1,9 @@
> +/* { dg-lto-do run } */
> +/* { dg-lto-options {{-O2 -fwhopr} } } */
> +
> +int call_me (void);
> +int
> +main(void)
> +{
> + return call_me ();
> +}
> Index: testsuite/gcc.dg/lto/noreturn-1_0.c
> ===================================================================
> --- testsuite/gcc.dg/lto/noreturn-1_0.c	(revision 0)
> +++ testsuite/gcc.dg/lto/noreturn-1_0.c	(revision 0)
> @@ -0,0 +1,7 @@
> +void exit (int);
> +__attribute__ ((noreturn))
> +int
> +call_me (void)
> +{
> +  exit (0);
> +}
> Index: tree-cfgcleanup.c
> ===================================================================
> --- tree-cfgcleanup.c	(revision 160077)
> +++ tree-cfgcleanup.c	(working copy)
> @@ -538,6 +538,49 @@ remove_forwarder_block (basic_block bb)
>    return true;
>  }
>  
> +/* STMT is a call that has been discovered noreturn.  Fixup the CFG
> +   and remove LHS.  Return true if something changed.  */
> +
> +bool
> +fixup_noreturn_call (gimple stmt)
> +{
> +  basic_block bb = gimple_bb (stmt);
> +  bool changed = false;
> +
> +  if (gimple_call_builtin_p (stmt, BUILT_IN_RETURN))
> +    return false;
> +
> +  /* First split basic block if stmt is not last.  */
> +  if (stmt != gsi_stmt (gsi_last_bb (bb)))
> +    split_block (bb, stmt);
> +
> +  changed |= remove_fallthru_edge (bb->succs);
> +
> +  /* If there is LHS, remove it.  */
> +  if (gimple_call_lhs (stmt))
> +    {
> +      tree op = gimple_call_lhs (stmt);
> +      gimple_call_set_lhs (stmt, NULL_TREE);
> +      /* We need to remove SSA name to avoid checking.
> +	 All uses are dominated by the noreturn and thus will
> +	 be removed afterwards.  */
> +      if (TREE_CODE (op) == SSA_NAME)
> +	{
> +	  use_operand_p use_p;
> +          imm_use_iterator iter;
> +	  gimple use_stmt;
> +
> +          FOR_EACH_IMM_USE_STMT (use_stmt, iter, op)
> +	    FOR_EACH_IMM_USE_ON_STMT (use_p, iter)
> +	      SET_USE (use_p, error_mark_node);

you want release_ssa_name (op) here.

> +	}
> +      update_stmt (stmt);
> +      changed = true;
> +    }
> +  return changed;
> +}
> +
> +
>  /* Split basic blocks on calls in the middle of a basic block that are now
>     known not to return, and remove the unreachable code.  */
>  
> @@ -560,13 +603,10 @@ split_bbs_on_noreturn_calls (void)
>  	    || bb->index < NUM_FIXED_BLOCKS
>  	    || bb->index >= n_basic_blocks
>  	    || BASIC_BLOCK (bb->index) != bb
> -	    || last_stmt (bb) == stmt
>  	    || !gimple_call_noreturn_p (stmt))
>  	  continue;
>  
> -	changed = true;
> -	split_block (bb, stmt);
> -	remove_fallthru_edge (bb->succs);
> +	changed |= fixup_noreturn_call (stmt);
>        }
>  
>    return changed;
> Index: tree-optimize.c
> ===================================================================
> --- tree-optimize.c	(revision 160077)
> +++ tree-optimize.c	(working copy)
> @@ -231,7 +231,8 @@ execute_free_datastructures (void)
>  }
>  
>  /* Pass: fixup_cfg.  IPA passes, compilation of earlier functions or inlining
> -   might have changed some properties, such as marked functions nothrow.
> +   might have changed some properties, such as marked functions nothrow,
> +   pure, const or noreturn.
>     Remove redundant edges and basic blocks, and create new ones if necessary.
>  
>     This pass can't be executed as stand alone pass from pass manager, because
> @@ -267,19 +268,24 @@ execute_fixup_cfg (void)
>  	  tree decl = is_gimple_call (stmt)
>  		      ? gimple_call_fndecl (stmt)
>  		      : NULL;
> -
> -	  if (decl
> -	      && gimple_call_flags (stmt) & (ECF_CONST
> -					     | ECF_PURE
> -					     | ECF_LOOPING_CONST_OR_PURE))
> +	  if (decl)
>  	    {
> -	      if (gimple_in_ssa_p (cfun))
> +	      int flags = gimple_call_flags (stmt);
> +	      if (flags & (ECF_CONST | ECF_PURE | ECF_LOOPING_CONST_OR_PURE))
>  		{
> -		  todo |= TODO_update_ssa | TODO_cleanup_cfg;
> -		  mark_symbols_for_renaming (stmt);
> -		  update_stmt (stmt);
> +		  if (gimple_in_ssa_p (cfun))
> +		    {
> +		      todo |= TODO_update_ssa | TODO_cleanup_cfg;
> +		      mark_symbols_for_renaming (stmt);
> +		      update_stmt (stmt);
> +		    }
>  		}
> -	    }
> +	      
> +	      if (flags & ECF_NORETURN
> +		  && fixup_noreturn_call (stmt))
> +		todo |= TODO_cleanup_cfg;
> +		
> +	     }
>  
>  	  maybe_clean_eh_stmt (stmt);
>  	}
> Index: tree-flow.h
> ===================================================================
> --- tree-flow.h	(revision 160077)
> +++ tree-flow.h	(working copy)
> @@ -870,6 +870,7 @@ tree maybe_fold_tmr (tree);
>  
>  unsigned int execute_free_datastructures (void);
>  unsigned int execute_fixup_cfg (void);
> +bool fixup_noreturn_call (gimple stmt);
>  
>  #include "tree-flow-inline.h"
>  
> Index: tree-cfg.c
> ===================================================================
> --- tree-cfg.c	(revision 160079)
> +++ tree-cfg.c	(working copy)
> @@ -2987,7 +2987,10 @@ verify_gimple_call (gimple stmt)
>        return true;
>      }
>  
> -  if (gimple_call_lhs (stmt) && gimple_call_noreturn_p (stmt))
> +  /* By type merging or during IPA passes we can mark call noreturn.
> +     Before fixup_cfg is executed, ignore this check.  */
> +  if (gimple_call_lhs (stmt) && gimple_call_noreturn_p (stmt)
> +      && cgraph_state != CGRAPH_STATE_IPA_SSA)

Doesn't that mean that we never check this during early optimizations?
I do not think this is a good idea.  Why not avoid verifier calls
during the intermediate broken state at all?

Thanks,
Richard.