Re: PR middle-end/44813 (ICE in Mozilla build in ptr_deref_may_alias_decl_p)

[Richard Guenther <rguenther@suse.de>]

On Mon, 5 Jul 2010, Jan Hubicka wrote:

> Hi,
> the testcase in PR (Richard is running minimizing on it) produce ICE because
> ipa-split produce invalid gimple
> *<retval> = funcall.part ();
> where <retval> is not gimple register.  It is function returning by reference
> so we actually must write to retval directly for correctness.  <retval> is not
> gimple register because needs_to_live_in_memory returns true after calling
> aggregate_value_p.
> 
> This is confused. For functions returning by reference retval is pointer and
> as such it does not need to live in memory. aggregate_value_p is about return
> value itself, that is *<retval> that lives in memory.
> 
> So this patch changes behaviour of needs_to_live_in_memory that has some consequences.
> 
> 1) We get confused wanrings since tree-ssa-uninit.c considers retvals not initilized
> at function entry.  This is not valid for DECL_BY_REFERENCE>
> 
> 2) tree-inline produces bogus &tmp_1=&tmp_1 for return statement, since it no longer
> recognizes return value wrapped in SSA name.
> 
> 3) Alias analysis don't know that <retval>.filed stores escape and thus must
> be preserved
> 
> 4) Statement verifier does type checking when return statement is not retval.
> It should check DECL_BY_REFERENCE, but Richard plans to submit patch for it.
> So I just patched around strengthening the check for SSA name of DECL_BY_REFERENCE>
> 
> Bootstrapped/regtested x86_64 and tested on Mozilla build.  OK?
>
> Honza
> 
> 	PR middle-end/44813
> 	* tree-ssa-uninit.c (ssa_undefined_value_p): Retval_expr is initialized
> 	for functions returning by reference.
> 	* tree.c (needs_to_live_in_memory): Retval for functions returning by
> 	reference does not need to live in memory.
> 	* tree-inline.c (remap_gimple_stmt): Do not produce assignment
> 	for functions returning by reference.
> 	* tree-ssa-structalias.c (find_what_p_points_to): Look into result decls
> 	same was as into parm decls.
> 	* tree-cfg.c (verify_gimple_return): Expect RESULT_DECL to have SSA name.
> Index: tree-ssa-uninit.c
> ===================================================================
> --- tree-ssa-uninit.c	(revision 161774)
> +++ tree-ssa-uninit.c	(working copy)
> @@ -92,6 +92,12 @@ ssa_undefined_value_p (tree t)
>    if (TREE_CODE (var) == PARM_DECL)
>      return false;
>  
> +  /* When returning by reference the return address is actually a hidden
> +     parameter.  */
> +  if (TREE_CODE (SSA_NAME_VAR (t)) == RESULT_DECL
> +      && DECL_BY_REFERENCE (SSA_NAME_VAR (t)))
> +    return false;
> +
>    /* Hard register variables get their initial value from the ether.  */
>    if (TREE_CODE (var) == VAR_DECL && DECL_HARD_REGISTER (var))
>      return false;

This change is ok.

> Index: tree.c
> ===================================================================
> --- tree.c	(revision 161774)
> +++ tree.c	(working copy)
> @@ -9750,6 +9750,7 @@ needs_to_live_in_memory (const_tree t)
>    return (TREE_ADDRESSABLE (t)
>  	  || is_global_var (t)
>  	  || (TREE_CODE (t) == RESULT_DECL
> +	      && !DECL_BY_REFERENCE (t)
>  	      && aggregate_value_p (t, current_function_decl)));
>  }

Likewise.

> Index: tree-inline.c
> ===================================================================
> --- tree-inline.c	(revision 161774)
> +++ tree-inline.c	(working copy)
> @@ -1236,7 +1237,11 @@ remap_gimple_stmt (gimple stmt, copy_bod
>  	 If RETVAL is just the result decl, the result decl has
>  	 already been set (e.g. a recent "foo (&result_decl, ...)");
>  	 just toss the entire GIMPLE_RETURN.  */
> -      if (retval && TREE_CODE (retval) != RESULT_DECL)
> +      if (retval
> +	  && (TREE_CODE (retval) != RESULT_DECL
> +	      && (TREE_CODE (retval) != SSA_NAME
> +		  || !SSA_NAME_IS_DEFAULT_DEF (retval)
> +		  || TREE_CODE (SSA_NAME_VAR (retval)) != RESULT_DECL)))

We should never see a defintion of a RESULT_DECL SSA name for
DECL_BY_REFERENCE RESULT_DECLs (that would
be a bug - we should add verification to the SSA verifier, can you
do add that?).  So the || !SSA_NAME_IS_DEFAULT_DEF (retval) can be
dropped here.

This patch is ok with that change together with an addition to the
SSA verifier.

>          {
>  	  copy = gimple_build_assign (id->retvar, retval);
>  	  /* id->retvar is already substituted.  Skip it on later remapping.  */
> Index: tree-ssa-structalias.c
> ===================================================================
> --- tree-ssa-structalias.c	(revision 161774)
> +++ tree-ssa-structalias.c	(working copy)
> @@ -5751,7 +5751,8 @@ find_what_p_points_to (tree p)
>    /* For parameters, get at the points-to set for the actual parm
>       decl.  */
>    if (TREE_CODE (p) == SSA_NAME
> -      && TREE_CODE (SSA_NAME_VAR (p)) == PARM_DECL
> +      && (TREE_CODE (SSA_NAME_VAR (p)) == PARM_DECL
> +	  || TREE_CODE (SSA_NAME_VAR (p)) == RESULT_DECL)
>        && SSA_NAME_IS_DEFAULT_DEF (p))
>      lookup_p = SSA_NAME_VAR (p);

That doesn't look correct on its own.  In fact get_constraint_for_ssa_var
needs a similar adjustment, likewise for consistency (probably doesn't
happen here though) get_fi_for_callee.

The patch is ok with those changes.

> Index: tree-cfg.c
> ===================================================================
> --- tree-cfg.c	(revision 161774)
> +++ tree-cfg.c	(working copy)
> @@ -3818,8 +3818,7 @@ verify_gimple_return (gimple stmt)
>    if (op == NULL)
>      return false;
>  
> -  if (!is_gimple_val (op)
> -      && TREE_CODE (op) != RESULT_DECL)
> +  if (!is_gimple_val (op) && TREE_CODE (op) != RESULT_DECL)
>      {
>        error ("invalid operand in return statement");
>        debug_generic_stmt (op);
> @@ -3829,7 +3828,10 @@ verify_gimple_return (gimple stmt)
>    if (!useless_type_conversion_p (restype, TREE_TYPE (op))
>        /* ???  With C++ we can have the situation that the result
>  	 decl is a reference type while the return type is an aggregate.  */
> -      && !(TREE_CODE (op) == RESULT_DECL
> +      && !((TREE_CODE (op) == RESULT_DECL
> +	    || (TREE_CODE (op) == SSA_NAME
> +	        && SSA_NAME_IS_DEFAULT_DEF (op)
> +	        && TREE_CODE (SSA_NAME_VAR (op)) == RESULT_DECL))
>  	   && TREE_CODE (TREE_TYPE (op)) == REFERENCE_TYPE
>  	   && useless_type_conversion_p (restype, TREE_TYPE (TREE_TYPE (op)))))
>      {

I have committed a change similar to this already.

Thanks,
Richard.