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.
Richard.
> -- >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
>
