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

[Richard Guenther <rguenther@suse.de>]

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

On Sat, 4 Jun 2011, Diego Novillo wrote:

> 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.

Thanks.  It feels more consistent this way.

> >> >> +     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.

Hm, ok.

> 
> >> >> @@ -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.

Do you remember if it was only void_zero_node that causes problems?
We could just special-case it in 
lto_input_integer_cst/lto_output_integer_cst.  Or even fix the C family
frontends to not create or use this strange node.  It seems to be
a special tree for "empty valid something" which could as well be
a new tree code with a singleton object.

> >> >> @@ -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?

Looks good to me.

Thanks,
Richard.

> 	* 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.
> 

-- 
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