* [PATCH] c++: RESULT_DECL replacement in constexpr call result [PR105440]
@ 2023-04-28 19:05 Patrick Palka
2023-04-28 19:14 ` Patrick Palka
0 siblings, 1 reply; 4+ messages in thread
From: Patrick Palka @ 2023-04-28 19:05 UTC (permalink / raw)
To: gcc-patches; +Cc: jason, Patrick Palka
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;
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] c++: RESULT_DECL replacement in constexpr call result [PR105440]
2023-04-28 19:05 [PATCH] c++: RESULT_DECL replacement in constexpr call result [PR105440] Patrick Palka
@ 2023-04-28 19:14 ` Patrick Palka
2023-04-28 19:40 ` Patrick Palka
0 siblings, 1 reply; 4+ messages in thread
From: Patrick Palka @ 2023-04-28 19:14 UTC (permalink / raw)
To: Patrick Palka; +Cc: gcc-patches, jason
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 :(
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
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] c++: RESULT_DECL replacement in constexpr call result [PR105440]
2023-04-28 19:14 ` Patrick Palka
@ 2023-04-28 19:40 ` Patrick Palka
2023-05-01 19:07 ` Jason Merrill
0 siblings, 1 reply; 4+ messages in thread
From: Patrick Palka @ 2023-04-28 19:40 UTC (permalink / raw)
To: Patrick Palka; +Cc: gcc-patches, jason
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
> >
> >
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] c++: RESULT_DECL replacement in constexpr call result [PR105440]
2023-04-28 19:40 ` Patrick Palka
@ 2023-05-01 19:07 ` Jason Merrill
0 siblings, 0 replies; 4+ messages in thread
From: Jason Merrill @ 2023-05-01 19:07 UTC (permalink / raw)
To: Patrick Palka; +Cc: gcc-patches
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 *) &<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.
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<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());
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-05-01 19:07 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-04-28 19:05 [PATCH] c++: RESULT_DECL replacement in constexpr call result [PR105440] Patrick Palka
2023-04-28 19:14 ` Patrick Palka
2023-04-28 19:40 ` Patrick Palka
2023-05-01 19:07 ` Jason Merrill
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).