From patchwork Fri Nov 5 10:07:42 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jakub Jelinek X-Patchwork-Id: 47099 Return-Path: X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id E2ECB385841D for ; Fri, 5 Nov 2021 10:08:21 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org E2ECB385841D DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1636106901; bh=e+WT1IGTafm9jbkBXzUC9KkrxLAzbJQ3Xav9GNOzB+E=; h=Date:To:Subject:References:In-Reply-To:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=qFaCWVLEK3SVeWcyKivxj3kuGhML+eo8q2Jeb2DHQ6IygPXXdj4IJy/Odz1UGs1wZ i9SZp1Y5MmNHHhj251inDIETSwB+GNyAUJdX5Se2oyvlCQy726EWy/e1HNjEi8fCu/ QTLbWcyHcaX3cjykbDrVS1G7zwR7Wl14TOETThls= X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTPS id 52ACB3858D35 for ; Fri, 5 Nov 2021 10:07:51 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 52ACB3858D35 Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-514-GZBOFs6YNrSW9T-uZxZudQ-1; Fri, 05 Nov 2021 06:07:46 -0400 X-MC-Unique: GZBOFs6YNrSW9T-uZxZudQ-1 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 24C94192296A for ; Fri, 5 Nov 2021 10:07:46 +0000 (UTC) Received: from tucnak.zalov.cz (unknown [10.39.193.172]) by smtp.corp.redhat.com (Postfix) with ESMTPS id AED265D6B1; Fri, 5 Nov 2021 10:07:45 +0000 (UTC) Received: from tucnak.zalov.cz (localhost [127.0.0.1]) by tucnak.zalov.cz (8.16.1/8.16.1) with ESMTPS id 1A5A7gPr1270549 (version=TLSv1.3 cipher=TLS_AES_256_GCM_SHA384 bits=256 verify=NOT); Fri, 5 Nov 2021 11:07:43 +0100 Received: (from jakub@localhost) by tucnak.zalov.cz (8.16.1/8.16.1/Submit) id 1A5A7ghx1270548; Fri, 5 Nov 2021 11:07:42 +0100 Date: Fri, 5 Nov 2021 11:07:42 +0100 To: Jason Merrill Subject: [PATCH] c++, v2: Fix up -fstrong-eval-order handling of call arguments [PR70796] Message-ID: <20211105100742.GI304296@tucnak> References: <20211104140757.GX304296@tucnak> MIME-Version: 1.0 In-Reply-To: <20211104140757.GX304296@tucnak> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Disposition: inline X-Spam-Status: No, score=-0.9 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, KAM_SOMETLD_ARE_BAD_TLD, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_NONE, TXREP autolearn=no autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-Patchwork-Original-From: Jakub Jelinek via Gcc-patches From: Jakub Jelinek Reply-To: Jakub Jelinek Cc: gcc-patches@gcc.gnu.org Errors-To: gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org Sender: "Gcc-patches" On Thu, Nov 04, 2021 at 03:07:57PM +0100, Jakub Jelinek via Gcc-patches wrote: > For the METHOD_TYPE first argument > I use a temporary always though, that should be always is_gimple_reg_type... Doing so regressed +FAIL: g++.dg/cpp1z/inh-ctor23.C -std=gnu++11 scan-tree-dump gimple "V::V .this, _1.;" +FAIL: g++.dg/cpp1z/inh-ctor23.C -std=gnu++11 scan-tree-dump gimple "Y::Y ._2, _3.;" +FAIL: g++.dg/cpp1z/inh-ctor23.C -std=gnu++14 scan-tree-dump gimple "V::V .this, _1.;" +FAIL: g++.dg/cpp1z/inh-ctor23.C -std=gnu++14 scan-tree-dump gimple "Y::Y ._2, _3.;" +FAIL: g++.dg/cpp1z/inh-ctor23.C -std=gnu++17 scan-tree-dump gimple "V::V .this, _1.;" +FAIL: g++.dg/cpp1z/inh-ctor23.C -std=gnu++17 scan-tree-dump gimple "Y::Y ._2, _3.;" +FAIL: g++.dg/cpp1z/inh-ctor23.C -std=gnu++20 scan-tree-dump gimple "V::V .this, _1.;" +FAIL: g++.dg/cpp1z/inh-ctor23.C -std=gnu++20 scan-tree-dump gimple "Y::Y ._2, _3.;" +FAIL: g++.dg/cpp1z/inh-ctor23.C -std=gnu++2b scan-tree-dump gimple "V::V .this, _1.;" +FAIL: g++.dg/cpp1z/inh-ctor23.C -std=gnu++2b scan-tree-dump gimple "Y::Y ._2, _3.;" because the testcase relies on this being passed directly in gimple dump, rather than some SSA_NAME based on this. Instead of changing the testcase, I've figured out that it is actually quite easy to restore previous behavior here, for 2 reasons even. One is that there are no side-effects in the ctor call arguments, so the forcing of this into a temporary wasn't really needed, we can like in the other cases quite cheaply see if the call has any side-effect arguments. And the other reason is that in C++ this can't be modified, and similarly vars with reference type can't be modified, so for those we don't need to force them into a temporary either even if there are side-effects. This means e.g. on struct S { void foo (S &, int); void bar (int); }; void S::foo (S &p, int x) { this->bar (++x); p.bar (++x); } we can keep what we were emitting before even for -std=c++17. Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk and after a while for 7.3 too? 2021-11-05 Jakub Jelinek PR c++/70796 * cp-gimplify.c (cp_gimplify_arg): New function. (cp_gimplify_expr): Use cp_gimplify_arg instead of gimplify_arg, pass true as last argument to it if there are any following arguments in strong evaluation order with side-effects. * g++.dg/cpp1z/eval-order11.C: New test. Jakub --- gcc/cp/cp-gimplify.c.jj 2021-10-29 19:33:10.542344939 +0200 +++ gcc/cp/cp-gimplify.c 2021-11-05 00:41:29.124227336 +0100 @@ -398,6 +398,47 @@ gimplify_to_rvalue (tree *expr_p, gimple return t; } +/* Like gimplify_arg, but if ORDERED is set (which should be set if + any of the arguments this argument is sequenced before has + TREE_SIDE_EFFECTS set, make sure expressions with is_gimple_reg_type type + are gimplified into SSA_NAME or a fresh temporary and for + non-is_gimple_reg_type we don't optimize away TARGET_EXPRs. */ + +static enum gimplify_status +cp_gimplify_arg (tree *arg_p, gimple_seq *pre_p, location_t call_location, + bool ordered) +{ + enum gimplify_status t; + if (ordered + && !is_gimple_reg_type (TREE_TYPE (*arg_p)) + && TREE_CODE (*arg_p) == TARGET_EXPR) + { + /* gimplify_arg would strip away the TARGET_EXPR, but + that can mean we don't copy the argument and some following + argument with side-effect could modify it. */ + protected_set_expr_location (*arg_p, call_location); + return gimplify_expr (arg_p, pre_p, NULL, is_gimple_lvalue, fb_either); + } + else + { + t = gimplify_arg (arg_p, pre_p, call_location); + if (t == GS_ERROR) + return GS_ERROR; + else if (ordered + && is_gimple_reg_type (TREE_TYPE (*arg_p)) + && is_gimple_variable (*arg_p) + && TREE_CODE (*arg_p) != SSA_NAME + /* No need to force references into register, references + can't be modified. */ + && !TYPE_REF_P (TREE_TYPE (*arg_p)) + /* And this can't be modified either. */ + && *arg_p != current_class_ptr) + *arg_p = get_initialized_tmp_var (*arg_p, pre_p); + return t; + } + +} + /* Do C++-specific gimplification. Args are as for gimplify_expr. */ int @@ -613,7 +654,8 @@ cp_gimplify_expr (tree *expr_p, gimple_s gcc_assert (call_expr_nargs (*expr_p) == 2); gcc_assert (!CALL_EXPR_ORDERED_ARGS (*expr_p)); enum gimplify_status t - = gimplify_arg (&CALL_EXPR_ARG (*expr_p, 1), pre_p, loc); + = cp_gimplify_arg (&CALL_EXPR_ARG (*expr_p, 1), pre_p, loc, + TREE_SIDE_EFFECTS (CALL_EXPR_ARG (*expr_p, 0))); if (t == GS_ERROR) ret = GS_ERROR; } @@ -622,10 +664,18 @@ cp_gimplify_expr (tree *expr_p, gimple_s /* Leave the last argument for gimplify_call_expr, to avoid problems with __builtin_va_arg_pack(). */ int nargs = call_expr_nargs (*expr_p) - 1; + int last_side_effects_arg = -1; + for (int i = nargs; i > 0; --i) + if (TREE_SIDE_EFFECTS (CALL_EXPR_ARG (*expr_p, i))) + { + last_side_effects_arg = i; + break; + } for (int i = 0; i < nargs; ++i) { enum gimplify_status t - = gimplify_arg (&CALL_EXPR_ARG (*expr_p, i), pre_p, loc); + = cp_gimplify_arg (&CALL_EXPR_ARG (*expr_p, i), pre_p, loc, + i < last_side_effects_arg); if (t == GS_ERROR) ret = GS_ERROR; } @@ -639,8 +689,17 @@ cp_gimplify_expr (tree *expr_p, gimple_s fntype = TREE_TYPE (fntype); if (TREE_CODE (fntype) == METHOD_TYPE) { + int nargs = call_expr_nargs (*expr_p); + bool side_effects = false; + for (int i = 1; i < nargs; ++i) + if (TREE_SIDE_EFFECTS (CALL_EXPR_ARG (*expr_p, i))) + { + side_effects = true; + break; + } enum gimplify_status t - = gimplify_arg (&CALL_EXPR_ARG (*expr_p, 0), pre_p, loc); + = cp_gimplify_arg (&CALL_EXPR_ARG (*expr_p, 0), pre_p, loc, + side_effects); if (t == GS_ERROR) ret = GS_ERROR; } --- gcc/testsuite/g++.dg/cpp1z/eval-order11.C.jj 2021-11-04 14:02:50.439482571 +0100 +++ gcc/testsuite/g++.dg/cpp1z/eval-order11.C 2021-11-04 14:15:43.850479169 +0100 @@ -0,0 +1,89 @@ +// PR c++/70796 +// { dg-do run { target c++11 } } +// { dg-options "-fstrong-eval-order" { target c++14_down } } + +struct A +{ + int x = 0; + A & operator ++ () { ++x; return *this; } +}; +struct B +{ + A first, second; + B (A x, A y) : first{x}, second{y} {} +}; +struct C +{ + int first, second; + C (int x, int y) : first{x}, second{y} {} +}; +struct D +{ + int d; + void foo (int x, D *y) + { + if (y != this + 1) + __builtin_abort (); + d = x; + } +}; +D d[2] = { { 1 }, { 2 } }; + +void +foo () +{ + int i = 0; + C p{++i, ++i}; + if (p.first != 1 || p.second != 2) + __builtin_abort (); +} + +void +bar () +{ + int i = 0; + C p{++i, ++i}; + if (p.first != 1 || p.second != 2) + __builtin_abort (); + int &j = i; + C q{++j, ++j}; + if (q.first != 3 || q.second != 4) + __builtin_abort (); +} + +void +baz () +{ + int i = 0; + C p{(int &) ++i, (int &) ++i}; + if (p.first != 1 || p.second != 2) + __builtin_abort (); +} + +void +qux () +{ + A i; + B p{++i, ++i}; + if (p.first.x != 1 || p.second.x != 2) + __builtin_abort (); +} + +void +corge () +{ + D *p = &d[0]; + p->foo (3, ++p); + if (d[0].d != 3 || d[1].d != 2) + __builtin_abort (); +} + +int +main () +{ + bar (); + baz (); + foo (); + qux (); + corge (); +}