Re: [PATCH] c++: decl_constant_value and unsharing [PR96197]

[Patrick Palka <ppalka@redhat.com>]

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.

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

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 as the 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
	additional bool parameter.
	(scalar_constant_value): Add defaulted bool parameter.
	(decl_really_constant_value): Likewise.
	* cp-gimplify.c (cp_fold_maybe_rvalue): Call unshare_expr on the
	result of decl_constant_value.
	* init.c (constant_value_1): Add bool parameter.  Conditionally
	unshare the initializer only before returning.
	(scalar_constant_value): Add bool parameter and pass it to
	constant_value_1
	(decl_really_constant_value): Likewise.
	(decl_constant_value): New overload with an additional bool
	parameter.
	* pt.c (tsubst_copy) <case CONST_DECL>: Assert
	DECL_TEMPLATE_PARM_P and simplify accordingly.

gcc/testsuite/ChangeLog:

	PR c++/96197
	* g++.dg/cpp1y/constexpr-array8.C: New test.
---
 gcc/cp/constexpr.c                            |  4 +--
 gcc/cp/cp-tree.h                              |  5 +--
 gcc/cp/init.c                                 | 35 ++++++++++++-------
 gcc/cp/pt.c                                   |  7 ++--
 gcc/testsuite/g++.dg/cpp1y/constexpr-array8.C | 18 ++++++++++
 5 files changed, 48 insertions(+), 21 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..e32e5bc8eb2 100644
--- a/gcc/cp/cp-tree.h
+++ b/gcc/cp/cp-tree.h
@@ -6772,8 +6772,9 @@ extern tree build_vec_delete			(location_t, tree, tree,
 						 tsubst_flags_t);
 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 scalar_constant_value		(tree, bool = true);
+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..8298ec17fde 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.  The returned initializer is unshared
+   iff UNSHARE_P. */
 
 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,40 +2350,49 @@ 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
    of integral or enumeration type, or a constexpr variable of scalar type,
    then return that value.  These are those variables permitted in constant
-   expressions by [5.19/1].  */
+   expressions by [5.19/1].  The returned value is unshared iff UNSHARE_P.  */
 
 tree
-scalar_constant_value (tree decl)
+scalar_constant_value (tree decl, bool unshare_p /*= true*/)
 {
   return constant_value_1 (decl, /*strict_p=*/true,
-			   /*return_aggregate_cst_ok_p=*/false);
+			   /*return_aggregate_cst_ok_p=*/false,
+			   /*unshare_p=*/unshare_p);
 }
 
 /* Like scalar_constant_value, but can also return aggregate initializers.  */
 
 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);
 }
 \f
 /* Common subroutines of build_new and build_vec_delete.  */
diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c
index 72bdf869b55..3a168d506cf 100644
--- a/gcc/cp/pt.c
+++ b/gcc/cp/pt.c
@@ -23664,11 +23664,8 @@ unify (tree tparms, tree targs, tree parm, tree arg, int strict,
 		    strict, explain_p);
 
     case CONST_DECL:
-      if (DECL_TEMPLATE_PARM_P (parm))
-	return unify (tparms, targs, DECL_INITIAL (parm), arg, strict, explain_p);
-      if (arg != scalar_constant_value (parm))
-	return unify_template_argument_mismatch (explain_p, parm, arg);
-      return unify_success (explain_p);
+      gcc_assert (DECL_TEMPLATE_PARM_P (parm));
+      return unify (tparms, targs, DECL_INITIAL (parm), arg, strict, explain_p);
 
     case FIELD_DECL:
     case TEMPLATE_DECL:
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