| Message ID | 20200721190702.1137046-1-ppalka@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | c++: decl_constant_value and unsharing [PR96197] | expand |
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. > 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 >
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, "");
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: >
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); } /* 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();
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. > -- >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); > } > > /* 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(); >
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: -- >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();
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. Richard. > -- >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 >
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 >> >
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:
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? 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(-)