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?
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.
Since scalar_constant_value of a CONST_DECL should be its DECL_INITIAL
INTEGER_CST, that probably did work when we would see unlowered
enumerators. But apparently we don't anymore, so simplifying this makes
sense.
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?
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/cp/cp-gimplify.c | 2 +-
gcc/cp/cvt.c | 3 ++-
gcc/cp/init.c | 2 +-
gcc/cp/pt.c | 9 +++------
4 files changed, 7 insertions(+), 9 deletions(-)
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:
