On Wed, 22 Jul 2020, Richard Biener wrote:
> On Tue, Jul 21, 2020 at 9:08 PM Patrick Palka via Gcc-patches
> <gcc-patches@gcc.gnu.org> 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.
> >
> > Fortunately there's only six calls to these functions, two of which are
> > from cxx_eval_constant_expression where the unsharing is undesirable.
> > And in unify there is one call, to scalar_constant_value, that looks
> > like:
> >
> > case CONST_DECL:
> > if (DECL_TEMPLATE_PARM_P (parm))
> > return ...;
> > > if (arg != scalar_constant_value (parm))
> > return ...;
> >
> > where we are suspiciously testing for pointer equality despite
> > scalar_constant_value's unsharing behavior. This line seems to be dead
> > code however, so this patch replaces it with an appropriate gcc_assert.
> > Finally, this patch adds an explicit call to unshare_expr to the
> > remaining three callers.
> >
> > 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 in the PR falls from 5GB to 15MB according to -ftime-report.
> >
> > Bootstrapped and regtested on x86_64-pc-linux-gnu, and also tested on
> > cmcstl2 and a number of other libraries. Does this look OK to commit?
>
> Can you add the PRs testcase? Thanks for tracking this down! (but I can't
> approve the patch)
>
> Richard.
Here's a patch with a reduced reproducer that consumes >6GB memory
during constexpr evaluation without the patch and just a few MB with:
-- >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 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.
Fortunately there's just six calls to these functions, two of which are
from cxx_eval_constant_expression where the unsharing is undesirable.
And in unify there is one call, to scalar_constant_value, that looks
like:
case CONST_DECL:
if (DECL_TEMPLATE_PARM_P (parm))
return ...;
> if (arg != scalar_constant_value (parm))
return ...;
where we are suspiciously testing for pointer equality despite
scalar_constant_value's unsharing behavior. This line seems to be dead
code however, so this patch replaces it with an appropriate gcc_assert.
Finally, this patch adds an explicit call to unshare_expr to each of the
three remaining callers.
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.
Bootstrapped and regtested on x86_64-pc-linux-gnu, and also tested on
cmcstl2 and a number of other libraries. Does this look OK to commit?
gcc/cp/ChangeLog:
PR c++/96197
* cp-gimplify.c (cp_fold_maybe_rvalue): Call unshare_expr on the
result of decl_constant_value.
* cvt.c: Include gimplify.h.
(ocp_convert): Call unshare_expr on the result of
scalar_constant_value.
* init.c (constant_value_1): Don't call unshare_expr here,
so that callers can choose whether to unshare.
* pt.c (tsubst_copy): Call unshare_expr on the result of
scalar_constant_value.
(unify) <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/cp-gimplify.c | 2 +-
gcc/cp/cvt.c | 3 ++-
gcc/cp/init.c | 2 +-
gcc/cp/pt.c | 9 +++------
gcc/testsuite/g++.dg/cpp1y/constexpr-array8.C | 18 ++++++++++++++++++
5 files changed, 25 insertions(+), 9 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-array8.C
diff --git a/gcc/cp/cp-gimplify.c b/gcc/cp/cp-gimplify.c
index 0e949e29c5c..5c5c44dbc5d 100644
--- a/gcc/cp/cp-gimplify.c
+++ b/gcc/cp/cp-gimplify.c
@@ -2433,7 +2433,7 @@ cp_fold_maybe_rvalue (tree x, bool rval)
tree v = decl_constant_value (x);
if (v != x && v != error_mark_node)
{
- x = v;
+ x = unshare_expr (v);
continue;
}
}
diff --git a/gcc/cp/cvt.c b/gcc/cp/cvt.c
index c9e7b1ff044..a7e40a1fa51 100644
--- a/gcc/cp/cvt.c
+++ b/gcc/cp/cvt.c
@@ -36,6 +36,7 @@ along with GCC; see the file COPYING3. If not see
#include "stringpool.h"
#include "attribs.h"
#include "escaped_string.h"
+#include "gimplify.h"
static tree convert_to_pointer_force (tree, tree, tsubst_flags_t);
static tree build_type_conversion (tree, tree);
@@ -725,7 +726,7 @@ ocp_convert (tree type, tree expr, int convtype, int flags,
e = mark_rvalue_use (e);
tree v = scalar_constant_value (e);
if (!error_operand_p (v))
- e = v;
+ e = unshare_expr (v);
}
if (error_operand_p (e))
return error_mark_node;
diff --git a/gcc/cp/init.c b/gcc/cp/init.c
index ef4b3c4dc3c..bf229bd2ba5 100644
--- a/gcc/cp/init.c
+++ b/gcc/cp/init.c
@@ -2343,7 +2343,7 @@ 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;
}
diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c
index 34876788a9c..4d3ee099cea 100644
--- a/gcc/cp/pt.c
+++ b/gcc/cp/pt.c
@@ -16368,7 +16368,7 @@ tsubst_copy (tree t, tree args, tsubst_flags_t
complain, tree in_decl)
return t;
/* If ARGS is NULL, then T is known to be non-dependent. */
if (args == NULL_TREE)
- return scalar_constant_value (t);
+ return unshare_expr (scalar_constant_value (t));
/* Unfortunately, we cannot just call lookup_name here.
Consider:
@@ -23644,11 +23644,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..09603efeff4
--- /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 sum() {
+ int count = 0;
+ for (int i = 0; i < 5000; i++)
+ if (ary[i].p != 0)
+ count++;
+ return count;
+}
+
+static_assert(sum() == 5000, "");
--
2.28.0.rc1
