On Tue, Jul 21, 2020 at 9:08 PM Patrick Palka via Gcc-patches
<[email protected]> 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.
> 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:
> --
> 2.28.0.rc1
>
