On Wed, 1 Jun 2011, Diego Novillo wrote:
> On Wed, Jun 1, 2011 at 08:07, Richard Guenther <rguent...@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.
