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

[Richard Guenther <rguenther@suse.de>]

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

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.