Fix ICE with thunks taking scalars passed by reference

Fix ICE with thunks taking scalars passed by reference

[Jan Hubicka]

Hi,
this patch solves ICE in the attached testcase on mingw32.  The problem is that
on Windows API long double is passed & returned by reference and while expanidng
the tunk tail call, we get lost because we turn the parameter into SSA name and
later need its address to pass it further.

The patch extends hack https://gcc.gnu.org/ml/gcc-patches/2014-11/msg00423.html
to handle not only non-registers but also registers.

Bootstrapped/regtested ppc64-linux. OK?

The bug reproduced with ICF, but I suppose it will turn into ice on any C++ covariant
thunks taking scalar passed by reference.

ng double func1 (long double x)
{
  if (x > 0.0)
    return x;
  else if (x < 0.0)
    return -x;
  else
    return x;
}

long double func2 (long double x)
{
  if (x > 0.0)
    return x;
  else if (x < 0.0)
    return -x;
  else
    return x;
}

	PR ipa/65540
	* calls.c (initialize_argument_information): When producing tail
	call also turn SSA_NAMES passed by references to original PARM_DECLs
Index: calls.c
===================================================================
--- calls.c	(revision 221805)
+++ calls.c	(working copy)
@@ -1321,6 +1321,15 @@ initialize_argument_information (int num
 		  && TREE_CODE (base) != SSA_NAME
 		  && (!DECL_P (base) || MEM_P (DECL_RTL (base)))))
 	    {
+	      /* We may have turned the parameter value into an SSA name.
+		 Go back to the original parameter so we can take the
+		 address.  */
+	      if (TREE_CODE (args[i].tree_value) == SSA_NAME)
+		{
+		  gcc_assert (SSA_NAME_IS_DEFAULT_DEF (args[i].tree_value));
+		  args[i].tree_value = SSA_NAME_VAR (args[i].tree_value);
+		  gcc_assert (TREE_CODE (args[i].tree_value) == PARM_DECL);
+		}
 	      /* Argument setup code may have copied the value to register.  We
 		 revert that optimization now because the tail call code must
 		 use the original location.  */

Re: Fix ICE with thunks taking scalars passed by reference

[Jakub Jelinek]

On Wed, Apr 01, 2015 at 08:32:04AM +0200, Jan Hubicka wrote:
> this patch solves ICE in the attached testcase on mingw32.  The problem is that
> on Windows API long double is passed & returned by reference and while expanidng
> the tunk tail call, we get lost because we turn the parameter into SSA name and
> later need its address to pass it further.
> 
> The patch extends hack https://gcc.gnu.org/ml/gcc-patches/2014-11/msg00423.html
> to handle not only non-registers but also registers.
> 
> Bootstrapped/regtested ppc64-linux. OK?

I think it might be better to handle thunks the same way as other tail
calls, but
--- gcc/cgraphunit.c	2015-04-03 15:32:31.000000000 +0200
+++ gcc/cgraphunit.c	2015-04-07 13:02:16.537723725 +0200
@@ -1767,7 +1767,17 @@ cgraph_node::expand_thunk (bool output_a
 
 	  /* Build return value.  */
 	  if (!DECL_BY_REFERENCE (resdecl))
-	    ret = gimple_build_return (restmp);
+	    {
+	      if (is_gimple_reg_type (restype)
+		  && aggregate_value_p (resdecl, TREE_TYPE (thunk_fndecl)))
+		{
+		  gimple stmt = gimple_build_assign (resdecl, restmp);
+		  gsi_insert_after (&bsi, stmt, GSI_NEW_STMT);
+		  ret = gimple_build_return (resdecl);
+		}
+	      else
+		ret = gimple_build_return (restmp);
+	    }
 	  else
 	    ret = gimple_build_return (resdecl);
(which generates the same GIMPLE code as say
long double func1 (long double x);

long double
func2 (long double x)
{
  return func1 (x);
}
on both mingw and x86_64-linux) doesn't really work, because calls.c treats
calls in thunks differently from other calls - only in the thunks it takes
ADDR_EXPR of the parameter/result.  Thus your patch is ok
for trunk, but please add the testcase into the testsuite.

> 	PR ipa/65540
> 	* calls.c (initialize_argument_information): When producing tail
> 	call also turn SSA_NAMES passed by references to original PARM_DECLs
> Index: calls.c
> ===================================================================
> --- calls.c	(revision 221805)
> +++ calls.c	(working copy)
> @@ -1321,6 +1321,15 @@ initialize_argument_information (int num
>  		  && TREE_CODE (base) != SSA_NAME
>  		  && (!DECL_P (base) || MEM_P (DECL_RTL (base)))))
>  	    {
> +	      /* We may have turned the parameter value into an SSA name.
> +		 Go back to the original parameter so we can take the
> +		 address.  */
> +	      if (TREE_CODE (args[i].tree_value) == SSA_NAME)
> +		{
> +		  gcc_assert (SSA_NAME_IS_DEFAULT_DEF (args[i].tree_value));
> +		  args[i].tree_value = SSA_NAME_VAR (args[i].tree_value);
> +		  gcc_assert (TREE_CODE (args[i].tree_value) == PARM_DECL);
> +		}
>  	      /* Argument setup code may have copied the value to register.  We
>  		 revert that optimization now because the tail call code must
>  		 use the original location.  */

	Jakub

Re: Fix ICE with thunks taking scalars passed by reference

[Jan Hubicka]

> On Wed, Apr 01, 2015 at 08:32:04AM +0200, Jan Hubicka wrote:
> > this patch solves ICE in the attached testcase on mingw32.  The problem is that
> > on Windows API long double is passed & returned by reference and while expanidng
> > the tunk tail call, we get lost because we turn the parameter into SSA name and
> > later need its address to pass it further.
> > 
> > The patch extends hack https://gcc.gnu.org/ml/gcc-patches/2014-11/msg00423.html
> > to handle not only non-registers but also registers.
> > 
> > Bootstrapped/regtested ppc64-linux. OK?
> 
> I think it might be better to handle thunks the same way as other tail
> calls, but
> --- gcc/cgraphunit.c	2015-04-03 15:32:31.000000000 +0200
> +++ gcc/cgraphunit.c	2015-04-07 13:02:16.537723725 +0200
> @@ -1767,7 +1767,17 @@ cgraph_node::expand_thunk (bool output_a
>  
>  	  /* Build return value.  */
>  	  if (!DECL_BY_REFERENCE (resdecl))
> -	    ret = gimple_build_return (restmp);
> +	    {
> +	      if (is_gimple_reg_type (restype)
> +		  && aggregate_value_p (resdecl, TREE_TYPE (thunk_fndecl)))
> +		{
> +		  gimple stmt = gimple_build_assign (resdecl, restmp);
> +		  gsi_insert_after (&bsi, stmt, GSI_NEW_STMT);
> +		  ret = gimple_build_return (resdecl);
> +		}
> +	      else
> +		ret = gimple_build_return (restmp);
> +	    }
>  	  else
>  	    ret = gimple_build_return (resdecl);
> (which generates the same GIMPLE code as say
> long double func1 (long double x);
> 
> long double
> func2 (long double x)
> {
>   return func1 (x);
> }
> on both mingw and x86_64-linux) doesn't really work, because calls.c treats
> calls in thunks differently from other calls - only in the thunks it takes
> ADDR_EXPR of the parameter/result.  Thus your patch is ok

Yes, unforutnately thunk tail calls are quite special because we know we can
actually use the space occupied by incomming parameters because the signature
is not changed.

Implementing this trick in general may be nice, but with current gimple representation
not exposing ABI details very well it would be hard.
> for trunk, but please add the testcase into the testsuite.

Will do.
> 
> > 	PR ipa/65540
> > 	* calls.c (initialize_argument_information): When producing tail
> > 	call also turn SSA_NAMES passed by references to original PARM_DECLs
> > Index: calls.c
> > ===================================================================
> > --- calls.c	(revision 221805)
> > +++ calls.c	(working copy)
> > @@ -1321,6 +1321,15 @@ initialize_argument_information (int num
> >  		  && TREE_CODE (base) != SSA_NAME
> >  		  && (!DECL_P (base) || MEM_P (DECL_RTL (base)))))
> >  	    {
> > +	      /* We may have turned the parameter value into an SSA name.
> > +		 Go back to the original parameter so we can take the
> > +		 address.  */
> > +	      if (TREE_CODE (args[i].tree_value) == SSA_NAME)
> > +		{
> > +		  gcc_assert (SSA_NAME_IS_DEFAULT_DEF (args[i].tree_value));
> > +		  args[i].tree_value = SSA_NAME_VAR (args[i].tree_value);
> > +		  gcc_assert (TREE_CODE (args[i].tree_value) == PARM_DECL);
> > +		}
> >  	      /* Argument setup code may have copied the value to register.  We
> >  		 revert that optimization now because the tail call code must
> >  		 use the original location.  */
> 
> 	Jakub