Patchwork Fix PR tree-optimization/56424

login
register
mail settings
Submitter Eric Botcazou
Date March 1, 2013, 11:38 a.m.
Message ID <201303011238.05564.ebotcazou@adacore.com>
Download mbox | patch
Permalink /patch/224287/
State New
Headers show

Comments

Eric Botcazou - March 1, 2013, 11:38 a.m.
Hi,

this is the ICE during the build of the Ada runtime on x86-64/Windows: the
compiler stops on the assertion in declare_return_variable:

	  var = return_slot;
	  gcc_assert (TREE_CODE (var) != SSA_NAME);

The problem stems for the fnsplit pass: it turns

P.Sin (const long_long_float x)
{
  long_long_float _3;
  boolean _4;
  q__double _9;

  <bb 2>:
  _3 = ABS_EXPR <x_2(D)>;
  _4 = _3 < 1.0e+0;
  if (_4 != 0)
    goto <bb 3>;
  else
    goto <bb 4>;

  <bb 3>:
  <retval> = x_2(D);
  goto <bb 5>;

  <bb 4>:
  _9 = q.sin (x_2(D));
  <retval> = _9;

  <bb 5>:
  return <retval>;

}

into

P.Sin (const long_long_float x)
{
  long_long_float _3;
  boolean _4;

  <bb 2>:
  _3 = ABS_EXPR <x_2(D)>;
  _4 = _3 < 1.0e+0;
  if (_4 != 0)
    goto <bb 3>;
  else
    goto <bb 4>;

  <bb 3>:
  <retval> = x_2(D);
  goto <bb 5>;

  <bb 4>:
  <retval> = p.sin.part (x_2(D)); [return slot optimization]

  <bb 5>:
  return <retval>;

}

Note the RSO flag on the call to the split part: it's bogus since the return
type is scalar:

  <real_type 0x7ffff6da02a0 long_long_float sizes-gimplified visited XF>

Fixed by checking that either the type of the result is not is_gimple_reg_type
or the result is passed by reference before setting the RSO flag.

Tested on x86_64-suse-linux, OK for the mainline?


2013-01-01  Eric Botcazou  <ebotcazou@adacore.com>

	PR tree-optimization/56424
	* ipa-split.c (split_function): Do not set the RSO flag if result is
	not by reference and its type is not a register type.


2013-01-01  Eric Botcazou  <ebotcazou@adacore.com>

	* gcc.dg/pr56424.c: New test.
Richard Guenther - March 1, 2013, 11:58 a.m.
On Fri, Mar 1, 2013 at 12:38 PM, Eric Botcazou <ebotcazou@adacore.com> wrote:
> Hi,
>
> this is the ICE during the build of the Ada runtime on x86-64/Windows: the
> compiler stops on the assertion in declare_return_variable:
>
>           var = return_slot;
>           gcc_assert (TREE_CODE (var) != SSA_NAME);
>
> The problem stems for the fnsplit pass: it turns
>
> P.Sin (const long_long_float x)
> {
>   long_long_float _3;
>   boolean _4;
>   q__double _9;
>
>   <bb 2>:
>   _3 = ABS_EXPR <x_2(D)>;
>   _4 = _3 < 1.0e+0;
>   if (_4 != 0)
>     goto <bb 3>;
>   else
>     goto <bb 4>;
>
>   <bb 3>:
>   <retval> = x_2(D);
>   goto <bb 5>;
>
>   <bb 4>:
>   _9 = q.sin (x_2(D));
>   <retval> = _9;
>
>   <bb 5>:
>   return <retval>;
>
> }
>
> into
>
> P.Sin (const long_long_float x)
> {
>   long_long_float _3;
>   boolean _4;
>
>   <bb 2>:
>   _3 = ABS_EXPR <x_2(D)>;
>   _4 = _3 < 1.0e+0;
>   if (_4 != 0)
>     goto <bb 3>;
>   else
>     goto <bb 4>;
>
>   <bb 3>:
>   <retval> = x_2(D);
>   goto <bb 5>;
>
>   <bb 4>:
>   <retval> = p.sin.part (x_2(D)); [return slot optimization]
>
>   <bb 5>:
>   return <retval>;
>
> }
>
> Note the RSO flag on the call to the split part: it's bogus since the return
> type is scalar:
>
>   <real_type 0x7ffff6da02a0 long_long_float sizes-gimplified visited XF>
>
> Fixed by checking that either the type of the result is not is_gimple_reg_type
> or the result is passed by reference before setting the RSO flag.
>
> Tested on x86_64-suse-linux, OK for the mainline?

But in theory this pessimizes code as aggregate_value_p returned true
for this?  That is, isn't the bug that we rewrite a possible return-slot-decl
into SSA?  (do we do that here?)

Thanks,
Richard.

>
> 2013-01-01  Eric Botcazou  <ebotcazou@adacore.com>
>
>         PR tree-optimization/56424
>         * ipa-split.c (split_function): Do not set the RSO flag if result is
>         not by reference and its type is not a register type.
>
>
> 2013-01-01  Eric Botcazou  <ebotcazou@adacore.com>
>
>         * gcc.dg/pr56424.c: New test.
>
>
> --
> Eric Botcazou
Eric Botcazou - March 1, 2013, 2:56 p.m.
> But in theory this pessimizes code as aggregate_value_p returned true
> for this?  That is, isn't the bug that we rewrite a possible
> return-slot-decl into SSA?  (do we do that here?)

Yes, aggregate_value_p returns true for this because the ABI says that this 
type is returned in memory.  And, no, I don't think that this will further 
pessimize, given that it's already "pessimized" in gimplify_modify_expr_rhs:

	case CALL_EXPR:
	  /* For calls that return in memory, give *to_p as the CALL_EXPR's
	     return slot so that we don't generate a temporary.  */
	  if (!CALL_EXPR_RETURN_SLOT_OPT (*from_p)
	      && aggregate_value_p (*from_p, *from_p))
	    {
	      bool use_target;

	      if (!(rhs_predicate_for (*to_p))(*from_p))
		/* If we need a temporary, *to_p isn't accurate.  */
		use_target = false;
	      /* It's OK to use the return slot directly unless it's an NRV. */
	      else if (TREE_CODE (*to_p) == RESULT_DECL
		       && DECL_NAME (*to_p) == NULL_TREE
		       && needs_to_live_in_memory (*to_p))
		use_target = true;
	      else if (is_gimple_reg_type (TREE_TYPE (*to_p))
		       || (DECL_P (*to_p) && DECL_REGISTER (*to_p)))
		/* Don't force regs into memory.  */
		use_target = false;
Richard Guenther - March 4, 2013, 10:17 a.m.
On Fri, Mar 1, 2013 at 3:56 PM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> But in theory this pessimizes code as aggregate_value_p returned true
>> for this?  That is, isn't the bug that we rewrite a possible
>> return-slot-decl into SSA?  (do we do that here?)
>
> Yes, aggregate_value_p returns true for this because the ABI says that this
> type is returned in memory.  And, no, I don't think that this will further
> pessimize, given that it's already "pessimized" in gimplify_modify_expr_rhs:
>
>         case CALL_EXPR:
>           /* For calls that return in memory, give *to_p as the CALL_EXPR's
>              return slot so that we don't generate a temporary.  */
>           if (!CALL_EXPR_RETURN_SLOT_OPT (*from_p)
>               && aggregate_value_p (*from_p, *from_p))
>             {
>               bool use_target;
>
>               if (!(rhs_predicate_for (*to_p))(*from_p))
>                 /* If we need a temporary, *to_p isn't accurate.  */
>                 use_target = false;
>               /* It's OK to use the return slot directly unless it's an NRV. */
>               else if (TREE_CODE (*to_p) == RESULT_DECL
>                        && DECL_NAME (*to_p) == NULL_TREE
>                        && needs_to_live_in_memory (*to_p))
>                 use_target = true;
>               else if (is_gimple_reg_type (TREE_TYPE (*to_p))
>                        || (DECL_P (*to_p) && DECL_REGISTER (*to_p)))
>                 /* Don't force regs into memory.  */
>                 use_target = false;

Ah, ok.  Patch is ok then.

Thanks,
Richard.

> --
> Eric Botcazou

Patch

Index: ipa-split.c
===================================================================
--- ipa-split.c	(revision 196253)
+++ ipa-split.c	(working copy)
@@ -1309,7 +1309,9 @@  split_function (struct split_point *spli
      so return slot optimization is always possible.  Moreover this is
      required to make DECL_BY_REFERENCE work.  */
   if (aggregate_value_p (DECL_RESULT (current_function_decl),
-			 TREE_TYPE (current_function_decl)))
+			 TREE_TYPE (current_function_decl))
+      && (!is_gimple_reg_type (TREE_TYPE (DECL_RESULT (current_function_decl)))
+	  || DECL_BY_REFERENCE (DECL_RESULT (current_function_decl))))
     gimple_call_set_return_slot_opt (call, true);
 
   /* Update return value.  This is bit tricky.  When we do not return,