Optimize empty class copies within a C++ return statement
diff mbox

Message ID 54F8E5D3.4050607@redhat.com
State New
Headers show

Commit Message

Aldy Hernandez March 5, 2015, 11:25 p.m. UTC
While looking at PR65284, I was confused by the gimple we generate for 
returns of empty classes by value:

class obj {
   public:
    obj(int);
};

obj funky(){
     return obj(555);
}

For the above snippet, we generate:

obj funky() ()
{
   struct obj D.2248;
   struct obj D.2246;

   obj::obj (&D.2246, 555);
   try
     {
       return D.2248;
     }
   finally
     {
       D.2246 = {CLOBBER};
     }
}

Particularly confusing is the return of uninitialized D.2248.  Jason 
tried to beat into me today the fact that it doesn't matter because 
there is no value to initialize since the class is empty.  I still think 
it's weird, however...

What led us to the above gimple was the fact that we lowered into:

	return retval = D.2248 = D.2246

which was later optimized by cp_gimplify_expr into "return D.2248" 
because the C++ gimplifier hook notices that the copy is unnecessary.

Jason suggested that it would be nice to remove D.2248 altogether.  With 
the attached patch we notice the superfluous copy in the return value 
and optimize it away.  After some hoops we now get:

   obj::obj (&D.2246, 555);
   try
     {
       return <retval>;
     }
   finally
     {
       D.2246 = {CLOBBER};
     }

Tested on x86-64 Linux.

OK?
commit 8fd545d608f2a6c11f889e11c700711b8f911c02
Author: Aldy Hernandez <aldyh@redhat.com>
Date:   Thu Mar 5 12:23:27 2015 -0800

    	* cp-gimplify.c (cp_gimplify_expr): Optimize empty class copies
    	within a return statement.

Comments

Jason Merrill March 6, 2015, 6:20 a.m. UTC | #1
On 03/05/2015 06:25 PM, Aldy Hernandez wrote:
> +	tree ret = TREE_OPERAND (*expr_p, 0);
> +	if (ret && (TREE_CODE (ret) == INIT_EXPR
> +		    || TREE_CODE (ret) == MODIFY_EXPR)
> +	    && TREE_CODE (TREE_OPERAND (ret, 0)) == RESULT_DECL
> +	    && is_gimple_lvalue (TREE_OPERAND (ret, 0))
> +	    && is_really_empty_class (TREE_TYPE (TREE_OPERAND (ret, 0))))
> +	  {
> +	    tree result_decl = TREE_OPERAND (ret, 0);
> +	    tree list = alloc_stmt_list ();
> +	    append_to_statement_list (TREE_OPERAND (ret, 1), &list);
> +	    append_to_statement_list (build1 (RETURN_EXPR, void_type_node,
> +					      result_decl), &list);
> +	    *expr_p = list;
> +	    return GS_OK;
> +	  }

This should really use the MODIFY_EXPR case rather than duplicate it 
here.  Actually, why don't we already hit that case when processing the 
RETURN_EXPR?

Jason
Aldy Hernandez March 6, 2015, 4:19 p.m. UTC | #2
On 03/05/2015 10:20 PM, Jason Merrill wrote:
> On 03/05/2015 06:25 PM, Aldy Hernandez wrote:
>> +    tree ret = TREE_OPERAND (*expr_p, 0);
>> +    if (ret && (TREE_CODE (ret) == INIT_EXPR
>> +            || TREE_CODE (ret) == MODIFY_EXPR)
>> +        && TREE_CODE (TREE_OPERAND (ret, 0)) == RESULT_DECL
>> +        && is_gimple_lvalue (TREE_OPERAND (ret, 0))
>> +        && is_really_empty_class (TREE_TYPE (TREE_OPERAND (ret, 0))))
>> +      {
>> +        tree result_decl = TREE_OPERAND (ret, 0);
>> +        tree list = alloc_stmt_list ();
>> +        append_to_statement_list (TREE_OPERAND (ret, 1), &list);
>> +        append_to_statement_list (build1 (RETURN_EXPR, void_type_node,
>> +                          result_decl), &list);
>> +        *expr_p = list;
>> +        return GS_OK;
>> +      }
>
> This should really use the MODIFY_EXPR case rather than duplicate it
> here.  Actually, why don't we already hit that case when processing the
> RETURN_EXPR?

We are hitting the MODIFY_EXPR case.  Indeed, _because_ we hit the 
MODIFY_EXPR is that we return the uninitialized temporary.

For example, we start with:

	return retval = TARGET_EXPR <D.2347, ...>

which becomes:

	<stuff with &D.2347>
	return D.2349 = D.2347;

which the aforementioned MODIFY_EXPR case turns into:

	<stuff with &D.2347>
	return D.2349;

My patch was trying to notice this pattern and get rid of the temporary, 
thereby generating:

	<stuff with &D.2347>
	return retval;

If you still think it's profitable, I can come up with an alternative 
using the MODIFY_EXPR code ??.

Aldy
Jason Merrill March 6, 2015, 9:46 p.m. UTC | #3
On 03/06/2015 11:19 AM, Aldy Hernandez wrote:
> We are hitting the MODIFY_EXPR case.  Indeed, _because_ we hit the
> MODIFY_EXPR is that we return the uninitialized temporary.
>
> For example, we start with:
>
>      return retval = TARGET_EXPR <D.2347, ...>
>
> which becomes:
>
>      <stuff with &D.2347>
>      return D.2349 = D.2347;
>
> which the aforementioned MODIFY_EXPR case turns into:
>
>      <stuff with &D.2347>
>      return D.2349;

But doesn't this still involve a MODIFY_EXPR, i.e. return retval = D.2349?

Jason
Aldy Hernandez March 6, 2015, 9:54 p.m. UTC | #4
On 03/06/2015 01:46 PM, Jason Merrill wrote:
> On 03/06/2015 11:19 AM, Aldy Hernandez wrote:
>> We are hitting the MODIFY_EXPR case.  Indeed, _because_ we hit the
>> MODIFY_EXPR is that we return the uninitialized temporary.
>>
>> For example, we start with:
>>
>>      return retval = TARGET_EXPR <D.2347, ...>
>>
>> which becomes:
>>
>>      <stuff with &D.2347>
>>      return D.2349 = D.2347;
>>
>> which the aforementioned MODIFY_EXPR case turns into:
>>
>>      <stuff with &D.2347>
>>      return D.2349;
>
> But doesn't this still involve a MODIFY_EXPR, i.e. return retval = D.2349?

If I understand you correct, no.

gimplify_return_expr creates a new temporary and uses that instead of 
<retval>:

   else if (gimplify_ctxp->return_temp)
     result = gimplify_ctxp->return_temp;
   else
     {
       result = create_tmp_reg (TREE_TYPE (result_decl));
       ...
     }
...
...
   /* Smash the lhs of the MODIFY_EXPR to the temporary we plan to use.
      Then gimplify the whole thing.  */
   if (result != result_decl)
     TREE_OPERAND (ret_expr, 0) = result;

Aldy
Jason Merrill March 6, 2015, 10:01 p.m. UTC | #5
On 03/06/2015 04:54 PM, Aldy Hernandez wrote:
>> But doesn't this still involve a MODIFY_EXPR, i.e. return retval =
>> D.2349?
>
> If I understand you correct, no.
>
> gimplify_return_expr creates a new temporary and uses that instead of
> <retval>:
>
>    else if (gimplify_ctxp->return_temp)
>      result = gimplify_ctxp->return_temp;
>    else
>      {
>        result = create_tmp_reg (TREE_TYPE (result_decl));
>        ...
>      }
> ...
> ...
>    /* Smash the lhs of the MODIFY_EXPR to the temporary we plan to use.
>       Then gimplify the whole thing.  */
>    if (result != result_decl)
>      TREE_OPERAND (ret_expr, 0) = result;

Sounds like ret_expr is a MODIFY_EXPR.

Jason
Jason Merrill March 6, 2015, 10:01 p.m. UTC | #6
On 03/06/2015 05:01 PM, Jason Merrill wrote:
> On 03/06/2015 04:54 PM, Aldy Hernandez wrote:
>>> But doesn't this still involve a MODIFY_EXPR, i.e. return retval =
>>> D.2349?
>>
>> If I understand you correct, no.
>>
>> gimplify_return_expr creates a new temporary and uses that instead of
>> <retval>:
>>
>>    else if (gimplify_ctxp->return_temp)
>>      result = gimplify_ctxp->return_temp;
>>    else
>>      {
>>        result = create_tmp_reg (TREE_TYPE (result_decl));
>>        ...
>>      }
>> ...
>> ...
>>    /* Smash the lhs of the MODIFY_EXPR to the temporary we plan to use.
>>       Then gimplify the whole thing.  */
>>    if (result != result_decl)
>>      TREE_OPERAND (ret_expr, 0) = result;
>
> Sounds like ret_expr is a MODIFY_EXPR.

Oh, but with the wrong lhs, I see.

Jason
Aldy Hernandez March 9, 2015, 6:33 p.m. UTC | #7
On 03/06/2015 02:01 PM, Jason Merrill wrote:
> On 03/06/2015 05:01 PM, Jason Merrill wrote:
>> On 03/06/2015 04:54 PM, Aldy Hernandez wrote:
>>>> But doesn't this still involve a MODIFY_EXPR, i.e. return retval =
>>>> D.2349?
>>>
>>> If I understand you correct, no.
>>>
>>> gimplify_return_expr creates a new temporary and uses that instead of
>>> <retval>:
>>>
>>>    else if (gimplify_ctxp->return_temp)
>>>      result = gimplify_ctxp->return_temp;
>>>    else
>>>      {
>>>        result = create_tmp_reg (TREE_TYPE (result_decl));
>>>        ...
>>>      }
>>> ...
>>> ...
>>>    /* Smash the lhs of the MODIFY_EXPR to the temporary we plan to use.
>>>       Then gimplify the whole thing.  */
>>>    if (result != result_decl)
>>>      TREE_OPERAND (ret_expr, 0) = result;
>>
>> Sounds like ret_expr is a MODIFY_EXPR.
>
> Oh, but with the wrong lhs, I see.

I know you want to reuse the MODIFY_EXPR case in cp_gimplify_expr, but 
after playing around with it, I think it requires too much special 
casing to make it clean.

For instance, the MODIFY_EXPR case returns the RHS of expression which 
is the opposite of what we want.  For this:

	return retval = <obj>

...the MODIFY_EXPR case would build a COMPOUND_EXPR with "return 
<<<retval, <obj>>>>", which would return <obj>, not retval.  And what we 
probably want is a statement list with:

	<evaluation of obj>
	return retval

Also, the actual case we're dealing with here is a  bit more 
complicated, as it involves a COMPOUND_EXPR in the RHS, which we'd have 
to adapt MODIFY_EXPR to handle:

	return retval = <<<TARGET_EXPR, D.9999>>

IMHO, adding a special case for all this is a lot messier than what I 
originally suggested.

What do you think?

Aldy
Jason Merrill March 10, 2015, 2:50 a.m. UTC | #8
On 03/09/2015 02:33 PM, Aldy Hernandez wrote:
> On 03/06/2015 02:01 PM, Jason Merrill wrote:
>> On 03/06/2015 05:01 PM, Jason Merrill wrote:
>>> On 03/06/2015 04:54 PM, Aldy Hernandez wrote:
>>>>> But doesn't this still involve a MODIFY_EXPR, i.e. return retval =
>>>>> D.2349?
>>>>
>>>> If I understand you correct, no.
>>>>
>>>> gimplify_return_expr creates a new temporary and uses that instead of
>>>> <retval>:
>>>>
>>>>    else if (gimplify_ctxp->return_temp)
>>>>      result = gimplify_ctxp->return_temp;
>>>>    else
>>>>      {
>>>>        result = create_tmp_reg (TREE_TYPE (result_decl));
>>>>        ...
>>>>      }
>>>> ...
>>>> ...
>>>>    /* Smash the lhs of the MODIFY_EXPR to the temporary we plan to use.
>>>>       Then gimplify the whole thing.  */
>>>>    if (result != result_decl)
>>>>      TREE_OPERAND (ret_expr, 0) = result;
>>>
>>> Sounds like ret_expr is a MODIFY_EXPR.
>>
>> Oh, but with the wrong lhs, I see.
>
> I know you want to reuse the MODIFY_EXPR case in cp_gimplify_expr, but
> after playing around with it, I think it requires too much special
> casing to make it clean.
>
> For instance, the MODIFY_EXPR case returns the RHS of expression which
> is the opposite of what we want.  For this:
>
>      return retval = <obj>
>
> ...the MODIFY_EXPR case would build a COMPOUND_EXPR with "return
> <<<retval, <obj>>>>", which would return <obj>, not retval.  And what we
> probably want is a statement list with:
>
>      <evaluation of obj>
>      return retval

Agreed, we want (op1, op0) in this case.  I think this demonstrates that 
the existing code is wrong to sometimes return (op0, op1), since we 
might be using the result as an lvalue.  Better would be to always 
gimplify op0 to an lvalue, gimplify_and_add the rhs, and finally return 
the gimplified lhs, rather than mess with building up a COMPOUND_EXPR 
(or STATEMENT_LIST, as in your patch).

> Also, the actual case we're dealing with here is a  bit more
> complicated, as it involves a COMPOUND_EXPR in the RHS, which we'd have
> to adapt MODIFY_EXPR to handle:
>
>      return retval = <<<TARGET_EXPR, D.9999>>
>
> IMHO, adding a special case for all this is a lot messier than what I
> originally suggested.

I would expect the current code to handle this fine.

Jason

Patch
diff mbox

diff --git a/gcc/cp/cp-gimplify.c b/gcc/cp/cp-gimplify.c
index 4233a64..f8d4559 100644
--- a/gcc/cp/cp-gimplify.c
+++ b/gcc/cp/cp-gimplify.c
@@ -740,6 +740,29 @@  cp_gimplify_expr (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p)
 	}
       break;
 
+    case RETURN_EXPR:
+      {
+	/* Optimize `return <retval> = object' where object's type is
+	   an empty class.  Avoid the copy, altogether and just return
+	   retval.  */
+	tree ret = TREE_OPERAND (*expr_p, 0);
+	if (ret && (TREE_CODE (ret) == INIT_EXPR
+		    || TREE_CODE (ret) == MODIFY_EXPR)
+	    && TREE_CODE (TREE_OPERAND (ret, 0)) == RESULT_DECL
+	    && is_gimple_lvalue (TREE_OPERAND (ret, 0))
+	    && is_really_empty_class (TREE_TYPE (TREE_OPERAND (ret, 0))))
+	  {
+	    tree result_decl = TREE_OPERAND (ret, 0);
+	    tree list = alloc_stmt_list ();
+	    append_to_statement_list (TREE_OPERAND (ret, 1), &list);
+	    append_to_statement_list (build1 (RETURN_EXPR, void_type_node,
+					      result_decl), &list);
+	    *expr_p = list;
+	    return GS_OK;
+	  }
+	/* Otherwise fall through.  */
+      }
+
     default:
       ret = (enum gimplify_status) c_gimplify_expr (expr_p, pre_p, post_p);
       break;
diff --git a/gcc/testsuite/g++.dg/other/empty-class.C b/gcc/testsuite/g++.dg/other/empty-class.C
new file mode 100644
index 0000000..a14c437
--- /dev/null
+++ b/gcc/testsuite/g++.dg/other/empty-class.C
@@ -0,0 +1,17 @@ 
+/* { dg-do compile } */
+/* { dg-options "-fdump-tree-gimple" } */
+
+/* Test that we return retval directly, instead of going through an
+   intermediate temporary, when returning an empty class.  */
+
+class obj {
+  public:
+   obj(int);
+};
+
+obj funky(){
+    return obj(555);
+}
+
+/* { dg-final { scan-tree-dump-times "return <retval>;" 1 "gimple" } } */
+/* { dg-final { cleanup-tree-dump "gimple" } } */