diff mbox

c++/70594 debug info differences

Message ID 570E6B99.50901@acm.org
State New
Headers show

Commit Message

Nathan Sidwell April 13, 2016, 3:54 p.m. UTC
This patch builds from Jason's proto-patch in comment #26. As Jason discovered, 
that led to link problems with gt_ggc_mx (tree_node *&) when building cc1. 
Making 'hash_map<tree,fundef_copy *>' GC-able caused gengtype to put the 
GC-walkers for tree into constexpr.c. No idea why. That's also what led me to 
add the user GTY stuff in the patch I posted earlier. For some reason manual GC 
fns kept the tree GC-walker in a sane place. I wonder if the same problem is why 
Patrick's original patch wrapped the hash_map pointer in a local structure?

Rather than peer into gengtype's mind, I figured on changing to use a regular 
tree->tree mapper, which didn't disturb gengtype. I employ a TREE_LIST to hold 
the bits that fundef_copy held (body, parms, result) on (PURPOSE, VALUE, TYPE). 
A little unclean, but not the first time non-types are on such a TYPE, IIRC. 
While there I noticed that the getter only needed to use a hash getter, rather 
than get_or_insert.

The thrust of the patch makes the fundef copies and constexpr call tables 
GCable, not GC-deletable. Thus their contents are not affected by GC 
occurrences. Finally, a new hook called at end of parsing to delete the 
constexpr call & fundef copies tables, so they don't remain after parsing.  We 
don't do anything about stopping them getting too big.

Patch survives boot & test, and fixes the testcase in 70594/#4 (without Jakub's 
patch to not emit UIDs in the gimple dump).

ok?

nathan

Comments

Jason Merrill April 13, 2016, 7:41 p.m. UTC | #1
On 04/13/2016 11:54 AM, Nathan Sidwell wrote:
> This patch builds from Jason's proto-patch in comment #26. As Jason
> discovered, that led to link problems with gt_ggc_mx (tree_node *&) when
> building cc1. Making 'hash_map<tree,fundef_copy *>' GC-able caused
> gengtype to put the GC-walkers for tree into constexpr.c. No idea why.
> That's also what led me to add the user GTY stuff in the patch I posted
> earlier. For some reason manual GC fns kept the tree GC-walker in a sane
> place. I wonder if the same problem is why Patrick's original patch
> wrapped the hash_map pointer in a local structure?
>
> Rather than peer into gengtype's mind, I figured on changing to use a
> regular tree->tree mapper, which didn't disturb gengtype. I employ a
> TREE_LIST to hold the bits that fundef_copy held (body, parms, result)
> on (PURPOSE, VALUE, TYPE). A little unclean, but not the first time
> non-types are on such a TYPE, IIRC. While there I noticed that the
> getter only needed to use a hash getter, rather than get_or_insert.
>
> The thrust of the patch makes the fundef copies and constexpr call
> tables GCable, not GC-deletable. Thus their contents are not affected by
> GC occurrences. Finally, a new hook called at end of parsing to delete
> the constexpr call & fundef copies tables, so they don't remain after
> parsing.  We don't do anything about stopping them getting too big.
>
> Patch survives boot & test, and fixes the testcase in 70594/#4 (without
> Jakub's patch to not emit UIDs in the gimple dump).
>
> ok?

The fini_constexpr stuff is OK immediately.

The rest of the patch is OK if Jakub thinks it should go in, but if his 
approach addresses the problem, we might as well continue to GC the tables.

Jason
Nathan Sidwell April 14, 2016, 1:25 p.m. UTC | #2
On 04/13/16 15:41, Jason Merrill wrote:

> The fini_constexpr stuff is OK immediately.

As those two objects are currently GTY((deletable)) I don't think that's 
necessary.  Have I missed something?

> The rest of the patch is OK if Jakub thinks it should go in, but if his approach
> addresses the problem, we might as well continue to GC the tables.

Jakub?  I see you've obscured the UIDs in the dump, addressed a LABELs issue, 
and found another problem.  Do we still want DECL_UID stability?

nathan
Jason Merrill April 14, 2016, 1:43 p.m. UTC | #3
On 04/14/2016 09:25 AM, Nathan Sidwell wrote:
> On 04/13/16 15:41, Jason Merrill wrote:
>
>> The fini_constexpr stuff is OK immediately.
>
> As those two objects are currently GTY((deletable)) I don't think that's
> necessary.  Have I missed something?

True, I suppose it doesn't make much difference.

Jason
Jakub Jelinek April 14, 2016, 1:45 p.m. UTC | #4
On Thu, Apr 14, 2016 at 09:43:26AM -0400, Jason Merrill wrote:
> On 04/14/2016 09:25 AM, Nathan Sidwell wrote:
> >On 04/13/16 15:41, Jason Merrill wrote:
> >
> >>The fini_constexpr stuff is OK immediately.
> >
> >As those two objects are currently GTY((deletable)) I don't think that's
> >necessary.  Have I missed something?
> 
> True, I suppose it doesn't make much difference.

Right now our debugging shows that the remaining issue is just an unrelated
ipa-devirt bug (where the aggressive pruning of BLOCK trees with -g0 results
in different devirt decisions from non-pruned one).

So we hopefully can get away with deletable.

	Jakub
Richard Biener April 15, 2016, 9:57 a.m. UTC | #5
On Thu, Apr 14, 2016 at 3:45 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Thu, Apr 14, 2016 at 09:43:26AM -0400, Jason Merrill wrote:
>> On 04/14/2016 09:25 AM, Nathan Sidwell wrote:
>> >On 04/13/16 15:41, Jason Merrill wrote:
>> >
>> >>The fini_constexpr stuff is OK immediately.
>> >
>> >As those two objects are currently GTY((deletable)) I don't think that's
>> >necessary.  Have I missed something?
>>
>> True, I suppose it doesn't make much difference.
>
> Right now our debugging shows that the remaining issue is just an unrelated
> ipa-devirt bug (where the aggressive pruning of BLOCK trees with -g0 results
> in different devirt decisions from non-pruned one).
>
> So we hopefully can get away with deletable.

Seems not so - see PR70675.  I'd make the tables non-deletable but flush
them after parsing finished (I don't see them deleted anywhere right now).

Richard.

>         Jakub
diff mbox

Patch

2016-04-13  Jason Merrill  <jason@redhat.com>
	    Nathan Sidwell  <nathan@acm.org>

	PR c++/70594
	* constexpr.c (constexpr_call_table): Preserve in GC.
	(struct fundef_copy, struct fundef_copies_table_t):	Delete.
	(fundef_copies_table): Preserve in GC. Change to pointer to
	tree->tree hash.
	(maybe_initialize_fundef_copies_table): Adjust.
	(get_fundef_copy): Return a TREE_LIST.  Use non-inserting search.
	(save_fundef_copy): Adjust for a TREE_LIST.
	(cxx_eval_call_expression): Adjust for a fundef_copy TREE_LIST.
	(fini_constexpr): New.
	* cp-tree.h (fini_constexpr): Declare.
	* decl2.c (c_parse_final_cleanups): Call fini_constexpr.

Index: cp/constexpr.c
===================================================================
--- cp/constexpr.c	(revision 234934)
+++ cp/constexpr.c	(working copy)
@@ -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 ((deletable)) hash_table<constexpr_call_hasher> *constexpr_call_table;
+static GTY (()) hash_table<constexpr_call_hasher> *constexpr_call_table;
 
 static tree cxx_eval_constant_expression (const constexpr_ctx *, tree,
 					  bool, bool *, bool *, tree * = NULL);
@@ -965,17 +965,6 @@  maybe_initialize_constexpr_call_table (v
     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
@@ -983,45 +972,42 @@  struct fundef_copy
 
    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.  */
+   that's keyed off of the original FUNCTION_DECL and whose value is a
+   TREE_LIST of this function's unused copies awaiting reuse.
 
-struct fundef_copies_table_t
-{
-  hash_map<tree, fundef_copy *> *map;
-};
+   This is not GC-deletable to avoid GC affecting UID generation.  */
 
-static GTY((deletable)) fundef_copies_table_t fundef_copies_table;
+static GTY(()) hash_map<tree, tree> *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);
+  if (fundef_copies_table == NULL)
+    fundef_copies_table = hash_map<tree,tree>::create_ggc (101);
 }
 
 /* Reuse a copy or create a new unshared copy of the function FUN.
    Return this copy.  */
 
-static fundef_copy *
+static tree
 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;
+  tree copy;
+  tree *slot = fundef_copies_table->get (fun);
+  if (slot == NULL)
+    {
+      copy = build_tree_list (NULL, NULL);
+      /* PURPOSE is body, VALUE is parms, TYPE is result.  */
+      TREE_PURPOSE (copy) = copy_fn (fun, TREE_VALUE (copy), TREE_TYPE (copy));
     }
   else
     {
       copy = *slot;
-      *slot = (*slot)->prev;
+      *slot = TREE_CHAIN (copy);
     }
 
   return copy;
@@ -1030,10 +1016,10 @@  get_fundef_copy (tree fun)
 /* Save the copy COPY of function FUN for later reuse by get_fundef_copy().  */
 
 static void
-save_fundef_copy (tree fun, fundef_copy *copy)
+save_fundef_copy (tree fun, tree copy)
 {
-  fundef_copy **slot = &fundef_copies_table.map->get_or_insert (fun, NULL);
-  copy->prev = *slot;
+  tree *slot = &fundef_copies_table->get_or_insert (fun, NULL);
+  TREE_CHAIN (copy) = *slot;
   *slot = copy;
 }
 
@@ -1464,10 +1450,10 @@  cxx_eval_call_expression (const constexp
 	  tree body, 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;
+	  tree copy = get_fundef_copy (fun);
+	  body = TREE_PURPOSE (copy);
+	  parms = TREE_VALUE (copy);
+	  res = TREE_TYPE (copy);
 
 	  /* Associate the bindings with the remapped parms.  */
 	  tree bound = new_call.bindings;
@@ -5203,4 +5189,14 @@  require_potential_rvalue_constant_expres
   return potential_constant_expression_1 (t, true, true, tf_warning_or_error);
 }
 
+/* Finalize constexpr processing after parsing.  */
+
+void
+fini_constexpr (void)
+{
+  /* The contexpr call and fundef copies tables are no longer needed.  */
+  constexpr_call_table =  NULL;
+  fundef_copies_table = NULL;
+}
+
 #include "gt-cp-constexpr.h"
Index: cp/cp-tree.h
===================================================================
--- cp/cp-tree.h	(revision 234934)
+++ cp/cp-tree.h	(working copy)
@@ -6879,6 +6879,7 @@  bool cilkplus_an_triplet_types_ok_p
 						 tree);
 
 /* In constexpr.c */
+extern void fini_constexpr			(void);
 extern bool literal_type_p                      (tree);
 extern tree register_constexpr_fundef           (tree, tree);
 extern bool check_constexpr_ctor_body           (tree, tree, bool);
Index: cp/decl2.c
===================================================================
--- cp/decl2.c	(revision 234934)
+++ cp/decl2.c	(working copy)
@@ -4904,6 +4904,8 @@  c_parse_final_cleanups (void)
 
   finish_repo ();
 
+  fini_constexpr ();
+
   /* The entire file is now complete.  If requested, dump everything
      to a file.  */
   dump_tu ();