[lto] Merge streamer hooks from pph branch. (issue4568043)

On Wed, Jun 1, 2011 at 15:19, Richard Guenther <rguenther@suse.de> wrote:

> Yes, I see no benefit of using a global function to get access
> to the address of a global variable.

There is the minor benefit of being able to control  access to it, but
I don't have a really convincing reason to give you, so I changed it
to a global.

>> >> +     if (h->indexable_with_decls_p && h->indexable_with_decls_p (expr))
>> >> +       {
>> >> +         output_record_start (ob, LTO_global_decl_ref);
>> >> +         lto_output_var_decl_index (ob->decl_state, ob->main_stream, expr);
>> >
>> > Why hook it this way and not
>> >
>> >             if (h->output_tree_ref
>> >                 && h->output_tree_ref (...))
>> >               break;
>> >             gcc_unreachable ();
>> >
>> > I find the flag vs. function hook stuff somewhat odd.
>>
>> Sure.  It's
>
> ... missing words? ;)

Sorry.  I meant to continue with "It's just that this particular hook
is simpler if it only needs to decide whether the node can be written
as a decl reference.  The code to write the node will be the same
everywhere."  It would lead to duplication and the hooks would need to
know more internal details of the generic streamer (they need to write
the reference in exactly the way that lto_input_tree is expecting).

This is not a flag, actually.  It's a predicate function called on a
node.  If the node passes the predicate, then it is written in the
decl index table.

>> >> @@ -1438,8 +1450,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;
>> >> +     }
>> >
>> > ???  We should not arrive here for such global trees.  Please do not
>> > merge this part of the patch as part of the hook introducing (keep
>> > patches simple, make them do a single thing ...)
>>
>> Not sure what you are objecting to.  We do execute this for global
>> trees in the C++ FE (as described in the comment).  Are you objecting
>> to never handling unique constants or to merging this handling until
>> the pph bits are in?
>
> Are you not pre-loading those global trees then?

I am, but since the streamer always wanted to stream INTEGER_CSTs
separately, it wasn't getting a chance to check the cache first.

> Yes, I think this isn't the time to merge this piece.

No problem.  I'll keep this part in the branch.

> Ah, I think I get it - we don't stream integer constants as trees.

Right.

> But it's odd that you only handle this
> for integer-csts and not other trees we don't stream as-is (and
> thus do not enter into the cache)

Because constants are the only ones that are handled right before the
cache is consulted.  Every other pre-built tree can be cached
(regardless of whether it's handled by the streamer).

> - I think this should be moved up a level and made generic to handle all trees.  Or we should
> handle integer-csts similar to builtins - always enter them in the cache,

I tried this, but the result was sub-optimal
(http://gcc.gnu.org/ml/gcc-patches/2011-05/msg00563.html)
Putting constants in the cache, caused various failures which I never
fully debugged because I noticed an increase in the object size (I
remember it was noticeable, but not by how much).

I didn't insist too much with this approach, so maybe I could try it again.

> or only handle all pre-loaded nodes that way.

That is what we do.  Pre-loaded nodes always go through the cache.
The problem were pre-loaded constants, since they *never* go through
the cache.

>> >> @@ -2238,6 +2269,8 @@ static void
>> >>  lto_writer_init (void)
>> >>  {
>> >>    lto_streamer_init ();
>> >> +  if (streamer_hooks ()->writer_init)
>> >> +    streamer_hooks ()->writer_init ();
>> >
>> > This hook should always exist.  Why is this called in a context with
>> > lto_*?
>>
>> There is common streaming code that both the gimple and c++ streamers
>> use.  I suppose both could call lto_streamer_init and then call their
>> own initializer routines instead of doing a hook.
>
> Yeah, I'd prefer that.  I don't have a clear picture yet on what
> piece of the streamer is actually shared.

Sure.  Done.

>> >> +
>> >> +/* Initialize all the streamer hooks used for streaming GIMPLE.  */
>> >> +
>> >> +void
>> >> +gimple_streamer_hooks_init (void)
>> >
>> > It's more like lto_streamer_hooks_init, no?  You are basically
>> > turning lto_streamer_* to tree_streamer_* and make lto_streamer_*
>> > tree_streamer_* + lto_streamer_hooks, no?
>>
>> I was about to call them gimple_streamer_hooks, but lto_streamer_hooks
>> is also fine with me.  Subsequent patch.  So:
>>
>> 1- Call the generic implementation and streamer hooks tree_streamer_*
>> 2- Rename the lto-specific parts of streaming to lto_streamer_*
>> 3- Move generic streaming implementation to tree-streamer.[ch]
>>
>> Does that sound OK?
>
> That sounds ok.  You are only sharing the code streaming trees, right?

Right.  Patch coming up on top of the revised patch for streamer hooks.

The attached revision of the patch has been tested again with an LTO
profiledbootstrap on x86_64.  OK for trunk?

	* Makefile.in (lto-compress.o): Add dependency on LTO_STREAMER_H.
	(cgraph.o): Likewise.
	(cgraphunit.o): Likewise.
	* cgraphunit.c: Include lto-streamer.h
	(cgraph_finalize_compilation_unit): Call lto_streamer_hooks_init
	if LTO is enabled.
	* lto-streamer-in.c (unpack_value_fields): Call
	streamer_hooks.unpack_value_fields if set.
	(lto_materialize_tree): For unhandled nodes, first try to
	call lto_streamer_hooks.alloc_tree, if it exists.
	(lto_input_ts_decl_common_tree_pointers): Move reading of
	DECL_INITIAL to lto_streamer_read_tree.
	(lto_read_tree): Call lto_streamer_hooks.read_tree if set.
	(lto_streamer_read_tree): New.
	(lto_reader_init): Rename from lto_init_reader.
	Move initialization code to lto/lto.c.
	* lto-streamer-out.c (pack_value_fields): Call
	streamer_hooks.pack_value_fields if set.
	(lto_output_tree_ref): For tree nodes that are not
	normally indexable, call streamer_hooks.indexable_with_decls_p
	before giving up.
	(lto_output_ts_decl_common_tree_pointers): Move handling
	for FUNCTION_DECL and TRANSLATION_UNIT_DECL to
	lto_streamer_write_tree.
	(lto_output_tree_header): Call streamer_hooks.is_streamable
	instead of lto_is_streamable.
	Call lto_streamer_hooks.output_tree_header if set.
	(lto_write_tree): Call lto_streamer_hooks.write_tree if
	set.
	(lto_streamer_write_tree): New.
	(lto_output): Call lto_streamer_init directly.
	(lto_writer_init): Remove.
	* lto-streamer.c (streamer_hooks): New.
	(lto_streamer_cache_create): Call streamer_hooks.preload_common_nodes
	instead of lto_preload_common_nodes.
	(lto_is_streamable): Move from lto-streamer.h
	(lto_streamer_hooks_init): New.
	(streamer_hooks): New.
	(streamer_hooks_init): New.
	* lto-streamer.h (struct output_block): Forward declare.
	(struct lto_input_block): Likewise.
	(struct data_in): Likewise.
	(struct bitpack_d): Likewise.
	(struct streamer_hooks): Declare.
	(streamer_hooks): Declare.
	(lto_streamer_hooks_init): Declare.
	(lto_streamer_write_tree): Declare.
	(lto_streamer_read_tree): Declare.
	(streamer_hooks_init): Declare.
	(lto_is_streamable): Move to lto-streamer.c

lto/ChangeLog

	* lto.c (lto_init): New.
	(lto_main): Call it.

Diego.

[lto] Merge streamer hooks from pph branch. (issue4568043)

Commit Message

Comments

Patch