On Sat, 4 Jun 2011, Diego Novillo wrote:
> On Wed, Jun 1, 2011 at 15:19, Richard Guenther <rguent...@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 <rguent...@suse.de>
Novell / SUSE Labs
SUSE LINUX Products GmbH - Nuernberg - AG Nuernberg - HRB 16746
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer
