From patchwork Tue Mar 10 23:10:56 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Aldy Hernandez X-Patchwork-Id: 448744 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 75DE31400B6 for ; Wed, 11 Mar 2015 10:11:11 +1100 (AEDT) Authentication-Results: ozlabs.org; dkim=pass reason="1024-bit key; unprotected key" header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b=Uk4prKLB; dkim-adsp=none (unprotected policy); dkim-atps=neutral DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :message-id:date:from:mime-version:to:subject:references :in-reply-to:content-type; q=dns; s=default; b=x+mso0ORlk43qdck2 W9CrYnW6s107/+sawUz6QHXwUCPgbdvsCodLjFWExYufZcUN1qZ00lkVCqYD6u+h 6DbcacghwOQYNNigfpdDsZVPChr8pGbGNKyb5xsq2aJDcVGqsfrEGqtqq13++qBO jJ8P6MNII1Pez/ecl7AQaWd/tE= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :message-id:date:from:mime-version:to:subject:references :in-reply-to:content-type; s=default; bh=QPzbRACdLZ3lJawDcNtruyT vAwE=; b=Uk4prKLBzmwPQsaakhZIlJBLvmWda8kSlPuFKvcerJ9C7UiiR53EgZc HhVqdLBbct42ZZnpLrCxw6p3E846HY7ptSoeuHcPN4rer4nSe38op9N+j5g2fu9k MZcwCFippIU0ASb4esv/c5j/wP4YwaJk7NoO69uWVv9h/O2ViXYg= Received: (qmail 123122 invoked by alias); 10 Mar 2015 23:11:03 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Received: (qmail 123105 invoked by uid 89); 10 Mar 2015 23:11:02 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.7 required=5.0 tests=AWL, BAYES_00, SPF_HELO_PASS, SPF_PASS, T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Tue, 10 Mar 2015 23:11:01 +0000 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id t2ANAxrs007232 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL) for ; Tue, 10 Mar 2015 19:10:59 -0400 Received: from reynosa.quesejoda.com (unused [10.10.51.165] (may be forged)) by int-mx13.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t2ANAvkI005981; Tue, 10 Mar 2015 19:10:58 -0400 Message-ID: <54FF7A00.7060705@redhat.com> Date: Tue, 10 Mar 2015 16:10:56 -0700 From: Aldy Hernandez User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.5.0 MIME-Version: 1.0 To: Jason Merrill , gcc-patches Subject: Re: [patch] Optimize empty class copies within a C++ return statement References: <54F8E5D3.4050607@redhat.com> <54F94715.4000208@redhat.com> <54F9D396.3000400@redhat.com> <54FA2046.9090500@redhat.com> <54FA21FD.9040609@redhat.com> <54FA23B4.3090402@redhat.com> <54FA23D7.3030301@redhat.com> <54FDE762.6040409@redhat.com> <54FE5C13.4050904@redhat.com> In-Reply-To: <54FE5C13.4050904@redhat.com> > 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 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 ;" 1 "gimple" } } */ +/* { dg-final { cleanup-tree-dump "gimple" } } */