[pph] Add streamer hook for preloading common nodes

[Diego Novillo, 2011-05-04 20:27:02]

This patch adds a new streamer hook to populate the streamer cache
with common nodes.

The nodes we populate for GIMPLE in lto_get_common_nodes is not
sufficient for C++, unsurprisingly.

The patch fixes these regressions in pph.exp:

FAIL: g++.dg/pph/p1stdlib.cc  -fpph-map=pph.map -I. (test for excess errors)
FAIL: g++.dg/pph/p1stdlib.cc , PPH assembly missing

There is a second part to this patch to handle INTEGER_CSTs as regular
trees (so they can be cached).  This would allow us to handle special
constants in the C++ FE like void_zero_node, but that's giving me some
trouble with LTO tests.

Tested on x86_64.  Committed to the branch.


Diego.

ChangeLog.pph

	* Makefile.in (cgraphunit.o): Add dependency on LTO_STREAMER_H.
	* cgraphunit.c: Include lto-streamer.h
	(cgraph_finalize_compilation_unit): Call gimple_streamer_hooks_init
	if LTO is enabled.
	* lto-streamer-out.c (lto_output): Move call to
	gimple_streamer_hooks_init to cgraph_finalize_compilation_unit.
	* lto-streamer.c (lto_get_common_nodes): Remove if0 hack.
	(lto_streamer_cache_create): Call streamer_hooks.get_common_nodes
	instead of lto_get_common_nodes.
	(gimple_streamer_hooks_init): Set h->get_common_nodes to
	lto_get_common_nodes.
	* lto-streamer.h (struct lto_streamer_hooks): Add field
	get_common_nodes.

cp/ChangeLog.pph

	* pph-streamer.c (pph_get_common_nodes): New.
	(pph_stream_hooks_init): Set it in h->get_common_nodes.

diff --git a/gcc/Makefile.in b/gcc/Makefile.in
index 0af93ba..f96e059 100644
--- a/gcc/Makefile.in
+++ b/gcc/Makefile.in
@@ -3001,7 +3001,7 @@ cgraphunit.o : cgraphunit.c $(CONFIG_H) $(SYSTEM_H)
coretypes.h $(TM_H) \
    $(TREE_FLOW_H) $(TREE_PASS_H) debug.h $(DIAGNOSTIC_H) \
    $(FIBHEAP_H) output.h $(PARAMS_H) $(RTL_H) $(TIMEVAR_H) $(IPA_PROP_H) \
    gt-cgraphunit.h tree-iterator.h $(COVERAGE_H) $(TREE_DUMP_H) \
-   tree-pretty-print.h gimple-pretty-print.h
+   tree-pretty-print.h gimple-pretty-print.h $(LTO_STREAMER_H)
 cgraphbuild.o : cgraphbuild.c $(CONFIG_H) $(SYSTEM_H) coretypes.h $(TM_H) \
    $(TREE_H) langhooks.h $(CGRAPH_H) intl.h pointer-set.h $(GIMPLE_H) \
    $(TREE_FLOW_H) $(TREE_PASS_H) $(IPA_UTILS_H) $(EXCEPT_H) \
diff --git a/gcc/cgraphunit.c b/gcc/cgraphunit.c
index 3dbfc2b..0d1ec89 100644
--- a/gcc/cgraphunit.c
+++ b/gcc/cgraphunit.c
@@ -138,6 +138,7 @@ along with GCC; see the file COPYING3.  If not see
 #include "output.h"
 #include "coverage.h"
 #include "plugin.h"
+#include "lto-streamer.h"
 
 static void cgraph_expand_all_functions (void);
 static void cgraph_mark_functions_to_output (void);
@@ -1062,6 +1063,10 @@ cgraph_finalize_compilation_unit (void)
 {
   timevar_push (TV_CGRAPH);
 
+  /* If LTO is enabled, initialize the streamer hooks needed by GIMPLE.  */
+  if (flag_lto)
+    gimple_streamer_hooks_init ();
+
   /* If we're here there's no current function anymore.  Some frontends
      are lazy in clearing these.  */
   current_function_decl = NULL;
diff --git a/gcc/cp/pph-streamer.c b/gcc/cp/pph-streamer.c
index 5f2112e..c363c06 100644
--- a/gcc/cp/pph-streamer.c
+++ b/gcc/cp/pph-streamer.c
@@ -55,6 +55,36 @@ pph_indexable_with_decls_p (tree t)
 }
 
 
+/* Generate a vector of common nodes that should always be streamed as
+   indexes into the streamer cache.  These nodes are always built by
+   the front end, so there is no need to emit them.  */
+
+static VEC(tree,heap) *
+pph_get_common_nodes (void)
+{
+  unsigned i;
+  VEC(tree,heap) *common_nodes = NULL;
+
+  for (i = itk_char; i < itk_none; i++)
+    VEC_safe_push (tree, heap, common_nodes, integer_types[i]);
+
+  for (i = 0; i < TYPE_KIND_LAST; i++)
+    VEC_safe_push (tree, heap, common_nodes, sizetype_tab[i]);
+
+  /* global_trees[] can have NULL entries in it.  Skip them.  */
+  for (i = 0; i < TI_MAX; i++)
+    if (global_trees[i])
+      VEC_safe_push (tree, heap, common_nodes, global_trees[i]);
+
+  /* c_global_trees[] can have NULL entries in it.  Skip them.  */
+  for (i = 0; i < CTI_MAX; i++)
+    if (c_global_trees[i])
+      VEC_safe_push (tree, heap, common_nodes, c_global_trees[i]);
+
+  return common_nodes;
+}
+
+
 /* Initialize all the streamer hooks used for streaming ASTs.  */
 
 static void
@@ -62,6 +92,9 @@ pph_stream_hooks_init (void)
 {
   lto_streamer_hooks *h = streamer_hooks_init ();
   h->name = "C++ AST";
+  h->get_common_nodes = pph_get_common_nodes;
   h->is_streamable = pph_is_streamable;
   h->write_tree = pph_stream_write_tree;
   h->read_tree = pph_stream_read_tree;
diff --git a/gcc/lto-streamer-out.c b/gcc/lto-streamer-out.c
index c26de5d..b578419 100644
--- a/gcc/lto-streamer-out.c
+++ b/gcc/lto-streamer-out.c
@@ -2198,7 +2198,6 @@ lto_output (cgraph_node_set set, varpool_node_set vset)
   int i, n_nodes;
   lto_cgraph_encoder_t encoder = lto_get_out_decl_state
()->cgraph_node_encoder;
 
-  gimple_streamer_hooks_init ();
   lto_writer_init ();
 
   n_nodes = lto_cgraph_encoder_size (encoder);
diff --git a/gcc/lto-streamer.c b/gcc/lto-streamer.c
index 04a9f7a..d6b78c8 100644
--- a/gcc/lto-streamer.c
+++ b/gcc/lto-streamer.c
@@ -550,10 +550,6 @@ lto_get_common_nodes (void)
   else
     main_identifier_node = get_identifier ("main");
 
-  /* FIXME pph.  These assertions are never met while in the front end.
-     There should be a way of checking this only when we are in LTO
-     mode.  */
-#if 0
   gcc_assert (ptrdiff_type_node == integer_type_node);
 
   /* FIXME lto.  In the C++ front-end, fileptr_type_node is defined as a
@@ -564,7 +560,6 @@ lto_get_common_nodes (void)
      These should be assured in pass_ipa_free_lang_data.  */
   gcc_assert (fileptr_type_node == ptr_type_node);
   gcc_assert (TYPE_MAIN_VARIANT (fileptr_type_node) == ptr_type_node);
-#endif
 
   seen_nodes = pointer_set_create ();
 
@@ -631,7 +626,7 @@ lto_streamer_cache_create (void)
   /* Load all the well-known tree nodes that are always created by
      the compiler on startup.  This prevents writing them out
      unnecessarily.  */
-  common_nodes = lto_get_common_nodes ();
+  common_nodes = streamer_hooks ()->get_common_nodes ();
 
   FOR_EACH_VEC_ELT (tree, common_nodes, i, node)
     preload_common_node (cache, node);
@@ -820,6 +815,7 @@ gimple_streamer_hooks_init (void)
   h->name = "gimple";
   h->reader_init = gimple_streamer_reader_init;
   h->writer_init = NULL;
+  h->get_common_nodes = lto_get_common_nodes;
   h->is_streamable = lto_is_streamable;
   h->write_tree = gimple_streamer_write_tree;
   h->read_tree = gimple_streamer_read_tree;
diff --git a/gcc/lto-streamer.h b/gcc/lto-streamer.h
index b6f4b79..8be17da 100644
--- a/gcc/lto-streamer.h
+++ b/gcc/lto-streamer.h
@@ -55,6 +55,16 @@ typedef struct lto_streamer_hooks {
   /* A string identifying this streamer.  */
   const char *name;
 
+  /* Called by lto_streamer_cache_create to instantiate a cache of
+     well-known nodes.  These are tree nodes that are always
+     instantiated by the compiler on startup.  Additionally, these
+     nodes need to be shared.  This function produces a vector of
+     these well known trees that are then added to the streamer cache.
+     This way, the writer will only write out a reference to the tree
+     and the reader will instantiate the tree out of this
+     pre-populated cache.  */
+  VEC(tree,heap) *(*get_common_nodes) (void);
+
   /* Called by lto_reader_init after it does basic initialization.  */
   void (*reader_init) (void);
 

--
This patch is available for review at http://codereview.appspot.com/4478043

[richard.guenther_gmail.com, 2011-05-05 09:06:53]

On Wed, May 4, 2011 at 10:26 PM, Diego Novillo <dnovillo@google.com> wrote:
>
> This patch adds a new streamer hook to populate the streamer cache
> with common nodes.
>
> The nodes we populate for GIMPLE in lto_get_common_nodes is not
> sufficient for C++, unsurprisingly.
>
> The patch fixes these regressions in pph.exp:
>
> FAIL: g++.dg/pph/p1stdlib.cc  -fpph-map=pph.map -I. (test for excess errors)
> FAIL: g++.dg/pph/p1stdlib.cc , PPH assembly missing
>
> There is a second part to this patch to handle INTEGER_CSTs as regular
> trees (so they can be cached).  This would allow us to handle special
> constants in the C++ FE like void_zero_node, but that's giving me some
> trouble with LTO tests.
>
> Tested on x86_64.  Committed to the branch.

I think we should move away from pre-loading the streamer cache, that
has caused enough trouble when the common nodes are originating from
different Frontends and when compiling units with different flags which
happen to change those nodes (think of the hoops we jump through
to support that for -f[un]signed-char).

Richard.

>
> Diego.
>
> ChangeLog.pph
>
>        * Makefile.in (cgraphunit.o): Add dependency on LTO_STREAMER_H.
>        * cgraphunit.c: Include lto-streamer.h
>        (cgraph_finalize_compilation_unit): Call gimple_streamer_hooks_init
>        if LTO is enabled.
>        * lto-streamer-out.c (lto_output): Move call to
>        gimple_streamer_hooks_init to cgraph_finalize_compilation_unit.
>        * lto-streamer.c (lto_get_common_nodes): Remove if0 hack.
>        (lto_streamer_cache_create): Call streamer_hooks.get_common_nodes
>        instead of lto_get_common_nodes.
>        (gimple_streamer_hooks_init): Set h->get_common_nodes to
>        lto_get_common_nodes.
>        * lto-streamer.h (struct lto_streamer_hooks): Add field
>        get_common_nodes.
>
> cp/ChangeLog.pph
>
>        * pph-streamer.c (pph_get_common_nodes): New.
>        (pph_stream_hooks_init): Set it in h->get_common_nodes.
>
> diff --git a/gcc/Makefile.in b/gcc/Makefile.in
> index 0af93ba..f96e059 100644
> --- a/gcc/Makefile.in
> +++ b/gcc/Makefile.in
> @@ -3001,7 +3001,7 @@ cgraphunit.o : cgraphunit.c $(CONFIG_H) $(SYSTEM_H)
coretypes.h $(TM_H) \
>    $(TREE_FLOW_H) $(TREE_PASS_H) debug.h $(DIAGNOSTIC_H) \
>    $(FIBHEAP_H) output.h $(PARAMS_H) $(RTL_H) $(TIMEVAR_H) $(IPA_PROP_H) \
>    gt-cgraphunit.h tree-iterator.h $(COVERAGE_H) $(TREE_DUMP_H) \
> -   tree-pretty-print.h gimple-pretty-print.h
> +   tree-pretty-print.h gimple-pretty-print.h $(LTO_STREAMER_H)
>  cgraphbuild.o : cgraphbuild.c $(CONFIG_H) $(SYSTEM_H) coretypes.h $(TM_H) \
>    $(TREE_H) langhooks.h $(CGRAPH_H) intl.h pointer-set.h $(GIMPLE_H) \
>    $(TREE_FLOW_H) $(TREE_PASS_H) $(IPA_UTILS_H) $(EXCEPT_H) \
> diff --git a/gcc/cgraphunit.c b/gcc/cgraphunit.c
> index 3dbfc2b..0d1ec89 100644
> --- a/gcc/cgraphunit.c
> +++ b/gcc/cgraphunit.c
> @@ -138,6 +138,7 @@ along with GCC; see the file COPYING3.  If not see
>  #include "output.h"
>  #include "coverage.h"
>  #include "plugin.h"
> +#include "lto-streamer.h"
>
>  static void cgraph_expand_all_functions (void);
>  static void cgraph_mark_functions_to_output (void);
> @@ -1062,6 +1063,10 @@ cgraph_finalize_compilation_unit (void)
>  {
>   timevar_push (TV_CGRAPH);
>
> +  /* If LTO is enabled, initialize the streamer hooks needed by GIMPLE.  */
> +  if (flag_lto)
> +    gimple_streamer_hooks_init ();
> +
>   /* If we're here there's no current function anymore.  Some frontends
>      are lazy in clearing these.  */
>   current_function_decl = NULL;
> diff --git a/gcc/cp/pph-streamer.c b/gcc/cp/pph-streamer.c
> index 5f2112e..c363c06 100644
> --- a/gcc/cp/pph-streamer.c
> +++ b/gcc/cp/pph-streamer.c
> @@ -55,6 +55,36 @@ pph_indexable_with_decls_p (tree t)
>  }
>
>
> +/* Generate a vector of common nodes that should always be streamed as
> +   indexes into the streamer cache.  These nodes are always built by
> +   the front end, so there is no need to emit them.  */
> +
> +static VEC(tree,heap) *
> +pph_get_common_nodes (void)
> +{
> +  unsigned i;
> +  VEC(tree,heap) *common_nodes = NULL;
> +
> +  for (i = itk_char; i < itk_none; i++)
> +    VEC_safe_push (tree, heap, common_nodes, integer_types[i]);
> +
> +  for (i = 0; i < TYPE_KIND_LAST; i++)
> +    VEC_safe_push (tree, heap, common_nodes, sizetype_tab[i]);
> +
> +  /* global_trees[] can have NULL entries in it.  Skip them.  */
> +  for (i = 0; i < TI_MAX; i++)
> +    if (global_trees[i])
> +      VEC_safe_push (tree, heap, common_nodes, global_trees[i]);
> +
> +  /* c_global_trees[] can have NULL entries in it.  Skip them.  */
> +  for (i = 0; i < CTI_MAX; i++)
> +    if (c_global_trees[i])
> +      VEC_safe_push (tree, heap, common_nodes, c_global_trees[i]);
> +
> +  return common_nodes;
> +}
> +
> +
>  /* Initialize all the streamer hooks used for streaming ASTs.  */
>
>  static void
> @@ -62,6 +92,9 @@ pph_stream_hooks_init (void)
>  {
>   lto_streamer_hooks *h = streamer_hooks_init ();
>   h->name = "C++ AST";
> +  h->get_common_nodes = pph_get_common_nodes;
>   h->is_streamable = pph_is_streamable;
>   h->write_tree = pph_stream_write_tree;
>   h->read_tree = pph_stream_read_tree;
> diff --git a/gcc/lto-streamer-out.c b/gcc/lto-streamer-out.c
> index c26de5d..b578419 100644
> --- a/gcc/lto-streamer-out.c
> +++ b/gcc/lto-streamer-out.c
> @@ -2198,7 +2198,6 @@ lto_output (cgraph_node_set set, varpool_node_set vset)
>   int i, n_nodes;
>   lto_cgraph_encoder_t encoder = lto_get_out_decl_state
()->cgraph_node_encoder;
>
> -  gimple_streamer_hooks_init ();
>   lto_writer_init ();
>
>   n_nodes = lto_cgraph_encoder_size (encoder);
> diff --git a/gcc/lto-streamer.c b/gcc/lto-streamer.c
> index 04a9f7a..d6b78c8 100644
> --- a/gcc/lto-streamer.c
> +++ b/gcc/lto-streamer.c
> @@ -550,10 +550,6 @@ lto_get_common_nodes (void)
>   else
>     main_identifier_node = get_identifier ("main");
>
> -  /* FIXME pph.  These assertions are never met while in the front end.
> -     There should be a way of checking this only when we are in LTO
> -     mode.  */
> -#if 0
>   gcc_assert (ptrdiff_type_node == integer_type_node);
>
>   /* FIXME lto.  In the C++ front-end, fileptr_type_node is defined as a
> @@ -564,7 +560,6 @@ lto_get_common_nodes (void)
>      These should be assured in pass_ipa_free_lang_data.  */
>   gcc_assert (fileptr_type_node == ptr_type_node);
>   gcc_assert (TYPE_MAIN_VARIANT (fileptr_type_node) == ptr_type_node);
> -#endif
>
>   seen_nodes = pointer_set_create ();
>
> @@ -631,7 +626,7 @@ lto_streamer_cache_create (void)
>   /* Load all the well-known tree nodes that are always created by
>      the compiler on startup.  This prevents writing them out
>      unnecessarily.  */
> -  common_nodes = lto_get_common_nodes ();
> +  common_nodes = streamer_hooks ()->get_common_nodes ();
>
>   FOR_EACH_VEC_ELT (tree, common_nodes, i, node)
>     preload_common_node (cache, node);
> @@ -820,6 +815,7 @@ gimple_streamer_hooks_init (void)
>   h->name = "gimple";
>   h->reader_init = gimple_streamer_reader_init;
>   h->writer_init = NULL;
> +  h->get_common_nodes = lto_get_common_nodes;
>   h->is_streamable = lto_is_streamable;
>   h->write_tree = gimple_streamer_write_tree;
>   h->read_tree = gimple_streamer_read_tree;
> diff --git a/gcc/lto-streamer.h b/gcc/lto-streamer.h
> index b6f4b79..8be17da 100644
> --- a/gcc/lto-streamer.h
> +++ b/gcc/lto-streamer.h
> @@ -55,6 +55,16 @@ typedef struct lto_streamer_hooks {
>   /* A string identifying this streamer.  */
>   const char *name;
>
> +  /* Called by lto_streamer_cache_create to instantiate a cache of
> +     well-known nodes.  These are tree nodes that are always
> +     instantiated by the compiler on startup.  Additionally, these
> +     nodes need to be shared.  This function produces a vector of
> +     these well known trees that are then added to the streamer cache.
> +     This way, the writer will only write out a reference to the tree
> +     and the reader will instantiate the tree out of this
> +     pre-populated cache.  */
> +  VEC(tree,heap) *(*get_common_nodes) (void);
> +
>   /* Called by lto_reader_init after it does basic initialization.  */
>   void (*reader_init) (void);
>
>
> --
> This patch is available for review at http://codereview.appspot.com/4478043
>

[Diego Novillo, 2011-05-05 11:03:15]

On Thu, May 5, 2011 at 05:06, Richard Guenther
<richard.guenther@gmail.com> wrote:

> I think we should move away from pre-loading the streamer cache, that
> has caused enough trouble when the common nodes are originating from
> different Frontends and when compiling units with different flags which
> happen to change those nodes (think of the hoops we jump through
> to support that for -f[un]signed-char).

Sure, though that's not an issue for pph.  PPH images are generated
and consumed by one front end.

Pre-loading common nodes in C++ gives us:

1- Smaller pph images
2- Ability to do pointer comparison against common nodes.

#1 saves us from saving a few hundred nodes in each pph image.  But #2
is a stronger requirement.  How do you think we could do #2 without
pre-loading?


Diego.

[richard.guenther_gmail.com, 2011-05-05 11:07:49]

On Thu, May 5, 2011 at 1:03 PM, Diego Novillo <dnovillo@google.com> wrote:
> On Thu, May 5, 2011 at 05:06, Richard Guenther
> <richard.guenther@gmail.com> wrote:
>
>> I think we should move away from pre-loading the streamer cache, that
>> has caused enough trouble when the common nodes are originating from
>> different Frontends and when compiling units with different flags which
>> happen to change those nodes (think of the hoops we jump through
>> to support that for -f[un]signed-char).
>
> Sure, though that's not an issue for pph.  PPH images are generated
> and consumed by one front end.
>
> Pre-loading common nodes in C++ gives us:
>
> 1- Smaller pph images
> 2- Ability to do pointer comparison against common nodes.
>
> #1 saves us from saving a few hundred nodes in each pph image.  But #2
> is a stronger requirement.  How do you think we could do #2 without
> pre-loading?

For LTO we have type-merging for that, and we'd continue to pre-load
the type merger with the (LTO frontend specific) common tree nodes.
I suppose you are not doing any merging at all?  If so pre-loading those
nodes makes indeed sense (given you have a way to reject PPH images
when flags such as -f[un]signed-char differ ...).

Richard.

>
> Diego.
>

[Diego Novillo, 2011-05-05 11:25:22]

On Thu, May 5, 2011 at 07:07, Richard Guenther
<richard.guenther@gmail.com> wrote:

> For LTO we have type-merging for that, and we'd continue to pre-load
> the type merger with the (LTO frontend specific) common tree nodes.

OK.  For LTO it may make sense to eventually make this hook a nop, then.

> I suppose you are not doing any merging at all?  If so pre-loading those
> nodes makes indeed sense (given you have a way to reject PPH images
> when flags such as -f[un]signed-char differ ...).

There will be some amount of merging, but I'm anticipating using the
same merging scheme used by the parser.  As far as the parser is
concerned, pph images are not much different than a regular header
file.  What changes is the way those declarations get loaded in
memory.


Diego.