diff mbox

Optimize empty class copies within a C++ return statement

Message ID 54FF7A00.7060705@redhat.com
State New
Headers show

Commit Message

Aldy Hernandez March 10, 2015, 11:10 p.m. UTC 
> 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
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.

Comments

Jason Merrill March 10, 2015, 11:59 p.m. UTC | #1 
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
Aldy Hernandez March 11, 2015, 12:07 a.m. UTC | #2 
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
Jason Merrill March 11, 2015, 12:55 a.m. UTC | #3 
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
diff mbox

Patch

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" } } */