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

[Richard Guenther <rguenther at suse dot de>]

On Wed, 1 Jun 2011, Diego Novillo wrote:

> On Wed, Jun 1, 2011 at 08:07, Richard Guenther <rguenther@suse.de> wrote:
> 
> >> Âstatic void cgraph_expand_all_functions (void);
> >> Âstatic void cgraph_mark_functions_to_output (void);
> >> @@ -1092,6 +1093,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 ();
> >
> > Ugh. ÂIsn't there a better entry for this? ÂAre you going to add
> >
> > Âif (flag_pph)
> > Â Âinit_hooks_some_other_way ();
> >
> > here? ÂIt looks it rather belongs to opts.c or toplev.c if the hooks
> > are really initialized dependent on compiler flags.
> 
> Not at all, this is for gimple, specifically.  The front end
> initializes hooks in its own way.  The problem here is that the gimple
> hooks are needed by the middle end.  If we initialize gimple hooks too
> early, the FE will override them.  So we need to initialize them after
> the front end is done (hence the location for this call).
> 
> I'm happy to move this somewhere else, but it needs to happen right
> before the middle end starts calling LTO pickling routines.
> 
> >
> >> Â Â/* 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/lto-streamer-in.c b/gcc/lto-streamer-in.c
> >> index 88966f2..801fe6f 100644
> >> --- a/gcc/lto-streamer-in.c
> >> +++ b/gcc/lto-streamer-in.c
> >> @@ -1833,6 +1833,7 @@ static void
> >> Âunpack_value_fields (struct bitpack_d *bp, tree expr)
> >> Â{
> >> Â Âenum tree_code code;
> >> + Âlto_streamer_hooks *h = streamer_hooks ();
> >
> > A function to access a global ... we have lang_hooks and targetm,
> > so please simply use streamer_hooks as a variable.
> > streamer_hooks ()->preload_common_nodes (cache) looks super-ugly.
> 
> I did not want to add yet another global.  I don't feel too strong
> about this one, given the presence of lang_hooks and targetm.  So, you
> prefer the direct global access?

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

> >> @@ -1864,26 +1865,11 @@ unpack_value_fields (struct bitpack_d *bp, tree expr)
> >> Â Âif (CODE_CONTAINS_STRUCT (code, TS_BLOCK))
> >> Â Â Âunpack_ts_block_value_fields (bp, expr);
> >>
> >> - Âif (CODE_CONTAINS_STRUCT (code, TS_SSA_NAME))
> >> - Â Â{
> >> - Â Â Â/* We only stream the version number of SSA names. Â*/
> >> - Â Â Âgcc_unreachable ();
> >> - Â Â}
> >> -
> >> - Âif (CODE_CONTAINS_STRUCT (code, TS_STATEMENT_LIST))
> >> - Â Â{
> >> - Â Â Â/* This is only used by GENERIC. Â*/
> >> - Â Â Âgcc_unreachable ();
> >> - Â Â}
> >> -
> >> - Âif (CODE_CONTAINS_STRUCT (code, TS_OMP_CLAUSE))
> >> - Â Â{
> >> - Â Â Â/* This is only used by High GIMPLE. Â*/
> >> - Â Â Âgcc_unreachable ();
> >> - Â Â}
> >> -
> >> Â Âif (CODE_CONTAINS_STRUCT (code, TS_TRANSLATION_UNIT_DECL))
> >> Â Â Âunpack_ts_translation_unit_decl_value_fields (bp, expr);
> >> +
> >> + Âif (h->unpack_value_fields)
> >> + Â Âh->unpack_value_fields (bp, expr);
> >
> > I suppose the LTO implementation has a gcc_unreachable () for
> > the cases we do not handle here?
> 
> Right.  This was already superfluous.  It's tested already by
> lto_is_streamable().
> 
> >
> >> Â}
> >>
> >>
> >> @@ -1935,8 +1921,17 @@ lto_materialize_tree (struct lto_input_block *ib, struct data_in *data_in,
> >> Â Â Â}
> >> Â Âelse
> >> Â Â Â{
> >> - Â Â Â/* All other nodes can be materialized with a raw make_node call. Â*/
> >> - Â Â Âresult = make_node (code);
> >> + Â Â Âlto_streamer_hooks *h = streamer_hooks ();
> >> +
> >> + Â Â Â/* For all other nodes, see if the streamer knows how to allocate
> >> + Â Â Âit. Â*/
> >> + Â Â Âif (h->alloc_tree)
> >> + Â Â result = h->alloc_tree (code, ib, data_in);
> >> +
> >> + Â Â Â/* If the hook did not handle it, materialize the tree with a raw
> >> + Â Â Âmake_node call. Â*/
> >> + Â Â Âif (result == NULL_TREE)
> >> + Â Â result = make_node (code);
> >> Â Â Â}
> >>
> >> Â#ifdef LTO_STREAMER_DEBUG
> >> @@ -2031,12 +2026,8 @@ lto_input_ts_decl_common_tree_pointers (struct lto_input_block *ib,
> >> Â{
> >> Â ÂDECL_SIZE (expr) = lto_input_tree (ib, data_in);
> >> Â ÂDECL_SIZE_UNIT (expr) = lto_input_tree (ib, data_in);
> >> -
> >> - Âif (TREE_CODE (expr) != FUNCTION_DECL
> >> - Â Â Â&& TREE_CODE (expr) != TRANSLATION_UNIT_DECL)
> >> - Â ÂDECL_INITIAL (expr) = lto_input_tree (ib, data_in);
> >> -
> >
> > Why move those? ÂDECL_INITIAL _is_ in decl_common.
> 
> I needed to move the handling of DECL_INITIAL in the writer.  This
> forces us to move the handling in the reader.  Otherwise, reader and
> writer will be out of sync (DECL_INITIAL is now written last).
> 
> > Where do those checks go? ÂOr do we simply lose them?
> 
> They already are in lto_is_streamable.  See above.
> 
> >> - Âif (TREE_CODE (result) == VAR_DECL)
> >> - Â Âlto_register_var_decl_in_symtab (data_in, result);
> >> - Âelse if (TREE_CODE (result) == FUNCTION_DECL && !DECL_BUILT_IN (result))
> >> - Â Âlto_register_function_decl_in_symtab (data_in, result);
> >> + Âif (h->register_decls_in_symtab_p)
> >> + Â Â{
> >> + Â Â Âif (TREE_CODE (result) == VAR_DECL)
> >> + Â Â lto_register_var_decl_in_symtab (data_in, result);
> >> + Â Â Âelse if (TREE_CODE (result) == FUNCTION_DECL && !DECL_BUILT_IN (result))
> >> + Â Â lto_register_function_decl_in_symtab (data_in, result);
> >> + Â Â}
> >
> > I would say we should defer symtab registering to uniquify_nodes time,
> > when we are sure we completely streamed in the tree we want to register
> > (then we don't have to deal with partially read trees in those functions).
> >
> > Can you work on a preparatory patch for this please? ÂThat would get
> > rid for the need of this hook and clean up things at the same time.
> 
> That's a much better idea.  Will do.

Thanks.

> >
> >>
> >> Â Â/* end_marker = */ lto_input_1_unsigned (ib);
> >>
> >> @@ -2682,6 +2659,22 @@ lto_read_tree (struct lto_input_block *ib, struct data_in *data_in,
> >> Â}
> >>
> >>
> >> +/* LTO streamer hook for reading GIMPLE trees. ÂIB and DATA_IN are as in
> >> + Â lto_read_tree. ÂEXPR is the tree was materialized by lto_read_tree and
> >> + Â needs GIMPLE specific data to be filled in. Â*/
> >> +
> >> +void
> >> +gimple_streamer_read_tree (struct lto_input_block *ib,
> >> + Â Â Â Â Â Â Â Â Â Â Â Âstruct data_in *data_in,
> >> + Â Â Â Â Â Â Â Â Â Â Â Âtree expr)
> >> +{
> >> + Âif (DECL_P (expr)
> >> + Â Â Â&& TREE_CODE (expr) != FUNCTION_DECL
> >> + Â Â Â&& TREE_CODE (expr) != TRANSLATION_UNIT_DECL)
> >> + Â ÂDECL_INITIAL (expr) = lto_input_tree (ib, data_in);
> >
> > What's wrong with doing this in the decl-common path?
> 
> Just that if the writer code moves, the reader needs to move as well
> to avoid out-of-sync problems.
> 
> >> @@ -772,9 +758,23 @@ lto_output_tree_ref (struct output_block *ob, tree expr)
> >> Â Â Â Âbreak;
> >>
> >> Â Â Âdefault:
> >> - Â Â Â/* No other node is indexable, so it should have been handled
> >> - Â Â Âby lto_output_tree. Â*/
> >> - Â Â Âgcc_unreachable ();
> >> + Â Â Â{
> >> + Â Â lto_streamer_hooks *h = streamer_hooks ();
> >> +
> >> + Â Â /* See if streamer hooks allows this node to be indexable with
> >> + Â Â Â ÂVAR_DECLs. Â*/
> >
> > with VAR_DECLs? ÂMore like "similar to global decls."?
> 
> Changed.
> 
> >
> >> + Â Â 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? ;)

> >
> >> + Â Â Â }
> >> + Â Â else
> >> + Â Â Â {
> >> + Â Â Â Â /* No other node is indexable, so it should have been
> >> + Â Â Â Â Â handled by lto_output_tree. Â*/
> >> + Â Â Â Â gcc_unreachable ();
> >> + Â Â Â }
> >> + Â Â Â}
> >> Â Â Â}
> >> Â}
> >>
> >> @@ -883,27 +883,10 @@ lto_output_ts_decl_common_tree_pointers (struct output_block *ob, tree expr,
> >> Â Âlto_output_tree_or_ref (ob, DECL_SIZE (expr), ref_p);
> >> Â Âlto_output_tree_or_ref (ob, DECL_SIZE_UNIT (expr), ref_p);
> >>
> >> - Âif (TREE_CODE (expr) != FUNCTION_DECL
> >> - Â Â Â&& TREE_CODE (expr) != TRANSLATION_UNIT_DECL)
> >> - Â Â{
> >> - Â Â Âtree initial = DECL_INITIAL (expr);
> >> - Â Â Âif (TREE_CODE (expr) == VAR_DECL
> >> - Â Â Â && (TREE_STATIC (expr) || DECL_EXTERNAL (expr))
> >> - Â Â Â && initial)
> >> - Â Â {
> >> - Â Â Â lto_varpool_encoder_t varpool_encoder = ob->decl_state->varpool_node_encoder;
> >> - Â Â Â struct varpool_node *vnode = varpool_get_node (expr);
> >> - Â Â Â if (!vnode)
> >> - Â Â Â Â initial = error_mark_node;
> >> - Â Â Â else if (!lto_varpool_encoder_encode_initializer_p (varpool_encoder,
> >> - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â vnode))
> >> - Â Â Â Â initial = NULL;
> >> - Â Â }
> >> -
> >> - Â Â Âlto_output_tree_or_ref (ob, initial, ref_p);
> >> - Â Â}
> >> + Â/* Do not stream DECL_INITIAL. Â*/
> >
> > That's a gross comment ;) ÂWe _do_ stream it, but now in a hook.
> 
> Fair enough.
> 
> > I suppose all the streamer functions should lose their lto_ prefix -
> > reading them with such comments will get confusing ...
> 
> Yeah, good point.  I'll send a rename patch.
> 
> >> Â Â/* We should not see any non-GIMPLE tree nodes here. Â*/
> >> Â Âcode = TREE_CODE (expr);
> >> - Âif (!lto_is_streamable (expr))
> >> - Â Âinternal_error ("tree code %qs is not supported in gimple streams",
> >> - Â Â Â Â Â Â Â Â tree_code_name[code]);
> >> + Âif (h->is_streamable && !h->is_streamable (expr))
> >> + Â Âinternal_error ("tree code %qs is not supported in %s streams",
> >> + Â Â Â Â Â Â Â Â h->name, tree_code_name[code]);
> >
> > I'd say this hook should always exist.
> 
> Sure.
> 
> >> Â Â/* The header of a tree node consists of its tag, the size of
> >> Â Â Â the node, and any other information needed to instantiate
> >> Â Â Â EXPR on the reading side (such as the number of slots in
> >> Â Â Â variable sized nodes). Â*/
> >> Â Âtag = lto_tree_code_to_tag (code);
> >> + Âgcc_assert ((unsigned) tag < (unsigned) LTO_NUM_TAGS);
> >
> > Doesn't Honzas streaming this as enum already assert this?
> 
> Yeah.  I had removed this before testing, but I sent out the original patch.
> 
> >> +/* GIMPLE hook for writing GIMPLE-specific parts of trees. ÂOB, EXPR
> >> + Â and REF_P are as in lto_write_tree. Â*/
> >> +
> >> +void
> >> +gimple_streamer_write_tree (struct output_block *ob, tree expr, bool ref_p)
> >> +{
> >> + Âif (TREE_CODE (expr) == FUNCTION_DECL)
> >> + Â Â{
> >> + Â Â Â/* DECL_SAVED_TREE holds the GENERIC representation for DECL.
> >> + Â Â ÂAt this point, it should not exist. ÂEither because it was
> >> + Â Â Âconverted to gimple or because DECL didn't have a GENERIC
> >> + Â Â Ârepresentation in this TU. Â*/
> >> + Â Â Âgcc_assert (DECL_SAVED_TREE (expr) == NULL_TREE);
> >> + Â Â}
> >
> > I think we can drop this check.
> 
> Done.
> 
> >> @@ -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?  Yes, I think this isn't
the time to merge this piece.  Ah, I think I get it - we don't stream
integer constants as trees.  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) - 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,
or only handle all pre-loaded nodes that way.

> >> @@ -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.
 
> >> +
> >> +
> >> +/* Return true if EXPR is a tree node that can be written to disk. Â*/
> >> +
> >> +static inline bool
> >> +lto_is_streamable (tree expr)
> >> +{
> >> + Âenum tree_code code = TREE_CODE (expr);
> >> +
> >> + Â/* Notice that we reject SSA_NAMEs as well. ÂWe only emit the SSA
> >> + Â Â name version in lto_output_tree_ref (see output_ssa_names). Â*/
> >> + Âreturn !is_lang_specific (expr)
> >> + Â Â Â&& code != SSA_NAME
> >> + Â Â Â&& code != CALL_EXPR
> >> + Â Â Â&& code != LANG_TYPE
> >> + Â Â Â&& code != MODIFY_EXPR
> >> + Â Â Â&& code != INIT_EXPR
> >> + Â Â Â&& code != TARGET_EXPR
> >> + Â Â Â&& code != BIND_EXPR
> >> + Â Â Â&& code != WITH_CLEANUP_EXPR
> >> + Â Â Â&& code != STATEMENT_LIST
> >> + Â Â Â&& code != OMP_CLAUSE
> >> + Â Â Â&& code != OPTIMIZATION_NODE
> >> + Â Â Â&& (code == CASE_LABEL_EXPR
> >> + Â Â Â Â Â|| code == DECL_EXPR
> >> + Â Â Â Â Â|| TREE_CODE_CLASS (code) != tcc_statement);
> >> +}
> >
> > This change (with the removal of the cases with the unreachable()s)
> > could be made separately (directly calling it instead of hooking it).
> 
> I still need to hook this because pph_is_streamable is much more lenient.

Of course, you can do that in the hookization patch.  The above is just
to make that smaller.

> >
> >> +
> >> +/* 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?

Richard.