Re: [pph] Stream first/weak_object_global_name (issue4641086)

Re: [pph] Stream first/weak_object_global_name (issue4641086)

[gchare]

So I did find a better way of doing this, see Issue #4627087.

I'll go ahead and close this issue.

On 2011/07/01 01:27:26, Gabriel Charette wrote:
> notice_global_symbol is actually called in the parser (before we
stream out).

> I just confirmed this by setting a break point on the function in the
> pph generation of c1varoder.pph and hitting the function twice for the
> two variables defined in c1varorder.h

> It looks like rtl and assembler_name are set BEFORE we stream out...
> so that we would need to stream them (looks like assembler_name is
> already streamed, but not rtl...).

> Not streaming rtl causes it to be recreated later (which i think might
> be the cause of the bad ordering).

> Now another problem is that when two pph are generated, they are not
> aware of each other (and of their respective
> first/weak_global_object_name...) so even if we stream those
> variables, their could still be conflicts (and/or asm diffs..)

> However, I'm not sure about how make_decl_rtl works, but if it can
> regenerate the same rtl we had before (and even potentially solve the
> conflicts mentioned in the previous paragraph), we may not need to
> actually stream the rtl, we would just need to fix the ordering as the
> pph rtl's are being regenerated.

> (they are regenerated because DECL_RTL(NODE) either returns
> (NODE)->decl_with_rtl.rtl or calls make_decl_rtl(NODE) if it's
> NULL...)

> If we do this, I think we wanna have the first/weak_object_global_name
> set to what it should be by streaming it in from the first pph, to
> ensure all the rtl and assembler_name choices are made correctly (I'm
> not sure I understand all of this, but it seems to rely on those
> fields).

> Gab

> On Thu, Jun 30, 2011 at 4:33 PM, Diego Novillo
<mailto:dnovillo@google.com> wrote:
> > On Thu, Jun 30, 2011 at 16:24, Gabriel Charette
<mailto:gchare@google.com> wrote:
> >> first/weak_global_object_name are part of the global state.
> >>
> >> This seems to be used to produce the assembler names. They are set
only once
> as early as possible in the parsing; thus we should define it to be
whatever it
> was in the first pph (or even in the C file if it was set before any
pph was
> even loaded.
> >>
> >> I'm not sure exactly what this does, but trying to find a fix for
the asm
> diffs this is the first thing that I hit in the compiler logic that
was
> different in the good compiler vs the pph compiler. With this fix this
bit of
> logic is now correct.
> >> However, this doesn't fix any pph test (nor does it even change any
pph asm
> (I diff'ed the old bad assemblies (*.s+pph) with the new ones)).
> >
> > This does not look right to me. &nbsp;These two symbols are set when
> > calling notice_global_symbol, which is done during code generation
> > (you will see it called from cgraph_finalize_function,
> > cgraph_mark_reachable_node, etc).
> >
> > You are probably close to the cause of this relative ordering
problem,
> > but streaming these two globals is not the solution. &nbsp;They both
should
> > be set in the same order by both compilers, but not by forcing them
> > this way.
> >
> > One thing that may be happening here is that the order in which we
> > call cgraph_finalize_function is different in the two compilers.
> > That's what needs to change. &nbsp;One thing you could do is set a
> > breakpoint in notice_global_symbol. &nbsp;The two compilers should
be
> > calling it in the same order.
> >
> >
> > Diego.
> >



http://codereview.appspot.com/4641086/

Re: [pph] Stream first/weak_object_global_name (issue4641086)

[Gabriel Charette]

notice_global_symbol is actually called in the parser (before we stream out).

I just confirmed this by setting a break point on the function in the
pph generation of c1varoder.pph and hitting the function twice for the
two variables defined in c1varorder.h

It looks like rtl and assembler_name are set BEFORE we stream out...
so that we would need to stream them (looks like assembler_name is
already streamed, but not rtl...).

Not streaming rtl causes it to be recreated later (which i think might
be the cause of the bad ordering).

Now another problem is that when two pph are generated, they are not
aware of each other (and of their respective
first/weak_global_object_name...) so even if we stream those
variables, their could still be conflicts (and/or asm diffs..)

However, I'm not sure about how make_decl_rtl works, but if it can
regenerate the same rtl we had before (and even potentially solve the
conflicts mentioned in the previous paragraph), we may not need to
actually stream the rtl, we would just need to fix the ordering as the
pph rtl's are being regenerated.

(they are regenerated because DECL_RTL(NODE) either returns
(NODE)->decl_with_rtl.rtl or calls make_decl_rtl(NODE) if it's
NULL...)

If we do this, I think we wanna have the first/weak_object_global_name
set to what it should be by streaming it in from the first pph, to
ensure all the rtl and assembler_name choices are made correctly (I'm
not sure I understand all of this, but it seems to rely on those
fields).

Gab

On Thu, Jun 30, 2011 at 4:33 PM, Diego Novillo <dnovillo@google.com> wrote:
> On Thu, Jun 30, 2011 at 16:24, Gabriel Charette <gchare@google.com> wrote:
>> first/weak_global_object_name are part of the global state.
>>
>> This seems to be used to produce the assembler names. They are set only once as early as possible in the parsing; thus we should define it to be whatever it was in the first pph (or even in the C file if it was set before any pph was even loaded.
>>
>> I'm not sure exactly what this does, but trying to find a fix for the asm diffs this is the first thing that I hit in the compiler logic that was different in the good compiler vs the pph compiler. With this fix this bit of logic is now correct.
>> However, this doesn't fix any pph test (nor does it even change any pph asm (I diff'ed the old bad assemblies (*.s+pph) with the new ones)).
>
> This does not look right to me.  These two symbols are set when
> calling notice_global_symbol, which is done during code generation
> (you will see it called from cgraph_finalize_function,
> cgraph_mark_reachable_node, etc).
>
> You are probably close to the cause of this relative ordering problem,
> but streaming these two globals is not the solution.  They both should
> be set in the same order by both compilers, but not by forcing them
> this way.
>
> One thing that may be happening here is that the order in which we
> call cgraph_finalize_function is different in the two compilers.
> That's what needs to change.  One thing you could do is set a
> breakpoint in notice_global_symbol.  The two compilers should be
> calling it in the same order.
>
>
> Diego.
>

[pph] Stream first/weak_object_global_name (issue4641086)

[Gabriel Charette]

first/weak_global_object_name are part of the global state.

This seems to be used to produce the assembler names. They are set only once as early as possible in the parsing; thus we should define it to be whatever it was in the first pph (or even in the C file if it was set before any pph was even loaded.

I'm not sure exactly what this does, but trying to find a fix for the asm diffs this is the first thing that I hit in the compiler logic that was different in the good compiler vs the pph compiler. With this fix this bit of logic is now correct.
However, this doesn't fix any pph test (nor does it even change any pph asm (I diff'ed the old bad assemblies (*.s+pph) with the new ones)).

Tested with bootstrap build and pph regression testing.

2011-06-30  Gabriel Charette  <gchare@google.com>

	* Make-lang.in (pph-streamer-out.o): Add dependence on output.h
	(pph-streamer-in.o): Add dependence on output.h
	* pph-streamer-in.c (pph_read_file_contents): Stream in
	first_global_object_name and weak_global_object_name.
	* gcc/cp/pph-streamer-out.c (pph_write_file_contents): Stream out
	first_global_object_name and weak_global_object_name.

diff --git a/gcc/cp/Make-lang.in b/gcc/cp/Make-lang.in
index 0bb1a15..ec7596a 100644
--- a/gcc/cp/Make-lang.in
+++ b/gcc/cp/Make-lang.in
@@ -350,8 +350,8 @@ cp/pph-streamer.o: cp/pph-streamer.c $(CONFIG_H) $(SYSTEM_H) coretypes.h \
 cp/pph-streamer-out.o: cp/pph-streamer-out.c $(CONFIG_H) $(SYSTEM_H) \
 	coretypes.h $(TREE_H) tree-pretty-print.h $(LTO_STREAMER_H) \
 	$(CXX_PPH_STREAMER_H) $(CXX_PPH_H) $(TREE_PASS_H) version.h \
-	cppbuiltin.h tree-iterator.h
+	cppbuiltin.h tree-iterator.h output.h
 cp/pph-streamer-in.o: cp/pph-streamer-in.c $(CONFIG_H) $(SYSTEM_H) \
 	coretypes.h $(TREE_H) tree-pretty-print.h $(LTO_STREAMER_H) \
 	$(CXX_PPH_STREAMER_H) $(CXX_PPH_H) $(TREE_PASS_H) version.h \
-	cppbuiltin.h tree-iterator.h
+	cppbuiltin.h tree-iterator.h output.h
diff --git a/gcc/cp/pph-streamer-in.c b/gcc/cp/pph-streamer-in.c
index 3ac5243..c341d50 100644
--- a/gcc/cp/pph-streamer-in.c
+++ b/gcc/cp/pph-streamer-in.c
@@ -31,6 +31,7 @@ along with GCC; see the file COPYING3.  If not see
 #include "tree-pass.h"
 #include "version.h"
 #include "cppbuiltin.h"
+#include "output.h"
 
 /* Wrapper for memory allocation calls that should have their results
    registered in the PPH streamer cache.  DATA is the pointer returned
@@ -1309,7 +1310,7 @@ pph_read_file_contents (pph_stream *stream)
 {
   bool verified;
   cpp_ident_use *bad_use;
-  const char *cur_def;
+  const char *cur_def, *pph_fgon, *pph_wgon;
   cpp_idents_used idents_used;
   tree fndecl;
   unsigned i;
@@ -1325,6 +1326,16 @@ pph_read_file_contents (pph_stream *stream)
   /* Re-instantiate all the pre-processor symbols defined by STREAM.  */
   cpp_lt_replay (parse_in, &idents_used);
 
+  /* If first_global_object_name is not set yet, set it to the pph one.  */
+  pph_fgon = pph_in_string (stream);
+  if (!first_global_object_name)
+    first_global_object_name = pph_fgon;
+
+  /* If weak_global_object_name is not set yet, set it to the pph one.  */
+  pph_wgon = pph_in_string (stream);
+  if (!weak_global_object_name)
+    weak_global_object_name = pph_wgon;
+
   /* Read the bindings from STREAM and merge them with the current bindings.  */
   pph_in_scope_chain (stream);
 
diff --git a/gcc/cp/pph-streamer-out.c b/gcc/cp/pph-streamer-out.c
index acc0352..0d29f0b 100644
--- a/gcc/cp/pph-streamer-out.c
+++ b/gcc/cp/pph-streamer-out.c
@@ -31,6 +31,7 @@ along with GCC; see the file COPYING3.  If not see
 #include "tree-pass.h"
 #include "version.h"
 #include "cppbuiltin.h"
+#include "output.h"
 
 /* FIXME pph.  This holds the FILE handle for the current PPH file
    that we are writing.  It is necessary because the LTO callbacks do
@@ -1197,9 +1198,14 @@ static void
 pph_write_file_contents (pph_stream *stream, cpp_idents_used *idents_used)
 { 
   pph_out_identifiers (stream, idents_used);
+
+  pph_out_string (stream, first_global_object_name);
+  pph_out_string (stream, weak_global_object_name);
+
   pph_out_scope_chain (stream, scope_chain, false);
   if (flag_pph_dump_tree)
     pph_dump_namespace (pph_logfile, global_namespace);
+
   pph_out_tree (stream, keyed_classes, false);
   pph_out_tree_vec (stream, unemitted_tinfo_decls, false);
 }

--
This patch is available for review at http://codereview.appspot.com/4641086