On Mon, 4 Apr 2016, Jason Merrill wrote:
> On 04/02/2016 05:18 PM, Patrick Palka wrote:
> > Here's a version that uses a separate deletable table to cache the
> > function copies. For simplicity I used a hash_map instead of a
> > hash_table. Does this look OK to commit after bootstrap + regtest?
>
> Thanks. Minor nits:
>
> > +struct fundef_copies_table_t
> > +{
> > + hash_map<tree, fun_copy *> *map;
> > +};
>
> Why wrap the pointer in a struct?
If I don't wrap the pointer then I get really weird link errors. If the
following line is added to constexpr.c:
static GTY((deletable)) hash_map<tree, fundef_copy *> *blah;
then the final link fails with dozens of these undefined reference errors:
/home/patrick/code/gcc/gcc/hash-map.h:68: undefined reference to
`gt_pch_nx(tree_node*&)'
/home/patrick/code/gcc/gcc/hash-map.h:61: undefined reference to
`gt_ggc_mx(tree_node*&)'
/home/patrick/code/gcc/gcc/hash-map.h:62: undefined reference to
`gt_ggc_mx(tree_node*&)'
...
Seems to only happen when the value type of the hash_map is something other
than "tree". This strangeness can be conveniently avoided by wrapping the
hash_map.
>
> > + maybe_initialize_fundef_copies_table ();
> > + fun_copy *copy = get_fun_copy (fun);
>
> Let's move the initialization call inside get_fun_copy.
Done.
>
> > On a related note, I noticed that the constexpr_call_table is not marked
> > deletable. Marking it deletable speeds up the test case in the PR by
> > about 10% and saves about 10MB. Do you think doing so is a good idea?
>
> Please.
Done.
>
> > On another related note, I noticed that marking something as both
> > GTY((deletable, cache)) doesn't work as intended, because the
> > gt_cleare_cache functions run _after_ all deletable roots get
> > zeroed out. So during GC the gt_cleare_cache function of a root
> > marked "deletable, cache" would always be a no-op. Concretely I think
> > this means that our cv_cache and fold_cache leak memory during GC
> > because their underlying hash_map (allocated by operator new) is zeroed
> > before gc_cleare_cache could clear it.
>
> Hmm, I thought I remembered hitting the breakpoint in gt_cleare_cache and it
> being non-null. But I guess we can get rid of the cache_map class and use the
> approach you have here, of a deletable gc-allocated hash_map pointer; I'd
> still use ->empty() for dumping the cache outside of GC, though.
Could do this too in a subsequent patch.
Here's an updated patch that that adjusts the initialization call and marks
constexpr_call_table as deletable. (Also fun_copy is renamed to fundef_copy
for consistency.)
-- >8 --
gcc/cp/ChangeLog:
PR c++/70452
* constexpr.c (struct fundef_copy): New struct.
(struct fundef_copies_table_t): New struct.
(fundef_copies_table): New static variable.
(maybe_initialize_fundef_copies_table): New static function.
(get_fundef_copy): New static function.
(save_fundef_copy): New static function.
(cxx_eval_call_expression): Use get_fundef_copy, and
save_fundef_copy.
(constexpr_call_table): Add "deletable" GTY marker.
gcc/testsuite/ChangeLog:
PR c++/70452
* g++.dg/ext/constexpr-vla4.C: New test.
---
gcc/cp/constexpr.c | 99 +++++++++++++++++++++++++++++--
gcc/testsuite/g++.dg/ext/constexpr-vla4.C | 17 ++++++
2 files changed, 111 insertions(+), 5 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/ext/constexpr-vla4.C
diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c
index b94b346..bcbf9bd 100644
--- a/gcc/cp/constexpr.c
+++ b/gcc/cp/constexpr.c
@@ -915,7 +915,7 @@ struct constexpr_ctx {
/* A table of all constexpr calls that have been evaluated by the
compiler in this translation unit. */
-static GTY (()) hash_table<constexpr_call_hasher> *constexpr_call_table;
+static GTY ((deletable)) hash_table<constexpr_call_hasher>
*constexpr_call_table;
static tree cxx_eval_constant_expression (const constexpr_ctx *, tree,
bool, bool *, bool *, tree * = NULL);
@@ -965,6 +965,78 @@ maybe_initialize_constexpr_call_table (void)
constexpr_call_table = hash_table<constexpr_call_hasher>::create_ggc (101);
}
+/* The representation of a single node in the per-function freelist maintained
+ by FUNDEF_COPIES_TABLE. */
+
+struct fundef_copy
+{
+ tree body;
+ tree parms;
+ tree res;
+ fundef_copy *prev;
+};
+
+/* During constexpr CALL_EXPR evaluation, to avoid issues with sharing when
+ a function happens to get called recursively, we unshare the callee
+ function's body and evaluate this unshared copy instead of evaluating the
+ original body.
+
+ FUNDEF_COPIES_TABLE is a per-function freelist of these unshared function
+ copies. The underlying data structure of FUNDEF_COPIES_TABLE is a hash_map
+ that's keyed off of the original FUNCTION_DECL and whose value is the chain
+ of this function's unused copies awaiting reuse. */
+
+struct fundef_copies_table_t
+{
+ hash_map<tree, fundef_copy *> *map;
+};
+
+static GTY((deletable)) fundef_copies_table_t fundef_copies_table;
+
+/* Initialize FUNDEF_COPIES_TABLE if it's not initialized. */
+
+static void
+maybe_initialize_fundef_copies_table ()
+{
+ if (fundef_copies_table.map == NULL)
+ fundef_copies_table.map = hash_map<tree, fundef_copy *>::create_ggc (101);
+}
+
+/* Reuse a copy or create a new unshared copy of the function FUN.
+ Return this copy. */
+
+static fundef_copy *
+get_fundef_copy (tree fun)
+{
+ maybe_initialize_fundef_copies_table ();
+
+ fundef_copy *copy;
+ fundef_copy **slot = &fundef_copies_table.map->get_or_insert (fun, NULL);
+ if (*slot == NULL)
+ {
+ copy = ggc_alloc<fundef_copy> ();
+ copy->body = copy_fn (fun, copy->parms, copy->res);
+ copy->prev = NULL;
+ }
+ else
+ {
+ copy = *slot;
+ *slot = (*slot)->prev;
+ }
+
+ return copy;
+}
+
+/* Save the copy COPY of function FUN for later reuse by get_fundef_copy(). */
+
+static void
+save_fundef_copy (tree fun, fundef_copy *copy)
+{
+ fundef_copy **slot = &fundef_copies_table.map->get_or_insert (fun, NULL);
+ copy->prev = *slot;
+ *slot = copy;
+}
+
/* We have an expression tree T that represents a call, either CALL_EXPR
or AGGR_INIT_EXPR. If the call is lexically to a named function,
retrun the _DECL for that function. */
@@ -1365,10 +1437,13 @@ cxx_eval_call_expression (const constexpr_ctx *ctx,
tree t,
if (!result || result == error_mark_node)
{
gcc_assert (DECL_SAVED_TREE (fun));
- tree parms, res;
+ tree body, parms, res;
- /* Unshare the whole function body. */
- tree body = copy_fn (fun, parms, res);
+ /* Reuse or create a new unshared copy of this function's body. */
+ fundef_copy *copy = get_fundef_copy (fun);
+ body = copy->body;
+ parms = copy->parms;
+ res = copy->res;
/* Associate the bindings with the remapped parms. */
tree bound = new_call.bindings;
@@ -1397,8 +1472,14 @@ cxx_eval_call_expression (const constexpr_ctx *ctx, tree
t,
else
ctx->values->put (res, NULL_TREE);
+ /* Track the callee's evaluated SAVE_EXPRs so that we can forget
+ their values after the call. */
+ constexpr_ctx ctx_with_save_exprs = *ctx;
+ hash_set<tree> save_exprs;
+ ctx_with_save_exprs.save_exprs = &save_exprs;
+
tree jump_target = NULL_TREE;
- cxx_eval_constant_expression (ctx, body,
+ cxx_eval_constant_expression (&ctx_with_save_exprs, body,
lval, non_constant_p, overflow_p,
&jump_target);
@@ -1423,6 +1504,11 @@ cxx_eval_call_expression (const constexpr_ctx *ctx, tree
t,
}
}
+ /* Forget the saved values of the callee's SAVE_EXPRs. */
+ for (hash_set<tree>::iterator iter = save_exprs.begin();
+ iter != save_exprs.end(); ++iter)
+ ctx_with_save_exprs.values->remove (*iter);
+
/* Remove the parms/result from the values map. Is it worth
bothering to do this when the map itself is only live for
one constexpr evaluation? If so, maybe also clear out
@@ -1432,6 +1518,9 @@ cxx_eval_call_expression (const constexpr_ctx *ctx, tree
t,
ctx->values->remove (slot);
for (tree parm = parms; parm; parm = TREE_CHAIN (parm))
ctx->values->remove (parm);
+
+ /* Make the unshared function copy we used available for re-use. */
+ save_fundef_copy (fun, copy);
}
if (result == error_mark_node)
diff --git a/gcc/testsuite/g++.dg/ext/constexpr-vla4.C
b/gcc/testsuite/g++.dg/ext/constexpr-vla4.C
new file mode 100644
index 0000000..428a8fd
--- /dev/null
+++ b/gcc/testsuite/g++.dg/ext/constexpr-vla4.C
@@ -0,0 +1,17 @@
+// PR c++/70452
+// { dg-do compile { target c++14 } }
+
+constexpr int
+foo (int n, bool p)
+{
+ __extension__ int a [n] = { 0 };
+ if (n == 3)
+ foo (n - 2, false);
+ if (n == 3)
+ foo(n - 1, true);
+ if (p)
+ return a[1];
+ return 0;
+}
+
+constexpr int i2 = foo (3, false); // { dg-bogus "array subscript out of
bound" }
--
2.8.0.rc3.27.gade0865
