Re: [pph] Fix 3 asm differences (issue4695048)

[Gabriel Charette <gchare@google.com>]

I like the modified implementation with VEC.

We probably want pph_register_decl_in_symtab to be inline as it does
so little now.

Now that you simply chainon bindings, you probably want to nreverse
them before you chain them on (this way we will stream them in from
first->last as this pacth does (to alloc stuff in order), but them in
the chain we want them to be last->first as they should be if they had
been pushed as they are in the original parser).

Gab

On Tue, Jul 12, 2011 at 12:19 PM, Diego Novillo <dnovillo@google.com> wrote:
>
> This patch is a slight adaptation of Gab's fix to the order in which
> we stream chains (http://codereview.appspot.com/4657092).  I mostly
> just changed how we keep the temporary list to reverse (it now uses a
> VEC instead of a custom-build linked list).
>
> The other change is in the reader.  We were not registering symbols in
> scope_chain->static_aggregates as they come from the PPH file (which
> would cause an ICE in x1hardlookup.cc).
>
> This fixes 3 tests, but we still have some asm differences that are
> similar in nature: when reinstantiating PPH images, the compiler emits
> some symbols in different order, causing different numbering and
> naming in the assembler output (we need to generate identical output
> from a pph or from text).
>
> Tested on x86_64.  Committed to branch.
>
>
> Diego.
>
> 2011-07-12   Diego Novillo  <dnovillo@google.com>
>
>        * pph-streamer-in.c (pph_register_decl_in_symtab): New.
>        (pph_register_binding_in_symtab): Rename from
>        pph_register_decls_in_symtab.  Update all users.
>        Do not call nreverse on bl->names and bl->namespaces.
>        Call pph_register_decl_in_symtab.
>        (pph_read_file_contents): Register decls in
>        FILE_STATIC_AGGREGATES.
>
> 2011-07-12  Gabriel Charette  <gchare@google.com>
>            Diego Novillo  <dnovillo@google.com>
>
>        * pph-streamer-out.c (pph_out_chained_tree): New.
>        (pph_out_chain_filtered): Add REVERSE_P parameter.
>        If REVERSE_P is set, write the list in reverse order.
>        Update all users.
>        (pph_out_binding_level): Write out lists bl->names,
>        bl->namespaces, bl->usings and bl->using_directives in
>        reverse.
>
>
> testsuite/ChangeLog.pph
> 2011-07-12  Gabriel Charette  <gchare@google.com>
>
>        * g++.dg/pph/c1pr44948-1a.cc: Mark fixed.
>        * g++.dg/pph/c2pr36533.cc: Mark fixed.
>        * g++.dg/pph/x2functions.cc: Mark fixed.
>
> diff --git a/gcc/cp/pph-streamer-in.c b/gcc/cp/pph-streamer-in.c
> index 63f8965..e7d1d00 100644
> --- a/gcc/cp/pph-streamer-in.c
> +++ b/gcc/cp/pph-streamer-in.c
> @@ -1175,29 +1175,33 @@ pph_in_lang_type (pph_stream *stream)
>  }
>
>
> +/* Register DECL with the middle end.  */
> +
> +static void
> +pph_register_decl_in_symtab (tree decl)
> +{
> +  if (TREE_CODE (decl) == VAR_DECL
> +      && TREE_STATIC (decl)
> +      && !DECL_EXTERNAL (decl))
> +    varpool_finalize_decl (decl);
> +}
> +
> +
>  /* Register all the symbols in binding level BL in the callgraph symbol
>    table.  */
>
>  static void
> -pph_register_decls_in_symtab (struct cp_binding_level *bl)
> +pph_register_binding_in_symtab (struct cp_binding_level *bl)
>  {
>   tree t;
>
> -  /* The chains are built backwards (ref: add_decl_to_level),
> -     reverse them before putting them back in.  */
> -  bl->names = nreverse (bl->names);
> -  bl->namespaces = nreverse (bl->namespaces);
> -
> +  /* Add file-local symbols to the varpool.  */
>   for (t = bl->names; t; t = DECL_CHAIN (t))
> -    {
> -      /* Add file-local symbols to the varpool.  */
> -      if (TREE_CODE (t) == VAR_DECL && TREE_STATIC (t) && !DECL_EXTERNAL (t))
> -       varpool_finalize_decl (t);
> -    }
> +    pph_register_decl_in_symtab (t);
>
>   /* Recurse into the namespaces contained in BL.  */
>   for (t = bl->namespaces; t; t = DECL_CHAIN (t))
> -    pph_register_decls_in_symtab (NAMESPACE_LEVEL (t));
> +    pph_register_binding_in_symtab (NAMESPACE_LEVEL (t));
>  }
>
>
> @@ -1220,7 +1224,7 @@ pph_in_scope_chain (pph_stream *stream)
>   new_bindings = pph_in_binding_level (stream, scope_chain->bindings);
>
>   /* Register all the symbols in STREAM with the call graph.  */
> -  pph_register_decls_in_symtab (new_bindings);
> +  pph_register_binding_in_symtab (new_bindings);
>
>   /* Merge the bindings from STREAM into saved_scope->bindings.  */
>   chainon (cur_bindings->names, new_bindings->names);
> @@ -1413,6 +1417,16 @@ pph_read_file_contents (pph_stream *stream)
>   file_static_aggregates = pph_in_tree (stream);
>   static_aggregates = chainon (file_static_aggregates, static_aggregates);
>
> +  /* Register all symbols in FILE_STATIC_AGGREGATES with the middle end.
> +     Each element of this list is an INIT_EXPR expression.  */
> +  for (t = file_static_aggregates; t; t = TREE_CHAIN (t))
> +    {
> +      tree lhs = TREE_OPERAND (TREE_PURPOSE (t), 0);
> +      tree rhs = TREE_OPERAND (TREE_PURPOSE (t), 1);
> +      pph_register_decl_in_symtab (lhs);
> +      pph_register_decl_in_symtab (rhs);
> +    }
> +
>   /* Expand all the functions with bodies that we read from STREAM.  */
>   FOR_EACH_VEC_ELT (tree, stream->fns_to_expand, i, fndecl)
>     {
> diff --git a/gcc/cp/pph-streamer-out.c b/gcc/cp/pph-streamer-out.c
> index f7bf739..9c9a1f8 100644
> --- a/gcc/cp/pph-streamer-out.c
> +++ b/gcc/cp/pph-streamer-out.c
> @@ -584,21 +584,44 @@ pph_out_label_binding (pph_stream *stream, cp_label_binding *lb, bool ref_p)
>  }
>
>
> +/* Outputs chained tree T by nulling out its chain first and restoring it
> +   after the streaming is done. STREAM and REF_P are as in
> +   pph_out_chain_filtered.  */
> +
> +static inline void
> +pph_out_chained_tree (pph_stream *stream, tree t, bool ref_p)
> +{
> +  tree saved_chain;
> +
> +  saved_chain = TREE_CHAIN (t);
> +  TREE_CHAIN (t) = NULL_TREE;
> +
> +  pph_out_tree_or_ref_1 (stream, t, ref_p, 2);
> +
> +  TREE_CHAIN (t) = saved_chain;
> +}
> +
> +
>  /* Output a chain of nodes to STREAM starting with FIRST.  Skip any
>    nodes that do not match FILTER.  REF_P is true if nodes in the chain
> -   should be emitted as references.  */
> +   should be emitted as references.  Stream the chain in the reverse order
> +   if REVERSE is true.*/
>
>  static void
>  pph_out_chain_filtered (pph_stream *stream, tree first, bool ref_p,
> -                          enum chain_filter filter)
> +                          enum chain_filter filter, bool reverse_p)
>  {
>   unsigned count;
> +  int i;
>   tree t;
> +  VEC(tree,gc) *reverse_list = NULL;
>
>   /* Special case.  If the caller wants no filtering, it is much
>      faster to just call pph_out_chain directly.  */
>   if (filter == NONE)
>     {
> +      if (reverse_p)
> +       nreverse (first);
>       pph_out_chain (stream, first, ref_p);
>       return;
>     }
> @@ -612,24 +635,31 @@ pph_out_chain_filtered (pph_stream *stream, tree first, bool ref_p,
>     }
>   pph_out_uint (stream, count);
>
> +  /* We cannot use the actual chain and simply reverse that before
> +     streaming out below as there are other chains being streamed out
> +     as part of streaming the trees in the current chain and this
> +     creates conflicts. Thus, we will create an array containing all
> +     the trees we want to stream out and stream that backwards without
> +     altering the chain itself.  */
> +  if (reverse_p && count > 0)
> +    reverse_list = VEC_alloc (tree, gc, count);
> +
>   /* Output all the nodes that match the filter.  */
>   for (t = first; t; t = TREE_CHAIN (t))
>     {
> -      tree saved_chain;
> -
>       /* Apply filters to T.  */
>       if (filter == NO_BUILTINS && DECL_P (t) && DECL_IS_BUILTIN (t))
>        continue;
>
> -      /* Clear TREE_CHAIN to avoid blindly recursing into the rest
> -        of the list.  */
> -      saved_chain = TREE_CHAIN (t);
> -      TREE_CHAIN (t) = NULL_TREE;
> -
> -      pph_out_tree_or_ref_1 (stream, t, ref_p, 2);
> -
> -      TREE_CHAIN (t) = saved_chain;
> +      if (reverse_p)
> +       VEC_quick_push (tree, reverse_list, t);
> +      else
> +       pph_out_chained_tree (stream, t, ref_p);
>     }
> +
> +  if (reverse_p && count > 0)
> +    FOR_EACH_VEC_ELT_REVERSE (tree, reverse_list, i, t)
> +      pph_out_chained_tree (stream, t, ref_p);
>  }
>
>
> @@ -648,13 +678,14 @@ pph_out_binding_level (pph_stream *stream, struct cp_binding_level *bl,
>   if (!pph_out_start_record (stream, bl))
>     return;
>
> -  pph_out_chain_filtered (stream, bl->names, ref_p, NO_BUILTINS);
> -  pph_out_chain_filtered (stream, bl->namespaces, ref_p, NO_BUILTINS);
> +  pph_out_chain_filtered (stream, bl->names, ref_p, NO_BUILTINS, true);
> +  pph_out_chain_filtered (stream, bl->namespaces, ref_p, NO_BUILTINS, true);
>
>   pph_out_tree_vec (stream, bl->static_decls, ref_p);
>
> -  pph_out_chain_filtered (stream, bl->usings, ref_p, NO_BUILTINS);
> -  pph_out_chain_filtered (stream, bl->using_directives, ref_p, NO_BUILTINS);
> +  pph_out_chain_filtered (stream, bl->usings, ref_p, NO_BUILTINS, true);
> +  pph_out_chain_filtered (stream, bl->using_directives, ref_p, NO_BUILTINS,
> +                         true);
>
>   pph_out_uint (stream, VEC_length (cp_class_binding, bl->class_shadowed));
>   FOR_EACH_VEC_ELT (cp_class_binding, bl->class_shadowed, i, cs)
> diff --git a/gcc/testsuite/g++.dg/pph/c1pr44948-1a.cc b/gcc/testsuite/g++.dg/pph/c1pr44948-1a.cc
> index c13ee87..0340dc5 100644
> --- a/gcc/testsuite/g++.dg/pph/c1pr44948-1a.cc
> +++ b/gcc/testsuite/g++.dg/pph/c1pr44948-1a.cc
> @@ -1,3 +1,3 @@
>  // { dg-xfail-if "INFINITE" { "*-*-*" } { "-fpph-map=pph.map" } }
> -// { dg-bogus "internal compiler error: in lto_streamer_cache_get, at lto-streamer.c" "" { xfail *-*-* } 0 }
> +// { dg-bogus "internal compiler error: in lto_get_pickled_tree, at lto-streamer-in.c" "" { xfail *-*-* } 0 }
>  #include "c0pr44948-1a.h"
> diff --git a/gcc/testsuite/g++.dg/pph/c2pr36533.cc b/gcc/testsuite/g++.dg/pph/c2pr36533.cc
> index d8d6d8c..b44e8c9 100644
> --- a/gcc/testsuite/g++.dg/pph/c2pr36533.cc
> +++ b/gcc/testsuite/g++.dg/pph/c2pr36533.cc
> @@ -1,3 +1,2 @@
>  /* { dg-options "-w -fpermissive" } */
> -// pph asm xdiff
>  #include "c1pr36533.h"
> diff --git a/gcc/testsuite/g++.dg/pph/x2functions.cc b/gcc/testsuite/g++.dg/pph/x2functions.cc
> index 78df01b..d4e2cf7 100644
> --- a/gcc/testsuite/g++.dg/pph/x2functions.cc
> +++ b/gcc/testsuite/g++.dg/pph/x2functions.cc
> @@ -1,5 +1,3 @@
> -// pph asm xdiff
> -
>  #include "x1functions.h"
>
>  int type::mbr_decl_then_def(int i)      // need body
>
> --
> This patch is available for review at http://codereview.appspot.com/4695048
>