* [PATCH] c++: decl_constant_value and unsharing [PR96197]
@ 2020-07-21 19:07 Patrick Palka
2020-07-22 6:18 ` Richard Biener
2020-07-30 4:31 ` Jason Merrill
0 siblings, 2 replies; 9+ messages in thread
From: Patrick Palka @ 2020-07-21 19:07 UTC (permalink / raw)
To: gcc-patches
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(-)
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] c++: decl_constant_value and unsharing [PR96197]
2020-07-21 19:07 [PATCH] c++: decl_constant_value and unsharing [PR96197] Patrick Palka
@ 2020-07-22 6:18 ` Richard Biener
2020-07-22 12:57 ` Patrick Palka
2020-07-30 4:31 ` Jason Merrill
1 sibling, 1 reply; 9+ messages in thread
From: Richard Biener @ 2020-07-22 6:18 UTC (permalink / raw)
To: Patrick Palka; +Cc: GCC Patches
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
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] c++: decl_constant_value and unsharing [PR96197]
2020-07-22 6:18 ` Richard Biener
@ 2020-07-22 12:57 ` Patrick Palka
0 siblings, 0 replies; 9+ messages in thread
From: Patrick Palka @ 2020-07-22 12:57 UTC (permalink / raw)
To: Richard Biener; +Cc: Patrick Palka, GCC Patches
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] c++: decl_constant_value and unsharing [PR96197]
2020-07-21 19:07 [PATCH] c++: decl_constant_value and unsharing [PR96197] Patrick Palka
2020-07-22 6:18 ` Richard Biener
@ 2020-07-30 4:31 ` Jason Merrill
2020-07-30 13:49 ` Patrick Palka
1 sibling, 1 reply; 9+ messages in thread
From: Jason Merrill @ 2020-07-30 4:31 UTC (permalink / raw)
To: Patrick Palka, gcc-patches
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:
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] c++: decl_constant_value and unsharing [PR96197]
2020-07-30 4:31 ` Jason Merrill
@ 2020-07-30 13:49 ` Patrick Palka
2020-07-30 14:55 ` Jason Merrill
0 siblings, 1 reply; 9+ messages in thread
From: Patrick Palka @ 2020-07-30 13:49 UTC (permalink / raw)
To: Jason Merrill; +Cc: Patrick Palka, gcc-patches
On Thu, 30 Jul 2020, Jason Merrill wrote:
> On 7/21/20 3:07 PM, Patrick Palka wrote:
> > In the testcase from the PR we are seeing excessive memory use (> 5GB)
> > during constexpr evaluation, almost all of which is due to the call to
> > decl_constant_value in the VAR_DECL/CONST_DECL branch of
> > cxx_eval_constant_expression. We reach here every time we evaluate an
> > ARRAY_REF of a constexpr VAR_DECL, which in this testcase is quite
> > often, and from there decl_constant_value makes an unshared copy of the
> > VAR_DECL's initializer, even though the unsharing is not needed at this
> > call site (because it is up to callers of cxx_eval_constant_expression
> > to unshare).
> >
> > To fix this, this patch moves the responsibility of unsharing the result
> > of decl_constant_value, decl_really_constant_value and
> > scalar_constant_value from the callee to the caller.
>
> How about creating another entry point that doesn't unshare, and using that in
> constexpr evaluation?
Is something like this what you have in mind? This adds a defaulted
bool parameter to the three routines that controls unsharing (except for
decl_constant_value, which instead needs a new overload if we don't want
to touch its common declaration in c-common.h.) Bootstrap and regtest
in progress.
-- >8 --
Subject: [PATCH] c++: decl_constant_value and unsharing [PR96197]
In the testcase from the PR we are seeing excessive memory use (> 5GB)
during constexpr evaluation, almost all of which is due to the call to
decl_constant_value in the VAR_DECL/CONST_DECL branch of
cxx_eval_constant_expression. We reach here every time we evaluate an
ARRAY_REF of a constexpr VAR_DECL, which in this testcase is quite
often, and from there decl_constant_value makes an unshared copy of the
VAR_DECL's initializer, even though the unsharing is not needed at this
call site (because callers of cxx_eval_constant_expression already
unshare its result when necessary).
To fix this excessive unsharing, this patch introduces a new defaulted
parameter unshare_p to scalar_constant_value, decl_really_constant_value
and decl_constant_value to allow callers to decide if these routines
should unshare their result before returning. (Since decl_constant_value
is declared in c-common.h, it instead gets a new overload declared in
cp-tree.h.)
As a simplification, this patch also moves the call to unshare_expr in
constant_value_1 outside of the loop, since calling unshare_expr on a
DECL_P should be a no-op.
Additionally, in unify there is one call to scalar_constant_value that
seems to be dead code since we apparently don't see unlowered
enumerators anymore, so this patch replaces it with an appropriate
gcc_assert.
Now that the the calls to decl_constant_value and
decl_really_constant_value from cxx_eval_constant_expression no longer
unshare their result, memory use during constexpr evaluation for the
testcase from the PR falls from ~5GB to 15MB according to -ftime-report.
gcc/cp/ChangeLog:
PR c++/96197
* constexpr.c (cxx_eval_constant_expression) <case CONST_DECL>:
Pass false as the decl_constant_value and
decl_really_constant_value so that they don't unshare their
result.
* cp-tree.h (decl_constant_value): New declaration with an
additional bool parameter.
(scalar_constant_value): Add defaulted bool parameter.
(decl_really_constant_value): Likewise.
* cp-gimplify.c (cp_fold_maybe_rvalue): Call unshare_expr on the
result of decl_constant_value.
* init.c (constant_value_1): Add bool parameter. Conditionally
unshare the initializer only before returning.
(scalar_constant_value): Add bool parameter and pass it to
constant_value_1
(decl_really_constant_value): Likewise.
(decl_constant_value): New overload with an additional bool
parameter.
* pt.c (tsubst_copy) <case CONST_DECL>: Assert
DECL_TEMPLATE_PARM_P and simplify accordingly.
gcc/testsuite/ChangeLog:
PR c++/96197
* g++.dg/cpp1y/constexpr-array8.C: New test.
---
gcc/cp/constexpr.c | 4 +--
gcc/cp/cp-tree.h | 5 +--
gcc/cp/init.c | 35 ++++++++++++-------
gcc/cp/pt.c | 7 ++--
gcc/testsuite/g++.dg/cpp1y/constexpr-array8.C | 18 ++++++++++
5 files changed, 48 insertions(+), 21 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-array8.C
diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c
index 97dcc1b1d10..b1c1d249c6e 100644
--- a/gcc/cp/constexpr.c
+++ b/gcc/cp/constexpr.c
@@ -5695,9 +5695,9 @@ cxx_eval_constant_expression (const constexpr_ctx *ctx, tree t,
TREE_CONSTANT (r) = true;
}
else if (ctx->strict)
- r = decl_really_constant_value (t);
+ r = decl_really_constant_value (t, /*unshare_p=*/false);
else
- r = decl_constant_value (t);
+ r = decl_constant_value (t, /*unshare_p=*/false);
if (TREE_CODE (r) == TARGET_EXPR
&& TREE_CODE (TARGET_EXPR_INITIAL (r)) == CONSTRUCTOR)
r = TARGET_EXPR_INITIAL (r);
diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
index ea4871f836a..e32e5bc8eb2 100644
--- a/gcc/cp/cp-tree.h
+++ b/gcc/cp/cp-tree.h
@@ -6772,8 +6772,9 @@ extern tree build_vec_delete (location_t, tree, tree,
tsubst_flags_t);
extern tree create_temporary_var (tree);
extern void initialize_vtbl_ptrs (tree);
-extern tree scalar_constant_value (tree);
-extern tree decl_really_constant_value (tree);
+extern tree decl_constant_value (tree, bool);
+extern tree scalar_constant_value (tree, bool = true);
+extern tree decl_really_constant_value (tree, bool = true);
extern int diagnose_uninitialized_cst_or_ref_member (tree, bool, bool);
extern tree build_vtbl_address (tree);
extern bool maybe_reject_flexarray_init (tree, tree);
diff --git a/gcc/cp/init.c b/gcc/cp/init.c
index 913fa4a0080..8298ec17fde 100644
--- a/gcc/cp/init.c
+++ b/gcc/cp/init.c
@@ -2277,10 +2277,12 @@ build_offset_ref (tree type, tree member, bool address_p,
recursively); otherwise, return DECL. If STRICT_P, the
initializer is only returned if DECL is a
constant-expression. If RETURN_AGGREGATE_CST_OK_P, it is ok to
- return an aggregate constant. */
+ return an aggregate constant. The returned initializer is unshared
+ iff UNSHARE_P. */
static tree
-constant_value_1 (tree decl, bool strict_p, bool return_aggregate_cst_ok_p)
+constant_value_1 (tree decl, bool strict_p, bool return_aggregate_cst_ok_p,
+ bool unshare_p)
{
while (TREE_CODE (decl) == CONST_DECL
|| decl_constant_var_p (decl)
@@ -2348,40 +2350,49 @@ constant_value_1 (tree decl, bool strict_p, bool return_aggregate_cst_ok_p)
&& !DECL_INITIALIZED_BY_CONSTANT_EXPRESSION_P (decl)
&& DECL_NONTRIVIALLY_INITIALIZED_P (decl))
break;
- decl = unshare_expr (init);
+ decl = init;
}
- return decl;
+ return unshare_p ? unshare_expr (decl) : decl;
}
/* If DECL is a CONST_DECL, or a constant VAR_DECL initialized by constant
of integral or enumeration type, or a constexpr variable of scalar type,
then return that value. These are those variables permitted in constant
- expressions by [5.19/1]. */
+ expressions by [5.19/1]. The returned value is unshared iff UNSHARE_P. */
tree
-scalar_constant_value (tree decl)
+scalar_constant_value (tree decl, bool unshare_p /*= true*/)
{
return constant_value_1 (decl, /*strict_p=*/true,
- /*return_aggregate_cst_ok_p=*/false);
+ /*return_aggregate_cst_ok_p=*/false,
+ /*unshare_p=*/unshare_p);
}
/* Like scalar_constant_value, but can also return aggregate initializers. */
tree
-decl_really_constant_value (tree decl)
+decl_really_constant_value (tree decl, bool unshare_p /*= true*/)
{
return constant_value_1 (decl, /*strict_p=*/true,
- /*return_aggregate_cst_ok_p=*/true);
+ /*return_aggregate_cst_ok_p=*/true,
+ /*unshare_p=*/unshare_p);
}
-/* A more relaxed version of scalar_constant_value, used by the
+/* A more relaxed version of decl_really_constant_value, used by the
common C/C++ code. */
tree
-decl_constant_value (tree decl)
+decl_constant_value (tree decl, bool unshare_p)
{
return constant_value_1 (decl, /*strict_p=*/processing_template_decl,
- /*return_aggregate_cst_ok_p=*/true);
+ /*return_aggregate_cst_ok_p=*/true,
+ /*unshare_p=*/unshare_p);
+}
+
+tree
+decl_constant_value (tree decl)
+{
+ return decl_constant_value (decl, /*unshare_p=*/true);
}
\f
/* Common subroutines of build_new and build_vec_delete. */
diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c
index 72bdf869b55..3a168d506cf 100644
--- a/gcc/cp/pt.c
+++ b/gcc/cp/pt.c
@@ -23664,11 +23664,8 @@ unify (tree tparms, tree targs, tree parm, tree arg, int strict,
strict, explain_p);
case CONST_DECL:
- if (DECL_TEMPLATE_PARM_P (parm))
- return unify (tparms, targs, DECL_INITIAL (parm), arg, strict, explain_p);
- if (arg != scalar_constant_value (parm))
- return unify_template_argument_mismatch (explain_p, parm, arg);
- return unify_success (explain_p);
+ gcc_assert (DECL_TEMPLATE_PARM_P (parm));
+ return unify (tparms, targs, DECL_INITIAL (parm), arg, strict, explain_p);
case FIELD_DECL:
case TEMPLATE_DECL:
diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-array8.C b/gcc/testsuite/g++.dg/cpp1y/constexpr-array8.C
new file mode 100644
index 00000000000..339abb69019
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-array8.C
@@ -0,0 +1,18 @@
+// PR c++/96197
+// { dg-do compile { target c++14 } }
+
+struct S {
+ S* p = this;
+};
+
+constexpr S ary[5000] = {};
+
+constexpr int foo() {
+ int count = 0;
+ for (int i = 0; i < 5000; i++)
+ if (ary[i].p != nullptr)
+ count++;
+ return count;
+}
+
+constexpr int bar = foo();
--
2.28.0.rc1
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] c++: decl_constant_value and unsharing [PR96197]
2020-07-30 13:49 ` Patrick Palka
@ 2020-07-30 14:55 ` Jason Merrill
2020-07-30 18:54 ` Patrick Palka
0 siblings, 1 reply; 9+ messages in thread
From: Jason Merrill @ 2020-07-30 14:55 UTC (permalink / raw)
To: Patrick Palka; +Cc: gcc-patches
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);
> }
> \f
> /* Common subroutines of build_new and build_vec_delete. */
> diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c
> index 72bdf869b55..3a168d506cf 100644
> --- a/gcc/cp/pt.c
> +++ b/gcc/cp/pt.c
> @@ -23664,11 +23664,8 @@ unify (tree tparms, tree targs, tree parm, tree arg, int strict,
> strict, explain_p);
>
> case CONST_DECL:
> - if (DECL_TEMPLATE_PARM_P (parm))
> - return unify (tparms, targs, DECL_INITIAL (parm), arg, strict, explain_p);
> - if (arg != scalar_constant_value (parm))
> - return unify_template_argument_mismatch (explain_p, parm, arg);
> - return unify_success (explain_p);
> + gcc_assert (DECL_TEMPLATE_PARM_P (parm));
> + return unify (tparms, targs, DECL_INITIAL (parm), arg, strict, explain_p);
>
> case FIELD_DECL:
> case TEMPLATE_DECL:
> diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-array8.C b/gcc/testsuite/g++.dg/cpp1y/constexpr-array8.C
> new file mode 100644
> index 00000000000..339abb69019
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-array8.C
> @@ -0,0 +1,18 @@
> +// PR c++/96197
> +// { dg-do compile { target c++14 } }
> +
> +struct S {
> + S* p = this;
> +};
> +
> +constexpr S ary[5000] = {};
> +
> +constexpr int foo() {
> + int count = 0;
> + for (int i = 0; i < 5000; i++)
> + if (ary[i].p != nullptr)
> + count++;
> + return count;
> +}
> +
> +constexpr int bar = foo();
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] c++: decl_constant_value and unsharing [PR96197]
2020-07-30 14:55 ` Jason Merrill
@ 2020-07-30 18:54 ` Patrick Palka
2020-07-31 6:07 ` Richard Biener
0 siblings, 1 reply; 9+ messages in thread
From: Patrick Palka @ 2020-07-30 18:54 UTC (permalink / raw)
To: Jason Merrill; +Cc: Patrick Palka, gcc-patches
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);
}
\f
/* 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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] c++: decl_constant_value and unsharing [PR96197]
2020-07-30 18:54 ` Patrick Palka
@ 2020-07-31 6:07 ` Richard Biener
2020-07-31 20:29 ` Jason Merrill
0 siblings, 1 reply; 9+ messages in thread
From: Richard Biener @ 2020-07-31 6:07 UTC (permalink / raw)
To: Patrick Palka; +Cc: Jason Merrill, GCC Patches
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
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] c++: decl_constant_value and unsharing [PR96197]
2020-07-31 6:07 ` Richard Biener
@ 2020-07-31 20:29 ` Jason Merrill
0 siblings, 0 replies; 9+ messages in thread
From: Jason Merrill @ 2020-07-31 20:29 UTC (permalink / raw)
To: Richard Biener, Patrick Palka; +Cc: GCC Patches
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
>>
>
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-07-31 20:29 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-21 19:07 [PATCH] c++: decl_constant_value and unsharing [PR96197] Patrick Palka
2020-07-22 6:18 ` Richard Biener
2020-07-22 12:57 ` Patrick Palka
2020-07-30 4:31 ` Jason Merrill
2020-07-30 13:49 ` Patrick Palka
2020-07-30 14:55 ` Jason Merrill
2020-07-30 18:54 ` Patrick Palka
2020-07-31 6:07 ` Richard Biener
2020-07-31 20:29 ` Jason Merrill
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).