* [patch] Optimize empty class copies within a C++ return statement
@ 2015-03-05 23:25 Aldy Hernandez
2015-03-06 6:20 ` Jason Merrill
0 siblings, 1 reply; 15+ messages in thread
From: Aldy Hernandez @ 2015-03-05 23:25 UTC (permalink / raw)
To: jason merrill, gcc-patches
[-- Attachment #1: Type: text/plain, Size: 1243 bytes --]
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?
[-- Attachment #2: curr --]
[-- Type: text/plain, Size: 2052 bytes --]
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.
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" } } */
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [patch] Optimize empty class copies within a C++ return statement
2015-03-05 23:25 [patch] Optimize empty class copies within a C++ return statement Aldy Hernandez
@ 2015-03-06 6:20 ` Jason Merrill
2015-03-06 16:19 ` Aldy Hernandez
0 siblings, 1 reply; 15+ messages in thread
From: Jason Merrill @ 2015-03-06 6:20 UTC (permalink / raw)
To: Aldy Hernandez, gcc-patches
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [patch] Optimize empty class copies within a C++ return statement
2015-03-06 6:20 ` Jason Merrill
@ 2015-03-06 16:19 ` Aldy Hernandez
2015-03-06 21:46 ` Jason Merrill
0 siblings, 1 reply; 15+ messages in thread
From: Aldy Hernandez @ 2015-03-06 16:19 UTC (permalink / raw)
To: Jason Merrill, gcc-patches
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [patch] Optimize empty class copies within a C++ return statement
2015-03-06 16:19 ` Aldy Hernandez
@ 2015-03-06 21:46 ` Jason Merrill
2015-03-06 21:54 ` Aldy Hernandez
0 siblings, 1 reply; 15+ messages in thread
From: Jason Merrill @ 2015-03-06 21:46 UTC (permalink / raw)
To: Aldy Hernandez, gcc-patches
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [patch] Optimize empty class copies within a C++ return statement
2015-03-06 21:46 ` Jason Merrill
@ 2015-03-06 21:54 ` Aldy Hernandez
2015-03-06 22:01 ` Jason Merrill
0 siblings, 1 reply; 15+ messages in thread
From: Aldy Hernandez @ 2015-03-06 21:54 UTC (permalink / raw)
To: Jason Merrill, gcc-patches
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [patch] Optimize empty class copies within a C++ return statement
2015-03-06 21:54 ` Aldy Hernandez
@ 2015-03-06 22:01 ` Jason Merrill
2015-03-06 22:02 ` Jason Merrill
0 siblings, 1 reply; 15+ messages in thread
From: Jason Merrill @ 2015-03-06 22:01 UTC (permalink / raw)
To: Aldy Hernandez, gcc-patches
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [patch] Optimize empty class copies within a C++ return statement
2015-03-06 22:01 ` Jason Merrill
@ 2015-03-06 22:02 ` Jason Merrill
2015-03-09 18:33 ` Aldy Hernandez
0 siblings, 1 reply; 15+ messages in thread
From: Jason Merrill @ 2015-03-06 22:02 UTC (permalink / raw)
To: Aldy Hernandez, gcc-patches
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [patch] Optimize empty class copies within a C++ return statement
2015-03-06 22:02 ` Jason Merrill
@ 2015-03-09 18:33 ` Aldy Hernandez
2015-03-10 2:51 ` Jason Merrill
0 siblings, 1 reply; 15+ messages in thread
From: Aldy Hernandez @ 2015-03-09 18:33 UTC (permalink / raw)
To: Jason Merrill, gcc-patches
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [patch] Optimize empty class copies within a C++ return statement
2015-03-09 18:33 ` Aldy Hernandez
@ 2015-03-10 2:51 ` Jason Merrill
2015-03-10 23:11 ` Aldy Hernandez
0 siblings, 1 reply; 15+ messages in thread
From: Jason Merrill @ 2015-03-10 2:51 UTC (permalink / raw)
To: Aldy Hernandez, gcc-patches
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [patch] Optimize empty class copies within a C++ return statement
2015-03-10 2:51 ` Jason Merrill
@ 2015-03-10 23:11 ` Aldy Hernandez
2015-03-10 23:59 ` Jason Merrill
0 siblings, 1 reply; 15+ messages in thread
From: Aldy Hernandez @ 2015-03-10 23:11 UTC (permalink / raw)
To: Jason Merrill, gcc-patches
[-- Attachment #1: Type: text/plain, Size: 904 bytes --]
> 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).
Fair enough.
I had to tweak the condition a bit, as the returns I'm seeing contain a
COMPOUND_EXPR. I also unfortunately had to leave that ugly special case
for TREE_THIS_VOLATILE, as the comment is correct :), gimplify_expr goes
into an infinite loop gimplifying a volatile temporary.
Finally, I am using a goto instead of recursing, as the case for
INIT_EXPR degrades INIT_EXPRs containing AGGR_INIT_EXPRs incorrectly.
The attached patch was tested on x86-64 Linux. How does it look?
Aldy
[-- Attachment #2: curr --]
[-- Type: text/plain, Size: 4770 bytes --]
commit 451ffe1998bd26175677fe62e9449313da642fbe
Author: Aldy Hernandez <aldyh@redhat.com>
Date: Thu Mar 5 12:23:27 2015 -0800
* cp-gimplify.c (simple_empty_class_p): New.
* cp-gimplify.c (cp_gimplify_expr): Handle RETURN_EXPR. Abstract
the code for empty class copies into simple_empty_class_p, and
adapt it to handle COMPOUND_EXPRs.
diff --git a/gcc/cp/cp-gimplify.c b/gcc/cp/cp-gimplify.c
index 4233a64..115cbaa 100644
--- a/gcc/cp/cp-gimplify.c
+++ b/gcc/cp/cp-gimplify.c
@@ -528,6 +528,29 @@ gimplify_must_not_throw_expr (tree *expr_p, gimple_seq *pre_p)
return GS_ALL_DONE;
}
+/* Return TRUE if an operand (OP) of a given TYPE being copied is
+ really just an empty class copy.
+
+ Check that the operand has a simple form so that TARGET_EXPRs and
+ non-empty CONSTRUCTORs get reduced properly, and we leave the
+ return slot optimization alone because it isn't a copy. */
+
+static bool
+simple_empty_class_p (tree type, tree op)
+{
+ return
+ ((TREE_CODE (op) == COMPOUND_EXPR
+ && simple_empty_class_p (type, TREE_OPERAND (op, 1)))
+ || is_gimple_lvalue (op)
+ || INDIRECT_REF_P (op)
+ || (TREE_CODE (op) == CONSTRUCTOR
+ && CONSTRUCTOR_NELTS (op) == 0
+ && !TREE_CLOBBER_P (op))
+ || (TREE_CODE (op) == CALL_EXPR
+ && !CALL_EXPR_RETURN_SLOT_OPT (op)))
+ && is_really_empty_class (type);
+}
+
/* Do C++-specific gimplification. Args are as for gimplify_expr. */
int
@@ -597,6 +620,7 @@ cp_gimplify_expr (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p)
return GS_OK;
/* Otherwise fall through. */
case MODIFY_EXPR:
+ modify_expr_case:
{
if (fn_contains_cilk_spawn_p (cfun)
&& cilk_detect_spawn_and_unwrap (expr_p)
@@ -616,31 +640,20 @@ cp_gimplify_expr (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p)
TREE_OPERAND (*expr_p, 1) = build1 (VIEW_CONVERT_EXPR,
TREE_TYPE (op0), op1);
- else if ((is_gimple_lvalue (op1) || INDIRECT_REF_P (op1)
- || (TREE_CODE (op1) == CONSTRUCTOR
- && CONSTRUCTOR_NELTS (op1) == 0
- && !TREE_CLOBBER_P (op1))
- || (TREE_CODE (op1) == CALL_EXPR
- && !CALL_EXPR_RETURN_SLOT_OPT (op1)))
- && is_really_empty_class (TREE_TYPE (op0)))
+ else if (simple_empty_class_p (TREE_TYPE (op0), op1))
{
- /* Remove any copies of empty classes. We check that the RHS
- has a simple form so that TARGET_EXPRs and non-empty
- CONSTRUCTORs get reduced properly, and we leave the return
- slot optimization alone because it isn't a copy (FIXME so it
- shouldn't be represented as one).
-
- Also drop volatile variables on the RHS to avoid infinite
- recursion from gimplify_expr trying to load the value. */
- if (!TREE_SIDE_EFFECTS (op1))
- *expr_p = op0;
- else if (TREE_THIS_VOLATILE (op1)
- && (REFERENCE_CLASS_P (op1) || DECL_P (op1)))
- *expr_p = build2 (COMPOUND_EXPR, TREE_TYPE (*expr_p),
- build_fold_addr_expr (op1), op0);
- else
- *expr_p = build2 (COMPOUND_EXPR, TREE_TYPE (*expr_p),
- op0, op1);
+ if (TREE_SIDE_EFFECTS (op1))
+ {
+ /* Drop volatile variables on the RHS to avoid
+ infinite recursion from gimplify_expr trying to
+ load the value. */
+ if (TREE_THIS_VOLATILE (op1)
+ && (REFERENCE_CLASS_P (op1) || DECL_P (op1)))
+ op1 = build_fold_addr_expr (op1);
+
+ gimplify_and_add (op1, pre_p);
+ }
+ *expr_p = op0;
}
}
ret = GS_OK;
@@ -740,6 +753,19 @@ cp_gimplify_expr (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p)
}
break;
+ case RETURN_EXPR:
+ if (TREE_OPERAND (*expr_p, 0)
+ && (TREE_CODE (TREE_OPERAND (*expr_p, 0)) == INIT_EXPR
+ || TREE_CODE (TREE_OPERAND (*expr_p, 0)) == MODIFY_EXPR))
+ {
+ expr_p = &TREE_OPERAND (*expr_p, 0);
+ code = TREE_CODE (*expr_p);
+ /* Avoid going through the INIT_EXPR case, which can
+ degrade INIT_EXPRs into AGGR_INIT_EXPRs. */
+ goto modify_expr_case;
+ }
+ /* 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" } } */
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [patch] Optimize empty class copies within a C++ return statement
2015-03-10 23:11 ` Aldy Hernandez
@ 2015-03-10 23:59 ` Jason Merrill
2015-03-11 0:07 ` Aldy Hernandez
0 siblings, 1 reply; 15+ messages in thread
From: Jason Merrill @ 2015-03-10 23:59 UTC (permalink / raw)
To: Aldy Hernandez, gcc-patches
On 03/10/2015 07:10 PM, Aldy Hernandez wrote:
> - /* Remove any copies of empty classes. We check that the RHS
> - has a simple form so that TARGET_EXPRs and non-empty
> - CONSTRUCTORs get reduced properly, and we leave the return
> - slot optimization alone because it isn't a copy (FIXME so it
> - shouldn't be represented as one).
Let's keep this comment.
> gimplify op0 to an lvalue
Also, I don't see this first step in the patch; I wanted that so the op0
side-effects happen first.
Jason
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [patch] Optimize empty class copies within a C++ return statement
2015-03-10 23:59 ` Jason Merrill
@ 2015-03-11 0:07 ` Aldy Hernandez
2015-03-11 0:55 ` Jason Merrill
0 siblings, 1 reply; 15+ messages in thread
From: Aldy Hernandez @ 2015-03-11 0:07 UTC (permalink / raw)
To: Jason Merrill, gcc-patches
On 03/10/2015 04:59 PM, Jason Merrill wrote:
> On 03/10/2015 07:10 PM, Aldy Hernandez wrote:
>> - /* Remove any copies of empty classes. We check that the RHS
>> - has a simple form so that TARGET_EXPRs and non-empty
>> - CONSTRUCTORs get reduced properly, and we leave the return
>> - slot optimization alone because it isn't a copy (FIXME so it
>> - shouldn't be represented as one).
The comment is now in the abstracted function simple_empty_class_p
header. Although, I did remove the "remove any copies of empty classes"
bit, as well as the FIXME. I can put both of those parts back in if
desired. Presumably, the "remove any copies" back where it was, and the
FIXME in the abstracted function?
Aldy
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [patch] Optimize empty class copies within a C++ return statement
2015-03-11 0:07 ` Aldy Hernandez
@ 2015-03-11 0:55 ` Jason Merrill
2015-03-11 1:45 ` Aldy Hernandez
0 siblings, 1 reply; 15+ messages in thread
From: Jason Merrill @ 2015-03-11 0:55 UTC (permalink / raw)
To: Aldy Hernandez, gcc-patches
On 03/10/2015 08:07 PM, Aldy Hernandez wrote:
> On 03/10/2015 04:59 PM, Jason Merrill wrote:
>> On 03/10/2015 07:10 PM, Aldy Hernandez wrote:
>>> - /* Remove any copies of empty classes. We check that the RHS
>>> - has a simple form so that TARGET_EXPRs and non-empty
>>> - CONSTRUCTORs get reduced properly, and we leave the return
>>> - slot optimization alone because it isn't a copy (FIXME so it
>>> - shouldn't be represented as one).
>
> The comment is now in the abstracted function simple_empty_class_p
> header. Although, I did remove the "remove any copies of empty classes"
> bit, as well as the FIXME. I can put both of those parts back in if
> desired. Presumably, the "remove any copies" back where it was, and the
> FIXME in the abstracted function?
Sounds good.
Jason
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [patch] Optimize empty class copies within a C++ return statement
2015-03-11 0:55 ` Jason Merrill
@ 2015-03-11 1:45 ` Aldy Hernandez
2015-03-11 12:06 ` Jason Merrill
0 siblings, 1 reply; 15+ messages in thread
From: Aldy Hernandez @ 2015-03-11 1:45 UTC (permalink / raw)
To: Jason Merrill, gcc-patches
[-- Attachment #1: Type: text/plain, Size: 1088 bytes --]
On 03/10/2015 05:55 PM, Jason Merrill wrote:
> On 03/10/2015 08:07 PM, Aldy Hernandez wrote:
>> On 03/10/2015 04:59 PM, Jason Merrill wrote:
>>> On 03/10/2015 07:10 PM, Aldy Hernandez wrote:
>>>> - /* Remove any copies of empty classes. We check that the RHS
>>>> - has a simple form so that TARGET_EXPRs and non-empty
>>>> - CONSTRUCTORs get reduced properly, and we leave the return
>>>> - slot optimization alone because it isn't a copy (FIXME
>>>> so it
>>>> - shouldn't be represented as one).
>>
>> The comment is now in the abstracted function simple_empty_class_p
>> header. Although, I did remove the "remove any copies of empty classes"
>> bit, as well as the FIXME. I can put both of those parts back in if
>> desired. Presumably, the "remove any copies" back where it was, and the
>> FIXME in the abstracted function?
>
> Sounds good.
>
> Jason
>
>
I would've thought the actual toplevel gimplification (from
gimplify_expr) would've handled the LHS side-effects, but I've done as
you requested.
How does this look?
Aldy
[-- Attachment #2: curr --]
[-- Type: text/plain, Size: 5167 bytes --]
commit 2c819732ff6cd611c05082f8522a1e6aa452dc65
Author: Aldy Hernandez <aldyh@redhat.com>
Date: Thu Mar 5 12:23:27 2015 -0800
* cp-gimplify.c (simple_empty_class_p): New.
* cp-gimplify.c (cp_gimplify_expr): Handle RETURN_EXPR. Abstract
the code for empty class copies into simple_empty_class_p, and
adapt it to handle COMPOUND_EXPRs.
diff --git a/gcc/cp/cp-gimplify.c b/gcc/cp/cp-gimplify.c
index 4233a64..70645b5 100644
--- a/gcc/cp/cp-gimplify.c
+++ b/gcc/cp/cp-gimplify.c
@@ -53,6 +53,8 @@ along with GCC; see the file COPYING3. If not see
#include "target.h"
#include "c-family/c-ubsan.h"
#include "cilk.h"
+#include "gimplify.h"
+#include "gimple-expr.h"
/* Forward declarations. */
@@ -528,6 +530,29 @@ gimplify_must_not_throw_expr (tree *expr_p, gimple_seq *pre_p)
return GS_ALL_DONE;
}
+/* Return TRUE if an operand (OP) of a given TYPE being copied is
+ really just an empty class copy.
+
+ Check that the operand has a simple form so that TARGET_EXPRs and
+ non-empty CONSTRUCTORs get reduced properly, and we leave the
+ return slot optimization alone because it isn't a copy. */
+
+static bool
+simple_empty_class_p (tree type, tree op)
+{
+ return
+ ((TREE_CODE (op) == COMPOUND_EXPR
+ && simple_empty_class_p (type, TREE_OPERAND (op, 1)))
+ || is_gimple_lvalue (op)
+ || INDIRECT_REF_P (op)
+ || (TREE_CODE (op) == CONSTRUCTOR
+ && CONSTRUCTOR_NELTS (op) == 0
+ && !TREE_CLOBBER_P (op))
+ || (TREE_CODE (op) == CALL_EXPR
+ && !CALL_EXPR_RETURN_SLOT_OPT (op)))
+ && is_really_empty_class (type);
+}
+
/* Do C++-specific gimplification. Args are as for gimplify_expr. */
int
@@ -597,6 +622,7 @@ cp_gimplify_expr (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p)
return GS_OK;
/* Otherwise fall through. */
case MODIFY_EXPR:
+ modify_expr_case:
{
if (fn_contains_cilk_spawn_p (cfun)
&& cilk_detect_spawn_and_unwrap (expr_p)
@@ -616,31 +642,22 @@ cp_gimplify_expr (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p)
TREE_OPERAND (*expr_p, 1) = build1 (VIEW_CONVERT_EXPR,
TREE_TYPE (op0), op1);
- else if ((is_gimple_lvalue (op1) || INDIRECT_REF_P (op1)
- || (TREE_CODE (op1) == CONSTRUCTOR
- && CONSTRUCTOR_NELTS (op1) == 0
- && !TREE_CLOBBER_P (op1))
- || (TREE_CODE (op1) == CALL_EXPR
- && !CALL_EXPR_RETURN_SLOT_OPT (op1)))
- && is_really_empty_class (TREE_TYPE (op0)))
+ else if (simple_empty_class_p (TREE_TYPE (op0), op1))
{
- /* Remove any copies of empty classes. We check that the RHS
- has a simple form so that TARGET_EXPRs and non-empty
- CONSTRUCTORs get reduced properly, and we leave the return
- slot optimization alone because it isn't a copy (FIXME so it
- shouldn't be represented as one).
-
- Also drop volatile variables on the RHS to avoid infinite
- recursion from gimplify_expr trying to load the value. */
- if (!TREE_SIDE_EFFECTS (op1))
- *expr_p = op0;
- else if (TREE_THIS_VOLATILE (op1)
- && (REFERENCE_CLASS_P (op1) || DECL_P (op1)))
- *expr_p = build2 (COMPOUND_EXPR, TREE_TYPE (*expr_p),
- build_fold_addr_expr (op1), op0);
- else
- *expr_p = build2 (COMPOUND_EXPR, TREE_TYPE (*expr_p),
- op0, op1);
+ /* Remove any copies of empty classes. Also drop volatile
+ variables on the RHS to avoid infinite recursion from
+ gimplify_expr trying to load the value. */
+ gimplify_expr (&TREE_OPERAND (*expr_p, 0), pre_p, post_p,
+ is_gimple_lvalue, fb_lvalue);
+ if (TREE_SIDE_EFFECTS (op1))
+ {
+ if (TREE_THIS_VOLATILE (op1)
+ && (REFERENCE_CLASS_P (op1) || DECL_P (op1)))
+ op1 = build_fold_addr_expr (op1);
+
+ gimplify_and_add (op1, pre_p);
+ }
+ *expr_p = TREE_OPERAND (*expr_p, 0);
}
}
ret = GS_OK;
@@ -740,6 +757,19 @@ cp_gimplify_expr (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p)
}
break;
+ case RETURN_EXPR:
+ if (TREE_OPERAND (*expr_p, 0)
+ && (TREE_CODE (TREE_OPERAND (*expr_p, 0)) == INIT_EXPR
+ || TREE_CODE (TREE_OPERAND (*expr_p, 0)) == MODIFY_EXPR))
+ {
+ expr_p = &TREE_OPERAND (*expr_p, 0);
+ code = TREE_CODE (*expr_p);
+ /* Avoid going through the INIT_EXPR case, which can
+ degrade INIT_EXPRs into AGGR_INIT_EXPRs. */
+ goto modify_expr_case;
+ }
+ /* 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" } } */
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [patch] Optimize empty class copies within a C++ return statement
2015-03-11 1:45 ` Aldy Hernandez
@ 2015-03-11 12:06 ` Jason Merrill
0 siblings, 0 replies; 15+ messages in thread
From: Jason Merrill @ 2015-03-11 12:06 UTC (permalink / raw)
To: Aldy Hernandez, gcc-patches
On 03/10/2015 09:45 PM, Aldy Hernandez wrote:
> + gimplify_expr (&TREE_OPERAND (*expr_p, 0), pre_p, post_p,
...
> + *expr_p = TREE_OPERAND (*expr_p, 0);
Can't you use op0 here, since you're about to throw away the MODIFY_EXPR?
OK either way.
Jason
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2015-03-11 12:06 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-03-05 23:25 [patch] Optimize empty class copies within a C++ return statement Aldy Hernandez
2015-03-06 6:20 ` Jason Merrill
2015-03-06 16:19 ` Aldy Hernandez
2015-03-06 21:46 ` Jason Merrill
2015-03-06 21:54 ` Aldy Hernandez
2015-03-06 22:01 ` Jason Merrill
2015-03-06 22:02 ` Jason Merrill
2015-03-09 18:33 ` Aldy Hernandez
2015-03-10 2:51 ` Jason Merrill
2015-03-10 23:11 ` Aldy Hernandez
2015-03-10 23:59 ` Jason Merrill
2015-03-11 0:07 ` Aldy Hernandez
2015-03-11 0:55 ` Jason Merrill
2015-03-11 1:45 ` Aldy Hernandez
2015-03-11 12:06 ` Jason Merrill
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).