On Mon, 1 May 2023, Jason Merrill wrote:
> 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?
Sorry, I lost track of this patch...
That does seem better but admittedly tricky to get right. Would the
original approach (i.e. the second patch in this thread, that just
re-evaluates the result after doing replace_decl) be OK to go in for
GCC 15? While not ideal, it seems like a safe improvement to our
current mechanism.
>
> > 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());
>
>
