Patchwork [pph] More DECL merging. (issue5268042)

login
register
mail settings
Submitter Lawrence Crowl
Date Oct. 13, 2011, 3:36 a.m.
Message ID <20111013033649.472DE222694@jade.mtv.corp.google.com>
Download mbox | patch
Permalink /patch/119342/
State New
Headers show

Comments

Lawrence Crowl - Oct. 13, 2011, 3:36 a.m.
Use the mangled name for merging, as this should enable us to
handle function overloads.  We use the regular identifier for other
declarations, as that should be sufficient and avoids the problem of

--
This patch is available for review at http://codereview.appspot.com/5268042
Diego Novillo - Oct. 13, 2011, 11:39 a.m.
On Wed, Oct 12, 2011 at 23:36, Lawrence Crowl <crowl@google.com> wrote:
> Use the mangled name for merging, as this should enable us to
> handle function overloads.  We use the regular identifier for other
> declarations, as that should be sufficient and avoids the problem of
> different typedefs mangling to the same name.
>
> Merge struct members as well as namespace members.  This will
> eventually help with member declaration versus definition issues.
>
> Change test cases to reflect the above.
>
> Comment on other failing tests.
>
> Comment on failing cache handling for merge.
>
> Tested on x64.
>
>
> Index: gcc/testsuite/ChangeLog.pph
>
> 2011-10-12   Lawrence Crowl  <crowl@google.com>
>
>        * g++.dg/pph/p2pr36533.cc: Mark expected fail on unexpanded intrinsic.
>        * g++.dg/pph/p4pr36533.cc: Likewise.
>        * g++.dg/pph/p4mean.cc: Likewise.
>        * g++.dg/pph/c3variables.cc: Comment on reason for fail.
>        * g++.dg/pph/c4vardef.cc: Likewise.
>
> Index: gcc/cp/ChangeLog.pph
>
> 2011-10-12   Lawrence Crowl  <crowl@google.com>
>
>        * pph-streamer.h (pph_merge_name): New.
>        * pph-streamer.c (pph_merge_name): New.
>        * pph-streamer-out.c (pph_out_mergeable_tree_vec): Emit the vector
>        in declaration order.
>        (pph_out_merge_name): New.
>        (pph_write_any_tree): Use pph_out_merge_name instead of raw code.
>        * pph-streamer-in.c (pph_match_to_link): Use pph_merge_name.
>        (pph_in_binding_level): Also merge members of structs.
>        (pph_read_any_tree): Save read tree to determine if it is different
>        from the tree to be used.
>
>
> Index: gcc/testsuite/g++.dg/pph/p2pr36533.cc
> ===================================================================
> --- gcc/testsuite/g++.dg/pph/p2pr36533.cc       (revision 179880)
> +++ gcc/testsuite/g++.dg/pph/p2pr36533.cc       (working copy)
> @@ -1,2 +1,6 @@
>  /* { dg-options "-w -fpermissive" } */
> +// pph asm xdiff 25347
> +// xfail BOGUS INTRINSIC
> +// failing to recognise memset as an intrinsic
> +
>  #include "p1pr36533.h"
> Index: gcc/testsuite/g++.dg/pph/c3variables.cc
> ===================================================================
> --- gcc/testsuite/g++.dg/pph/c3variables.cc     (revision 179880)
> +++ gcc/testsuite/g++.dg/pph/c3variables.cc     (working copy)
> @@ -1,4 +1,5 @@
>  // pph asm xdiff 34997
> +// xfail BOGUS DUPVAR
>  // tentative definition emitted twice
>
>  #include "c0variables1.h"
> Index: gcc/testsuite/g++.dg/pph/p4mean.cc
> ===================================================================
> --- gcc/testsuite/g++.dg/pph/p4mean.cc  (revision 179880)
> +++ gcc/testsuite/g++.dg/pph/p4mean.cc  (working copy)
> @@ -1,4 +1,8 @@
>  /* { dg-options "-w -fpermissive" }  */
> +// pph asm xdiff 39234
> +// xfail BOGUS INTRINSIC
> +// failing to recognize sqrt as an intrinsic
> +
>  #include <stdlib.h>
>  #include <stdio.h>
>  #include <math.h>
> Index: gcc/testsuite/g++.dg/pph/p4pr36533.cc
> ===================================================================
> --- gcc/testsuite/g++.dg/pph/p4pr36533.cc       (revision 179880)
> +++ gcc/testsuite/g++.dg/pph/p4pr36533.cc       (working copy)
> @@ -1,2 +1,6 @@
>  /* { dg-options "-w -fpermissive" } */
> +// pph asm xdiff 25347
> +// xfail BOGUS INTRINSIC
> +// failing to recognise memset as an intrinsic
> +
>  #include "p4pr36533.h"
> Index: gcc/testsuite/g++.dg/pph/c4vardef.cc
> ===================================================================
> --- gcc/testsuite/g++.dg/pph/c4vardef.cc        (revision 179880)
> +++ gcc/testsuite/g++.dg/pph/c4vardef.cc        (working copy)
> @@ -1,4 +1,6 @@
>  // pph asm xdiff 00553
> +// xfail BOGUS DUPVAR
> +// definition emitted twice
>
>  #include "c0vardef1.h"
>  #include "c0vardef2.h"
> Index: gcc/cp/pph-streamer-in.c
> ===================================================================
> --- gcc/cp/pph-streamer-in.c    (revision 179880)
> +++ gcc/cp/pph-streamer-in.c    (working copy)
> @@ -803,7 +803,7 @@ pph_match_to_function (tree expr ATTRIBU
>    against an LINK of a chain. */
>
>  static tree
> -pph_match_to_link (tree expr, location_t where, const char *idstr, tree* link)
> +pph_match_to_link (tree expr, location_t where, const char *idstr, tree *link)
>  {
>   enum tree_code link_code, expr_code;
>   tree idtree;
> @@ -817,7 +817,7 @@ pph_match_to_link (tree expr, location_t
>   if (link_code != expr_code)
>     return NULL;
>
> -  idtree = DECL_NAME (*link);
> +  idtree = pph_merge_name (*link);
>   if (!idtree)
>     return NULL;
>
> @@ -1072,7 +1072,7 @@ pph_in_binding_level (cp_binding_level *
>   *out_field = bl;
>
>   entity = bl->this_entity = pph_in_tree (stream);
> -  if (NAMESPACE_SCOPE_P (entity))
> +  if (NAMESPACE_SCOPE_P (entity) || DECL_CLASS_SCOPE_P (entity))
>     {
>       if (flag_pph_debug >= 3)
>         debug_tree_chain (bl->names);
> @@ -1962,7 +1962,8 @@ pph_read_any_tree (pph_stream *stream, t
>  {
>   struct lto_input_block *ib = stream->encoder.r.ib;
>   struct data_in *data_in = stream->encoder.r.data_in;
> -  tree expr = NULL_TREE;
> +  tree read = NULL;
> +  tree expr = NULL;
>   enum pph_record_marker marker;
>   unsigned image_ix, ix;
>   enum LTO_tags tag;
> @@ -1998,9 +1999,11 @@ pph_read_any_tree (pph_stream *stream, t
>
>       /* Materialize a new node from IB.  This will also read all the
>          language-independent bitfields for the new tree.  */
> -      expr = pph_read_tree_header (stream, tag);
> +      expr = read = pph_read_tree_header (stream, tag);
> +      gcc_assert (read != NULL);
>       if (chain)
>         expr = pph_merge_into_chain (stream, expr, chain);
> +      gcc_assert (expr != NULL);
>     }
>
>   gcc_assert (marker == PPH_RECORD_START
> @@ -2021,6 +2024,7 @@ pph_read_any_tree (pph_stream *stream, t
>   /* Add the new tree to the cache and read its body.  The tree
>      is added to the cache before we read its body to handle
>      circular references and references from children nodes.  */
> +  /* FIXME pph: We should not insert when read == expr, but it fails.  */

No.  We should always insert this tree in the cache.  Otherwise,
subsequent IREF records will not find what they're looking for.  If
READ == EXPR, it means that we did not do the merge, right?

pph_read_tree_header will allocate an incomplete tree and assign it to
EXPR and READ, so the only way we should be getting the same tree back
from pph_merge_into_chain is if it gives us back the same tree we just
allocated.

Regardless of what we get here, the tree we materialize must be added
to the cache.

> +  tree name = pph_merge_name (expr);
> +  if (name)
> +    pph_out_string_with_length (stream, IDENTIFIER_POINTER (name),
> +                                        IDENTIFIER_LENGTH (name));

Indent here needs to align with the open braces above.
Diego Novillo - Oct. 13, 2011, 3:18 p.m.
I'm seeing an infinite loop in g++.dg/pph/c1limits-externalid.cc.  The
while() loop in pph_search_in_chain is not ending.  Or maybe it's
falling into the N^2 trap you mention in that routine?

I've added a short timeout to this test and XFAIL'd it so you can debug it.


Diego.

Patch

different typedefs mangling to the same name.

Merge struct members as well as namespace members.  This will
eventually help with member declaration versus definition issues.

Change test cases to reflect the above.

Comment on other failing tests.

Comment on failing cache handling for merge.

Tested on x64.


Index: gcc/testsuite/ChangeLog.pph

2011-10-12   Lawrence Crowl  <crowl@google.com>

	* g++.dg/pph/p2pr36533.cc: Mark expected fail on unexpanded intrinsic.
	* g++.dg/pph/p4pr36533.cc: Likewise.
	* g++.dg/pph/p4mean.cc: Likewise.
	* g++.dg/pph/c3variables.cc: Comment on reason for fail.
	* g++.dg/pph/c4vardef.cc: Likewise.

Index: gcc/cp/ChangeLog.pph

2011-10-12   Lawrence Crowl  <crowl@google.com>

	* pph-streamer.h (pph_merge_name): New.
	* pph-streamer.c (pph_merge_name): New.
	* pph-streamer-out.c (pph_out_mergeable_tree_vec): Emit the vector
	in declaration order.
	(pph_out_merge_name): New.
	(pph_write_any_tree): Use pph_out_merge_name instead of raw code.
	* pph-streamer-in.c (pph_match_to_link): Use pph_merge_name.
	(pph_in_binding_level): Also merge members of structs.
	(pph_read_any_tree): Save read tree to determine if it is different
	from the tree to be used.


Index: gcc/testsuite/g++.dg/pph/p2pr36533.cc
===================================================================
--- gcc/testsuite/g++.dg/pph/p2pr36533.cc	(revision 179880)
+++ gcc/testsuite/g++.dg/pph/p2pr36533.cc	(working copy)
@@ -1,2 +1,6 @@ 
 /* { dg-options "-w -fpermissive" } */
+// pph asm xdiff 25347
+// xfail BOGUS INTRINSIC
+// failing to recognise memset as an intrinsic
+
 #include "p1pr36533.h"
Index: gcc/testsuite/g++.dg/pph/c3variables.cc
===================================================================
--- gcc/testsuite/g++.dg/pph/c3variables.cc	(revision 179880)
+++ gcc/testsuite/g++.dg/pph/c3variables.cc	(working copy)
@@ -1,4 +1,5 @@ 
 // pph asm xdiff 34997
+// xfail BOGUS DUPVAR
 // tentative definition emitted twice
 
 #include "c0variables1.h"
Index: gcc/testsuite/g++.dg/pph/p4mean.cc
===================================================================
--- gcc/testsuite/g++.dg/pph/p4mean.cc	(revision 179880)
+++ gcc/testsuite/g++.dg/pph/p4mean.cc	(working copy)
@@ -1,4 +1,8 @@ 
 /* { dg-options "-w -fpermissive" }  */
+// pph asm xdiff 39234
+// xfail BOGUS INTRINSIC
+// failing to recognize sqrt as an intrinsic
+
 #include <stdlib.h>
 #include <stdio.h>
 #include <math.h>
Index: gcc/testsuite/g++.dg/pph/p4pr36533.cc
===================================================================
--- gcc/testsuite/g++.dg/pph/p4pr36533.cc	(revision 179880)
+++ gcc/testsuite/g++.dg/pph/p4pr36533.cc	(working copy)
@@ -1,2 +1,6 @@ 
 /* { dg-options "-w -fpermissive" } */
+// pph asm xdiff 25347
+// xfail BOGUS INTRINSIC
+// failing to recognise memset as an intrinsic
+
 #include "p4pr36533.h"
Index: gcc/testsuite/g++.dg/pph/c4vardef.cc
===================================================================
--- gcc/testsuite/g++.dg/pph/c4vardef.cc	(revision 179880)
+++ gcc/testsuite/g++.dg/pph/c4vardef.cc	(working copy)
@@ -1,4 +1,6 @@ 
 // pph asm xdiff 00553
+// xfail BOGUS DUPVAR
+// definition emitted twice
 
 #include "c0vardef1.h"
 #include "c0vardef2.h"
Index: gcc/cp/pph-streamer-in.c
===================================================================
--- gcc/cp/pph-streamer-in.c	(revision 179880)
+++ gcc/cp/pph-streamer-in.c	(working copy)
@@ -803,7 +803,7 @@  pph_match_to_function (tree expr ATTRIBU
    against an LINK of a chain. */
 
 static tree
-pph_match_to_link (tree expr, location_t where, const char *idstr, tree* link)
+pph_match_to_link (tree expr, location_t where, const char *idstr, tree *link)
 {
   enum tree_code link_code, expr_code;
   tree idtree;
@@ -817,7 +817,7 @@  pph_match_to_link (tree expr, location_t
   if (link_code != expr_code)
     return NULL;
 
-  idtree = DECL_NAME (*link);
+  idtree = pph_merge_name (*link);
   if (!idtree)
     return NULL;
 
@@ -1072,7 +1072,7 @@  pph_in_binding_level (cp_binding_level *
   *out_field = bl;
 
   entity = bl->this_entity = pph_in_tree (stream);
-  if (NAMESPACE_SCOPE_P (entity))
+  if (NAMESPACE_SCOPE_P (entity) || DECL_CLASS_SCOPE_P (entity))
     {
       if (flag_pph_debug >= 3)
         debug_tree_chain (bl->names);
@@ -1962,7 +1962,8 @@  pph_read_any_tree (pph_stream *stream, t
 {
   struct lto_input_block *ib = stream->encoder.r.ib;
   struct data_in *data_in = stream->encoder.r.data_in;
-  tree expr = NULL_TREE;
+  tree read = NULL;
+  tree expr = NULL;
   enum pph_record_marker marker;
   unsigned image_ix, ix;
   enum LTO_tags tag;
@@ -1998,9 +1999,11 @@  pph_read_any_tree (pph_stream *stream, t
 
       /* Materialize a new node from IB.  This will also read all the
          language-independent bitfields for the new tree.  */
-      expr = pph_read_tree_header (stream, tag);
+      expr = read = pph_read_tree_header (stream, tag);
+      gcc_assert (read != NULL);
       if (chain)
         expr = pph_merge_into_chain (stream, expr, chain);
+      gcc_assert (expr != NULL);
     }
 
   gcc_assert (marker == PPH_RECORD_START
@@ -2021,6 +2024,7 @@  pph_read_any_tree (pph_stream *stream, t
   /* Add the new tree to the cache and read its body.  The tree
      is added to the cache before we read its body to handle
      circular references and references from children nodes.  */
+  /* FIXME pph: We should not insert when read == expr, but it fails.  */
   pph_cache_insert_at (&stream->cache, expr, ix, pph_tree_code_to_tag (expr));
   pph_read_tree_body (stream, expr);
 
Index: gcc/cp/pph-streamer.c
===================================================================
--- gcc/cp/pph-streamer.c	(revision 179880)
+++ gcc/cp/pph-streamer.c	(working copy)
@@ -727,3 +727,15 @@  pph_get_signature (tree t, size_t *nbyte
 
   return crc;
 }
+
+
+/* Return the merge name string identifier tree for a decl EXPR.  */
+
+tree
+pph_merge_name (tree expr)
+{
+  if (TREE_CODE (expr) == FUNCTION_DECL)
+    return DECL_ASSEMBLER_NAME (expr);
+  else
+    return DECL_NAME (expr);
+}
Index: gcc/cp/pph-streamer.h
===================================================================
--- gcc/cp/pph-streamer.h	(revision 179880)
+++ gcc/cp/pph-streamer.h	(working copy)
@@ -246,6 +246,7 @@  bool pph_cache_add (pph_cache *, void *,
 void pph_cache_sign (pph_cache *, unsigned, unsigned, size_t);
 unsigned pph_get_signature (tree, size_t *);
 void pph_writer_add_include (pph_stream *);
+tree pph_merge_name (tree expr);
 
 /* In pph-streamer-out.c.  */
 void pph_flush_buffers (pph_stream *);
Index: gcc/cp/pph-streamer-out.c
===================================================================
--- gcc/cp/pph-streamer-out.c	(revision 179880)
+++ gcc/cp/pph-streamer-out.c	(working copy)
@@ -819,7 +819,7 @@  pph_out_mergeable_tree_vec (pph_stream *
      using streamer_read_chain, we have to write VECs in exactly the
      same way as tree chains.  */
   pph_out_hwi (stream, VEC_length (tree, v));
-  FOR_EACH_VEC_ELT (tree, v, i, t)
+  FOR_EACH_VEC_ELT_REVERSE (tree, v, i, t)
     pph_out_mergeable_tree (stream, t);
 }
 
@@ -1906,9 +1906,23 @@  pph_write_tree_header (pph_stream *strea
 }
 
 
+/* Write the merge name string for a decl EXPR.  */
+
+static void
+pph_out_merge_name (pph_stream *stream, tree expr)
+{
+  tree name = pph_merge_name (expr);
+  if (name)
+    pph_out_string_with_length (stream, IDENTIFIER_POINTER (name),
+                                        IDENTIFIER_LENGTH (name));
+  else
+    pph_out_string (stream, NULL);
+}
+
+
 /* Write a tree EXPR (MERGEABLE or not) to STREAM.  */
 
-void
+static void
 pph_write_any_tree (pph_stream *stream, tree expr, bool mergeable)
 {
   enum pph_record_marker marker;
@@ -1952,16 +1966,9 @@  pph_write_any_tree (pph_stream *stream, 
           pph_write_tree_header (stream, expr);
           if (mergeable && DECL_P (expr))
             {
-              tree name;
-
               /* We may need to unify two declarations.  */
               pph_out_location (stream, DECL_SOURCE_LOCATION (expr));
-              name = DECL_NAME (expr);
-              if (name)
-                pph_out_string_with_length (stream, IDENTIFIER_POINTER (name),
-                                            IDENTIFIER_LENGTH (name));
-              else
-                pph_out_string (stream, NULL);
+              pph_out_merge_name (stream, expr);
             }
         }