[pph] Allow streamer to define unique INTEGER_CST nodes (issue4489044)

[pph] Allow streamer to define unique INTEGER_CST nodes (issue4489044)

[Diego Novillo]

I'm not altogether happy with this approach, so I'm looking for
suggestions on how to address this issue.

The front end defines a bunch of unique constants in c_global_trees
that are not meant to be shared and are subject to pointer comparisons
throughout the front end.  Morevoer, some constants do not even have a
"valid" type.  For instance, void_zero_node has type void, which
build_int_cst_wide refuses to build.  So, we were ICEing when trying
to materialize these functions in the reader.

Other constants are fine, but they are pointer-compared against
c_global_trees[], so I need them to be materialized into the same
address.

Initially, I thought I would just make the streamer treat constants
the same as any other tree node, but this increases the size of the
on-disk representation.  Writing cache references for constants takes
more space.  It was also quite invasive, I was introducing regressions
in LTO and having a perfectly horrible time with it.

The approach I ended up taking is to allow the streamer to declare
that it has some INTEGER_CSTs that need to be unique.  Since those
unique constants are preloaded in the cache, we can simply stream
references to them if we find a match.

So, regular constants are streamed like always, but unique constants
are streamed as cache references.  This does not affect the gimple
streamer and allows the C++ FE to preload these constants in the cache
and continue to use pointer equality throughout the parser.

This was the least invasive and quick solution I could come up for
now.  Any other ideas?

Tested on x86_64.  Committed to pph.


Diego.

cp/ChangeLog.pph

	* pph-streamer.c (pph_stream_hooks_init): Set
	has_unique_integer_csts_p field to true.

ChangeLog.pph

	* lto-streamer-out.c (lto_output_tree): If the streamer
	has unique INTEGER_CST nodes and a match is found in the
	streamer cache, do not call lto_output_integer_cst.
	* lto-streamer.h (struct lto_streamer_hooks): Add field
	has_unique_integer_csts_p.

diff --git a/gcc/cp/pph-streamer.c b/gcc/cp/pph-streamer.c
index efac32e..18a5e25 100644
--- a/gcc/cp/pph-streamer.c
+++ b/gcc/cp/pph-streamer.c
@@ -101,6 +101,7 @@ pph_stream_hooks_init (void)
   h->unpack_value_fields = pph_stream_unpack_value_fields;
   h->alloc_tree = pph_stream_alloc_tree;
   h->output_tree_header = pph_stream_output_tree_header;
+  h->has_unique_integer_csts_p = true;
 }
 
 
diff --git a/gcc/lto-streamer-out.c b/gcc/lto-streamer-out.c
index b578419..a7f0965 100644
--- a/gcc/lto-streamer-out.c
+++ b/gcc/lto-streamer-out.c
@@ -1387,8 +1387,27 @@ lto_output_tree (struct output_block *ob, tree expr, bool ref_p)
      to be materialized by the reader (to implement TYPE_CACHED_VALUES).  */
   if (TREE_CODE (expr) == INTEGER_CST)
     {
-      lto_output_integer_cst (ob, expr, ref_p);
-      return;
+      bool is_special;
+
+     /* There are some constants that are special to the streamer
+	(e.g., void_zero_node, truthvalue_false_node).
+	These constants cannot be rematerialized with
+	build_int_cst_wide because they may actually lack a type (like
+	void_zero_node) and they need to be pointer-identical to trees
+	materialized by the compiler tables like global_trees or
+	c_global_trees.
+
+	If the streamer told us that it has special constants, they
+	will be preloaded in the streamer cache.  If we find a match,
+	then stream the constant as a reference so the reader can
+	re-materialize it from the cache.  */
+      is_special = streamer_hooks ()->has_unique_integer_csts_p
+		   && lto_streamer_cache_lookup (ob->writer_cache, expr, NULL);
+      if (!is_special)
+	{
+	  lto_output_integer_cst (ob, expr, ref_p);
+	  return;
+	}
     }
 
   existed_p = lto_streamer_cache_insert (ob->writer_cache, expr, &ix);
diff --git a/gcc/lto-streamer.h b/gcc/lto-streamer.h
index 8be17da..9b64619 100644
--- a/gcc/lto-streamer.h
+++ b/gcc/lto-streamer.h
@@ -113,6 +113,12 @@ typedef struct lto_streamer_hooks {
      global symbol tables.  */
   unsigned register_decls_in_symtab_p : 1;
 
+  /* Non-zero if the streamer has special constants that cannot be
+     shared and are used in pointer-equality tests (e.g., void_zero_node,
+     truthvalue_false_node, etc).  These constants will be present in
+     the streamer cache and should be streamed as references.  */
+  unsigned has_unique_integer_csts_p : 1;
+
   /* Called by lto_materialize_tree for tree nodes that it does not
      know how to allocate memory for.  If defined, this hook should
      return a new tree node of the given code.  The data_in and

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

Re: [pph] Allow streamer to define unique INTEGER_CST nodes (issue4489044)

[Richard Guenther]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 5387 bytes --]

On Fri, 6 May 2011, Diego Novillo wrote:

> 
> I'm not altogether happy with this approach, so I'm looking for
> suggestions on how to address this issue.
> 
> The front end defines a bunch of unique constants in c_global_trees
> that are not meant to be shared and are subject to pointer comparisons
> throughout the front end.  Morevoer, some constants do not even have a
> "valid" type.  For instance, void_zero_node has type void, which
> build_int_cst_wide refuses to build.  So, we were ICEing when trying
> to materialize these functions in the reader.
> 
> Other constants are fine, but they are pointer-compared against
> c_global_trees[], so I need them to be materialized into the same
> address.
> 
> Initially, I thought I would just make the streamer treat constants
> the same as any other tree node, but this increases the size of the
> on-disk representation.  Writing cache references for constants takes
> more space.  It was also quite invasive, I was introducing regressions
> in LTO and having a perfectly horrible time with it.
> 
> The approach I ended up taking is to allow the streamer to declare
> that it has some INTEGER_CSTs that need to be unique.  Since those
> unique constants are preloaded in the cache, we can simply stream
> references to them if we find a match.
> 
> So, regular constants are streamed like always, but unique constants
> are streamed as cache references.  This does not affect the gimple
> streamer and allows the C++ FE to preload these constants in the cache
> and continue to use pointer equality throughout the parser.
> 
> This was the least invasive and quick solution I could come up for
> now.  Any other ideas?

We have the same issue for va_list_type_node which is compared
by equality, too, but may be merged with other types.  I thought
about inventing sth similar to builtin function codes for such
stuff ...

But I'm not sure that's really the way to go.  At least the problem
sounds similar to yours.

Richard.

> Tested on x86_64.  Committed to pph.
> 
> 
> Diego.
> 
> cp/ChangeLog.pph
> 
> 	* pph-streamer.c (pph_stream_hooks_init): Set
> 	has_unique_integer_csts_p field to true.
> 
> ChangeLog.pph
> 
> 	* lto-streamer-out.c (lto_output_tree): If the streamer
> 	has unique INTEGER_CST nodes and a match is found in the
> 	streamer cache, do not call lto_output_integer_cst.
> 	* lto-streamer.h (struct lto_streamer_hooks): Add field
> 	has_unique_integer_csts_p.
> 
> diff --git a/gcc/cp/pph-streamer.c b/gcc/cp/pph-streamer.c
> index efac32e..18a5e25 100644
> --- a/gcc/cp/pph-streamer.c
> +++ b/gcc/cp/pph-streamer.c
> @@ -101,6 +101,7 @@ pph_stream_hooks_init (void)
>    h->unpack_value_fields = pph_stream_unpack_value_fields;
>    h->alloc_tree = pph_stream_alloc_tree;
>    h->output_tree_header = pph_stream_output_tree_header;
> +  h->has_unique_integer_csts_p = true;
>  }
>  
>  
> diff --git a/gcc/lto-streamer-out.c b/gcc/lto-streamer-out.c
> index b578419..a7f0965 100644
> --- a/gcc/lto-streamer-out.c
> +++ b/gcc/lto-streamer-out.c
> @@ -1387,8 +1387,27 @@ lto_output_tree (struct output_block *ob, tree expr, bool ref_p)
>       to be materialized by the reader (to implement TYPE_CACHED_VALUES).  */
>    if (TREE_CODE (expr) == INTEGER_CST)
>      {
> -      lto_output_integer_cst (ob, expr, ref_p);
> -      return;
> +      bool is_special;
> +
> +     /* There are some constants that are special to the streamer
> +	(e.g., void_zero_node, truthvalue_false_node).
> +	These constants cannot be rematerialized with
> +	build_int_cst_wide because they may actually lack a type (like
> +	void_zero_node) and they need to be pointer-identical to trees
> +	materialized by the compiler tables like global_trees or
> +	c_global_trees.
> +
> +	If the streamer told us that it has special constants, they
> +	will be preloaded in the streamer cache.  If we find a match,
> +	then stream the constant as a reference so the reader can
> +	re-materialize it from the cache.  */
> +      is_special = streamer_hooks ()->has_unique_integer_csts_p
> +		   && lto_streamer_cache_lookup (ob->writer_cache, expr, NULL);
> +      if (!is_special)
> +	{
> +	  lto_output_integer_cst (ob, expr, ref_p);
> +	  return;
> +	}
>      }
>  
>    existed_p = lto_streamer_cache_insert (ob->writer_cache, expr, &ix);
> diff --git a/gcc/lto-streamer.h b/gcc/lto-streamer.h
> index 8be17da..9b64619 100644
> --- a/gcc/lto-streamer.h
> +++ b/gcc/lto-streamer.h
> @@ -113,6 +113,12 @@ typedef struct lto_streamer_hooks {
>       global symbol tables.  */
>    unsigned register_decls_in_symtab_p : 1;
>  
> +  /* Non-zero if the streamer has special constants that cannot be
> +     shared and are used in pointer-equality tests (e.g., void_zero_node,
> +     truthvalue_false_node, etc).  These constants will be present in
> +     the streamer cache and should be streamed as references.  */
> +  unsigned has_unique_integer_csts_p : 1;
> +
>    /* Called by lto_materialize_tree for tree nodes that it does not
>       know how to allocate memory for.  If defined, this hook should
>       return a new tree node of the given code.  The data_in and
> 
> --
> This patch is available for review at http://codereview.appspot.com/4489044
> 
> 

-- 
Richard Guenther <rguenther@suse.de>
Novell / SUSE Labs
SUSE LINUX Products GmbH - Nuernberg - AG Nuernberg - HRB 16746
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer