From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id 116613858421 for ; Mon, 1 May 2023 19:07:47 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 116613858421 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1682968066; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=KYVg1CC3HJh201/+up7QQw0GMaP49NvOTxDfMbnV9bs=; b=cHoe6moBvsCZTjh0WcbL3y6YLmXULB/tDMfsSD07SW074yLvFmVgUhnjqylcUkLxFAkNEO IfBqXVKt0f3Sa/dkUi55EFGE8uRF6t90/6V8Puk1R0QLZw+NnOiq1aUtOHc6Yoc8gXC4Un leVtfyRAU3npG1gTtVZdnh8pmwNgg6U= Received: from mail-qt1-f197.google.com (mail-qt1-f197.google.com [209.85.160.197]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-547-YMn6gRMHMmm2d2u_AoOIDQ-1; Mon, 01 May 2023 15:07:36 -0400 X-MC-Unique: YMn6gRMHMmm2d2u_AoOIDQ-1 Received: by mail-qt1-f197.google.com with SMTP id d75a77b69052e-3ef344b76ccso14525031cf.0 for ; Mon, 01 May 2023 12:07:36 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1682968056; x=1685560056; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=KYVg1CC3HJh201/+up7QQw0GMaP49NvOTxDfMbnV9bs=; b=g7Hq4vpFCctTj20KwiPgkIqZYlN8vpJw0HvyzDQE9J0sCCooE1gppkRPrrATRJEoKI Os1J5/j8xdbw1HZLNnmtASd+oRklYqmHdiMS2M+r2oRlEMtAt2R9/OFSdQQXo8we5Nsz EFPTywPuSawiEyX+3zWd4pl4DKiuHWmUiOjYVbwOaR4I1pIR90O4gtFNTxPHbxcsw45s vWK3kItvmbJZJfpAN6jh+dLdq/wMLPZEBhOQZ4sQSOSG3q1J4yLfQbVhQ1qTrd9iNoqh qV/NItlQz46+zJAcDXzmBJVpIE3QzIm3znQQDCUOyla7Jqa/BZfO0Vyk7KO47d6CLKwa 1jOw== X-Gm-Message-State: AC+VfDzW+bqKFjN8KuTSSfHRZoH8LMhvedXiSKILcV/hr0pslu0wecv+ jx3p/pYFaekvUaD9bTq6ibXPyYmP0hDApCXpLKmSo3vbs1ae3viwxtSfHysh2zjFGAK2IFxFX7M RWj8sC+5Ii/VrhV7fHkfSggfjqw== X-Received: by 2002:a05:622a:650:b0:3e3:93f9:1907 with SMTP id a16-20020a05622a065000b003e393f91907mr23715785qtb.40.1682968055590; Mon, 01 May 2023 12:07:35 -0700 (PDT) X-Google-Smtp-Source: ACHHUZ7AmPT8O7SdngrW+A8xoaXvTFDLmdNl/rAWoz3o4nK4K+YhlpBuUxIfKldWg2tCnGntD0iclQ== X-Received: by 2002:a05:622a:650:b0:3e3:93f9:1907 with SMTP id a16-20020a05622a065000b003e393f91907mr23715734qtb.40.1682968055137; Mon, 01 May 2023 12:07:35 -0700 (PDT) Received: from [192.168.1.108] (130-44-146-16.s12558.c3-0.arl-cbr1.sbo-arl.ma.cable.rcncustomer.com. [130.44.146.16]) by smtp.gmail.com with ESMTPSA id eb11-20020a05620a480b00b0074411b03972sm9039488qkb.51.2023.05.01.12.07.34 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 01 May 2023 12:07:34 -0700 (PDT) Message-ID: <54682021-901d-30aa-9cb4-172f32793a9b@redhat.com> Date: Mon, 1 May 2023 15:07:33 -0400 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.10.1 Subject: Re: [PATCH] c++: RESULT_DECL replacement in constexpr call result [PR105440] To: Patrick Palka Cc: gcc-patches@gcc.gnu.org References: <20230428190508.4091082-1-ppalka@redhat.com> <8cb1d63e-2afb-df4a-21f2-20756e9630ba@idea> From: Jason Merrill In-Reply-To: X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-13.5 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On 4/28/23 15:40, Patrick Palka wrote: > On Fri, 28 Apr 2023, Patrick Palka wrote: > >> On Fri, 28 Apr 2023, Patrick Palka wrote: >> >>> After mechanically replacing RESULT_DECL within a constexpr call result >>> (for sake of RVO), we can in some cases simplify the call result >>> further. >>> >>> In the below testcase the result of get() during evaluation of a's >>> initializer is the self-referential CONSTRUCTOR: >>> >>> {._M_p=(char *) &._M_local_buf} >>> >>> which after replacing RESULT_DECL with ctx->object (aka *D.2603, where >>> the D.2603 temporary points to the current element of _M_elems under >>> construction) becomes: >>> >>> {._M_p=(char *) &D.2603->_M_local_buf} >>> >>> but what we really want is: >>> >>> {._M_p=(char *) &a._M_elems[0]._M_local_buf}. >>> >>> so that the value of _M_p is independent of the value of the mutable >>> D.2603 temporary. >>> >>> So to that end, it seems we should constexpr evaluate the result again >>> after RESULT_DECL replacement, which is what this patch implements. >>> >>> Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK for >>> trunk? >>> >>> PR libstdc++/105440 >>> >>> gcc/cp/ChangeLog: >>> >>> * constexpr.cc (cxx_eval_call_expression): If any RESULT_DECLs get >>> replaced in the call result, try further evaluating the result. >>> >>> gcc/testsuite/ChangeLog: >>> >>> * g++.dg/cpp2a/constexpr-dtor16.C: New test. >>> --- >>> gcc/cp/constexpr.cc | 12 +++++- >>> gcc/testsuite/g++.dg/cpp2a/constexpr-dtor16.C | 39 +++++++++++++++++++ >>> 2 files changed, 49 insertions(+), 2 deletions(-) >>> create mode 100644 gcc/testsuite/g++.dg/cpp2a/constexpr-dtor16.C >>> >>> diff --git a/gcc/cp/constexpr.cc b/gcc/cp/constexpr.cc >>> index d1097764b10..22a1609e664 100644 >>> --- a/gcc/cp/constexpr.cc >>> +++ b/gcc/cp/constexpr.cc >>> @@ -3213,7 +3213,12 @@ cxx_eval_call_expression (const constexpr_ctx *ctx, tree t, >>> && CLASS_TYPE_P (TREE_TYPE (res)) >>> && !is_empty_class (TREE_TYPE (res))) >>> if (replace_decl (&result, res, ctx->object)) >>> - cacheable = false; >>> + { >>> + cacheable = false; >>> + result = cxx_eval_constant_expression (ctx, result, lval, >>> + non_constant_p, >>> + overflow_p); >>> + } >>> } >>> else >>> /* Couldn't get a function copy to evaluate. */ >>> @@ -5988,9 +5993,12 @@ cxx_eval_store_expression (const constexpr_ctx *ctx, tree t, >>> object = probe; >>> else >>> { >>> + tree orig_probe = probe; >>> probe = cxx_eval_constant_expression (ctx, probe, vc_glvalue, >>> non_constant_p, overflow_p); >>> evaluated = true; >>> + if (orig_probe == target) >>> + target = probe; >> >> Whoops, thanks to an accidental git commit --amend this patch contains >> an alternative approach that I considered: in cxx_eval_store_expression, >> ensure that we always set ctx->object to a fully reduced result (so >> &a._M_elems[0] instead of of *D.2603 in this case), which means later >> RESULT_DECL replacement with ctx->object should yield an already reduced >> result as well. But with this approach I ran into a bogus "modifying >> const object" error on cpp1y/constexpr-tracking-const23.C so I gave up >> on it :( > > Ah, the problem was that later in cxx_eval_store_expression we were > suppressing a TREE_READONLY update via pattern matching on 'target', > but if we are now updating 'target' to its reduced value the pattern > matching needs to consider the shape of the original 'target' instead. > Here's an alternative fix for this PR that passes regression testing, > not sure which approach would be preferable. With this patch I think we'd still miss the case where the target is a subobject (so probe != target). And we would miss clearing cacheable in the case where we the value depends on ctx->object. OTOH, that seems like it might already be an issue when we go through here: > if (lval == vc_glvalue) > { > /* If we want to return a reference to the target, we need to evaluate it > as a whole; otherwise, only evaluate the innermost piece to avoid > building up unnecessary *_REFs. */ > target = cxx_eval_constant_expression (ctx, target, lval, > non_constant_p, overflow_p); So I think it makes sense to try to avoid the need for the replace_decl at all by doing the replacement earlier along the lines of this patch, and replace the replace_decl with looking for references to ctx->object in the result? > PR c++/105440 > > gcc/cp/ChangeLog: > > * constexpr.cc (cxx_eval_store_expression): Save the original > target in 'orig_target'. Update 'target' after evaluating it in > the 'probe' loop. Use 'orig_target' instead of 'target' when > appropriate. > > gcc/testsuite/ChangeLog: > > * g++.dg/cpp2a/constexpr-dtor16.C: New test. > --- > gcc/cp/constexpr.cc | 15 ++++++++++----- > 1 file changed, 10 insertions(+), 5 deletions(-) > > diff --git a/gcc/cp/constexpr.cc b/gcc/cp/constexpr.cc > index 9dbbf6eec03..2939ac89a98 100644 > --- a/gcc/cp/constexpr.cc > +++ b/gcc/cp/constexpr.cc > @@ -5902,8 +5902,10 @@ cxx_eval_store_expression (const constexpr_ctx *ctx, tree t, > /* Just ignore clobbers. */ > return void_node; > > + const tree orig_target = TREE_OPERAND (t, 0); > + > /* First we figure out where we're storing to. */ > - tree target = TREE_OPERAND (t, 0); > + tree target = orig_target; > > maybe_simplify_trivial_copy (target, init); > > @@ -5993,9 +5995,12 @@ cxx_eval_store_expression (const constexpr_ctx *ctx, tree t, > object = probe; > else > { > + bool is_target = (probe == target); > probe = cxx_eval_constant_expression (ctx, probe, vc_glvalue, > non_constant_p, overflow_p); > evaluated = true; > + if (is_target) > + target = probe; > if (*non_constant_p) > return t; > } > @@ -6159,7 +6164,7 @@ cxx_eval_store_expression (const constexpr_ctx *ctx, tree t, > if (!empty_base && !(same_type_ignoring_top_level_qualifiers_p > (initialized_type (init), type))) > { > - gcc_assert (is_empty_class (TREE_TYPE (target))); > + gcc_assert (is_empty_class (TREE_TYPE (orig_target))); > empty_base = true; > } > > @@ -6294,14 +6299,14 @@ cxx_eval_store_expression (const constexpr_ctx *ctx, tree t, > && TREE_CODE (*valp) == CONSTRUCTOR > && TYPE_READONLY (type)) > { > - if (INDIRECT_REF_P (target) > + if (INDIRECT_REF_P (orig_target) > && (is_this_parameter > - (tree_strip_nop_conversions (TREE_OPERAND (target, 0))))) > + (tree_strip_nop_conversions (TREE_OPERAND (orig_target, 0))))) > /* We've just initialized '*this' (perhaps via the target > constructor of a delegating constructor). Leave it up to the > caller that set 'this' to set TREE_READONLY appropriately. */ > gcc_checking_assert (same_type_ignoring_top_level_qualifiers_p > - (TREE_TYPE (target), type) || empty_base); > + (TREE_TYPE (orig_target), type) || empty_base); > else > TREE_READONLY (*valp) = true; > } > diff --git a/gcc/testsuite/g++.dg/cpp2a/constexpr-dtor16.C b/gcc/testsuite/g++.dg/cpp2a/constexpr-dtor16.C > new file mode 100644 > index 00000000000..707a3e025b1 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/cpp2a/constexpr-dtor16.C > @@ -0,0 +1,39 @@ > +// PR c++/105440 > +// { dg-do compile { target c++20 } } > + > +struct basic_string { > + char _M_local_buf[32]; > + char* _M_p; > + constexpr basic_string() : _M_p{_M_local_buf} { } > + constexpr void f() { if (_M_p) { } } > + constexpr ~basic_string() { if (_M_p) { } } > +}; > + > +template > +struct array { > + basic_string _M_elems[N]; > +}; > + > +constexpr basic_string get() { return {}; } > + > +constexpr bool f1() { > + array<1> a{get()}; > + a._M_elems[0].f(); > + > + return true; > +} > + > +constexpr bool f2() { > + array<2> a2{get(), get()}; > + array<3> a3{get(), get(), get()}; > + > + for (basic_string& e : a2._M_elems) > + e.f(); > + for (basic_string& e : a3._M_elems) > + e.f(); > + > + return true; > +} > + > +static_assert(f1()); > +static_assert(f2());