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



Reply via email to