[prev in list] [next in list] [prev in thread] [next in thread]
List: gcc-patches
Subject: Re: [PATCH] c++: decl_constant_value and unsharing [PR96197]
From: Jason Merrill via Gcc-patches <gcc-patches () gcc ! gnu ! org>
Date: 2020-07-31 20:29:21
Message-ID: e9ba3b56-f389-0814-8d00-24025267e3be () redhat ! com
[Download RAW message or body]
On 7/31/20 2:07 AM, Richard Biener wrote:
> On Thu, Jul 30, 2020 at 8:55 PM Patrick Palka via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
> >
> > On Thu, 30 Jul 2020, Jason Merrill wrote:
> >
> > > On 7/30/20 9:49 AM, Patrick Palka wrote:
> > > > On Thu, 30 Jul 2020, Jason Merrill wrote:
> > > >
> > > > > On 7/21/20 3:07 PM, Patrick Palka wrote:
> > > > > > In the testcase from the PR we are seeing excessive memory use (> 5GB)
> > > > > > during constexpr evaluation, almost all of which is due to the call to
> > > > > > decl_constant_value in the VAR_DECL/CONST_DECL branch of
> > > > > > cxx_eval_constant_expression. We reach here every time we evaluate an
> > > > > > ARRAY_REF of a constexpr VAR_DECL, which in this testcase is quite
> > > > > > often, and from there decl_constant_value makes an unshared copy of the
> > > > > > VAR_DECL's initializer, even though the unsharing is not needed at this
> > > > > > call site (because it is up to callers of cxx_eval_constant_expression
> > > > > > to unshare).
> > > > > >
> > > > > > To fix this, this patch moves the responsibility of unsharing the result
> > > > > > of decl_constant_value, decl_really_constant_value and
> > > > > > scalar_constant_value from the callee to the caller.
> > > > >
> > > > > How about creating another entry point that doesn't unshare, and using
> > > > > that in
> > > > > constexpr evaluation?
> > > >
> > > > Is something like this what you have in mind? This adds a defaulted
> > > > bool parameter to the three routines that controls unsharing (except for
> > > > decl_constant_value, which instead needs a new overload if we don't want
> > > > to touch its common declaration in c-common.h.) Bootstrap and regtest
> > > > in progress.
> > >
> > > That looks good, though I don't think we need the added parameter for
> > > scalar_constant_value.
> >
> > Hmm, I guess it should always be cheap to unshare an scalar initializer.
> > So consider the parameter removed for scalar_constant_value.
> >
> > >
> > > > -- >8 --
> > > >
> > > > Subject: [PATCH] c++: decl_constant_value and unsharing [PR96197]
> > > >
> > > > In the testcase from the PR we are seeing excessive memory use (> 5GB)
> > > > during constexpr evaluation, almost all of which is due to the call to
> > > > decl_constant_value in the VAR_DECL/CONST_DECL branch of
> > > > cxx_eval_constant_expression. We reach here every time we evaluate an
> > > > ARRAY_REF of a constexpr VAR_DECL, which in this testcase is quite
> > > > often, and from there decl_constant_value makes an unshared copy of the
> > > > VAR_DECL's initializer, even though the unsharing is not needed at this
> > > > call site (because callers of cxx_eval_constant_expression already
> > > > unshare its result when necessary).
> > > >
> > > > To fix this excessive unsharing, this patch introduces a new defaulted
> > > > parameter unshare_p to scalar_constant_value, decl_really_constant_value
> > > > and decl_constant_value to allow callers to decide if these routines
> > > > should unshare their result before returning. (Since decl_constant_value
> > > > is declared in c-common.h, it instead gets a new overload declared in
> > > > cp-tree.h.)
> > > >
> > > > As a simplification, this patch also moves the call to unshare_expr in
> > > > constant_value_1 outside of the loop, since calling unshare_expr on a
> > > > DECL_P should be a no-op.
> > > >
> > > > Additionally, in unify there is one call to scalar_constant_value that
> > > > seems to be dead code since we apparently don't see unlowered
> > > > enumerators anymore, so this patch replaces it with an appropriate
> > > > gcc_assert.
> >
> > I'll also push this change as a separate patch, now that
> > scalar_constant_value is unrelated to the rest of the patch.
> >
> > Here is the main patch that I guess I'll commit after a full bootstrap
> > and regtest:
>
> Might be a good candidate for backporting to GCC 10 as well after
> a while - it at least looks simple enough.
Agreed.
>
> > -- >8 --
> >
> > Subject: [PATCH] c++: decl_constant_value and unsharing [PR96197]
> >
> > In the testcase from the PR we are seeing excessive memory use (> 5GB)
> > during constexpr evaluation, almost all of which is due to the call to
> > decl_constant_value in the VAR_DECL/CONST_DECL branch of
> > cxx_eval_constant_expression. We reach here every time we evaluate an
> > ARRAY_REF of a constexpr VAR_DECL, which in this testcase is quite
> > often, and from there decl_constant_value makes an unshared copy of the
> > VAR_DECL's initializer, even though the unsharing is not needed at this
> > call site (because callers of cxx_eval_constant_expression already
> > unshare its result when necessary).
> >
> > To fix this excessive unsharing, this patch introduces a new defaulted
> > parameter unshare_p to decl_really_constant_value and
> > decl_constant_value to allow callers to choose whether these routines
> > should unshare the returned result. (Since decl_constant_value is
> > declared in c-common.h, we introduce a new overload declared in
> > cp-tree.h instead of changing its existing declaration.)
> >
> > As a simplification, this patch also moves the call to unshare_expr in
> > constant_value_1 outside of the loop, since calling unshare_expr on a
> > DECL_P should be a no-op.
> >
> > Now that the the calls to decl_constant_value and
> > decl_really_constant_value from cxx_eval_constant_expression no longer
> > unshare their result, memory use during constexpr evaluation for the
> > testcase from the PR falls from ~5GB to 15MB according to -ftime-report.
> >
> > gcc/cp/ChangeLog:
> >
> > PR c++/96197
> > * constexpr.c (cxx_eval_constant_expression) <case CONST_DECL>:
> > Pass false to decl_constant_value and decl_really_constant_value
> > so that they don't unshare their result.
> > * cp-tree.h (decl_constant_value): New declaration with an added
> > bool parameter.
> > (decl_really_constant_value): Add bool parameter defaulting to
> > true to existing declaration.
> > * init.c (constant_value_1): Add bool parameter which controls
> > whether to unshare the initializer before returning. Call
> > unshare_expr at most once.
> > (scalar_constant_value): Pass true to constant_value_1's new
> > bool parameter.
> > (decl_really_constant_value): Add bool parameter and forward it
> > to constant_value_1.
> > (decl_constant_value): Likewise, but instead define a new
> > overload with an added bool parameter.
> >
> > gcc/testsuite/ChangeLog:
> >
> > PR c++/96197
> > * g++.dg/cpp1y/constexpr-array8.C: New test.
> > ---
> > gcc/cp/constexpr.c | 4 +--
> > gcc/cp/cp-tree.h | 3 +-
> > gcc/cp/init.c | 34 +++++++++++++------
> > gcc/testsuite/g++.dg/cpp1y/constexpr-array8.C | 18 ++++++++++
> > 4 files changed, 45 insertions(+), 14 deletions(-)
> > create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-array8.C
> >
> > diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c
> > index 97dcc1b1d10..b1c1d249c6e 100644
> > --- a/gcc/cp/constexpr.c
> > +++ b/gcc/cp/constexpr.c
> > @@ -5695,9 +5695,9 @@ cxx_eval_constant_expression (const constexpr_ctx *ctx, \
> > tree t, TREE_CONSTANT (r) = true;
> > }
> > else if (ctx->strict)
> > - r = decl_really_constant_value (t);
> > + r = decl_really_constant_value (t, /*unshare_p=*/false);
> > else
> > - r = decl_constant_value (t);
> > + r = decl_constant_value (t, /*unshare_p=*/false);
> > if (TREE_CODE (r) == TARGET_EXPR
> > && TREE_CODE (TARGET_EXPR_INITIAL (r)) == CONSTRUCTOR)
> > r = TARGET_EXPR_INITIAL (r);
> > diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
> > index ea4871f836a..1e583efd61d 100644
> > --- a/gcc/cp/cp-tree.h
> > +++ b/gcc/cp/cp-tree.h
> > @@ -6773,7 +6773,8 @@ extern tree build_vec_delete \
> > (location_t, tree, tree, extern tree create_temporary_var (tree);
> > extern void initialize_vtbl_ptrs (tree);
> > extern tree scalar_constant_value (tree);
> > -extern tree decl_really_constant_value (tree);
> > +extern tree decl_constant_value (tree, bool);
> > +extern tree decl_really_constant_value (tree, bool = true);
> > extern int diagnose_uninitialized_cst_or_ref_member (tree, bool, bool);
> > extern tree build_vtbl_address (tree);
> > extern bool maybe_reject_flexarray_init (tree, tree);
> > diff --git a/gcc/cp/init.c b/gcc/cp/init.c
> > index 913fa4a0080..04404a52167 100644
> > --- a/gcc/cp/init.c
> > +++ b/gcc/cp/init.c
> > @@ -2277,10 +2277,12 @@ build_offset_ref (tree type, tree member, bool address_p,
> > recursively); otherwise, return DECL. If STRICT_P, the
> > initializer is only returned if DECL is a
> > constant-expression. If RETURN_AGGREGATE_CST_OK_P, it is ok to
> > - return an aggregate constant. */
> > + return an aggregate constant. If UNSHARE_P, we unshare the
> > + intializer before returning it. */
> >
> > static tree
> > -constant_value_1 (tree decl, bool strict_p, bool return_aggregate_cst_ok_p)
> > +constant_value_1 (tree decl, bool strict_p, bool return_aggregate_cst_ok_p,
> > + bool unshare_p)
> > {
> > while (TREE_CODE (decl) == CONST_DECL
> > > > decl_constant_var_p (decl)
> > @@ -2348,9 +2350,9 @@ constant_value_1 (tree decl, bool strict_p, bool \
> > return_aggregate_cst_ok_p) && !DECL_INITIALIZED_BY_CONSTANT_EXPRESSION_P (decl)
> > && DECL_NONTRIVIALLY_INITIALIZED_P (decl))
> > break;
> > - decl = unshare_expr (init);
> > + decl = init;
> > }
> > - return decl;
> > + return unshare_p ? unshare_expr (decl) : decl;
> > }
> >
> > /* If DECL is a CONST_DECL, or a constant VAR_DECL initialized by constant
> > @@ -2362,26 +2364,36 @@ tree
> > scalar_constant_value (tree decl)
> > {
> > return constant_value_1 (decl, /*strict_p=*/true,
> > - /*return_aggregate_cst_ok_p=*/false);
> > + /*return_aggregate_cst_ok_p=*/false,
> > + /*unshare_p=*/true);
> > }
> >
> > -/* Like scalar_constant_value, but can also return aggregate initializers. */
> > +/* Like scalar_constant_value, but can also return aggregate initializers.
> > + If UNSHARE_P, we unshare the initializer before returning it. */
> >
> > tree
> > -decl_really_constant_value (tree decl)
> > +decl_really_constant_value (tree decl, bool unshare_p /*= true*/)
> > {
> > return constant_value_1 (decl, /*strict_p=*/true,
> > - /*return_aggregate_cst_ok_p=*/true);
> > + /*return_aggregate_cst_ok_p=*/true,
> > + /*unshare_p=*/unshare_p);
> > }
> >
> > -/* A more relaxed version of scalar_constant_value, used by the
> > +/* A more relaxed version of decl_really_constant_value, used by the
> > common C/C++ code. */
> >
> > tree
> > -decl_constant_value (tree decl)
> > +decl_constant_value (tree decl, bool unshare_p)
> > {
> > return constant_value_1 (decl, /*strict_p=*/processing_template_decl,
> > - /*return_aggregate_cst_ok_p=*/true);
> > + /*return_aggregate_cst_ok_p=*/true,
> > + /*unshare_p=*/unshare_p);
> > +}
> > +
> > +tree
> > +decl_constant_value (tree decl)
> > +{
> > + return decl_constant_value (decl, /*unshare_p=*/true);
> > }
> >
> > /* Common subroutines of build_new and build_vec_delete. */
> > diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-array8.C \
> > b/gcc/testsuite/g++.dg/cpp1y/constexpr-array8.C new file mode 100644
> > index 00000000000..339abb69019
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-array8.C
> > @@ -0,0 +1,18 @@
> > +// PR c++/96197
> > +// { dg-do compile { target c++14 } }
> > +
> > +struct S {
> > + S* p = this;
> > +};
> > +
> > +constexpr S ary[5000] = {};
> > +
> > +constexpr int foo() {
> > + int count = 0;
> > + for (int i = 0; i < 5000; i++)
> > + if (ary[i].p != nullptr)
> > + count++;
> > + return count;
> > +}
> > +
> > +constexpr int bar = foo();
> > --
> > 2.28.0.rc1
> >
>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic