Issue 4695048: [pph] Fix 3 asm differences (Closed)
|
Created:
12 years, 9 months ago by Diego Novillo Modified:
12 years, 8 months ago Reviewers:
CC:
Lawrence Crowl, Gabriel Charette, gcc-patches_gcc.gnu.org Visibility:
Public. |
Patch Set 1 #
MessagesTotal messages: 4
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
Sign in to reply to this message.
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 >
Sign in to reply to this message.
On 11-07-12 16:34 , Gabriel Charette wrote: > We probably want pph_register_decl_in_symtab to be inline as it does > so little now. It doesn't really matter all that much. Given that it's a static function, the compiler will inline it (or not) as an optimization. The 'inline' keyword is more and more just a suggestion than an actual guarantee. > 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). Perhaps, but first I want to make sure we really want to reverse them all. Not every list is processed from back to front. Diego.
Sign in to reply to this message.
On Tue, Jul 12, 2011 at 3:25 PM, Diego Novillo <dnovillo@google.com> wrote: > On 11-07-12 16:34 , Gabriel Charette wrote: > >> We probably want pph_register_decl_in_symtab to be inline as it does >> so little now. > > It doesn't really matter all that much. Given that it's a static function, > the compiler will inline it (or not) as an optimization. The 'inline' > keyword is more and more just a suggestion than an actual guarantee. > OK >> 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). > > Perhaps, but first I want to make sure we really want to reverse them all. > Not every list is processed from back to front. > Ok, well for now in the code though they are inserted in the current_bindings in the reverse order (names and namespaces for sure, usings I'm not sure) then they were in the parser when originally written out (I don't know if this causes problems...?) Gab
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
