From: Patrick Palka <ppalka@redhat.com>
To: Patrick Palka <ppalka@redhat.com>
Cc: gcc-patches@gcc.gnu.org, jason@redhat.com
Subject: Re: [PATCH] c++: RESULT_DECL replacement in constexpr call result [PR105440]
Date: Fri, 28 Apr 2023 15:40:08 -0400 (EDT) [thread overview]
Message-ID: <d9515d6f-8217-e20c-1a05-3b9e15ab7e86@idea> (raw)
In-Reply-To: <8cb1d63e-2afb-df4a-21f2-20756e9630ba@idea>
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 *) &<retval>._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.
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<int N>
+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());
--
2.40.1.445.gf85cd430b1
>
> Here's the correct patch:
>
> 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 | 7 +++-
> gcc/testsuite/g++.dg/cpp2a/constexpr-dtor16.C | 39 +++++++++++++++++++
> 2 files changed, 45 insertions(+), 1 deletion(-)
> 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..9dbbf6eec03 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. */
> 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<int N>
> +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());
> --
> 2.40.1.445.gf85cd430b1
>
>
> > if (*non_constant_p)
> > return t;
> > }
> > @@ -6154,7 +6162,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 (TREE_OPERAND (t, 0))));
> > empty_base = 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<int N>
> > +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());
> > --
> > 2.40.1.445.gf85cd430b1
> >
> >
>
next prev parent reply other threads:[~2023-04-28 19:40 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-04-28 19:05 Patrick Palka
2023-04-28 19:14 ` Patrick Palka
2023-04-28 19:40 ` Patrick Palka [this message]
2023-05-01 19:07 ` Jason Merrill
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=d9515d6f-8217-e20c-1a05-3b9e15ab7e86@idea \
--to=ppalka@redhat.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=jason@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).