[pph] Stream DECL_CHAIN only for VAR/FUNCTION_DECLs that are part of a RECORD_OR_UNION_TYPE

[Gabriel Charette, 2011-07-09 01:20:50]

The fix of streaming DECL_CHAIN in pph for VAR_DECL and FUNCTION_DECL was
introduced to fix "member not found" errors for structs (and we don't have any
tests with unions (we probably should...), but I believe they work the same).
The fix was too general and was actually interfering with something I'm trying
to do as part of the bug fix I'm on.

This thus restricts the streaming of the DECL_CHAIN for those two types only
when needed.

Again, this doesn't fix any tests, but helps me in the current bug I'm working
on.

Tested with bootstrap and pph regression testing.

Gab

2011-07-08  Gabriel Charette  <gchare@google.com>

	* pph-streamer-in.c (pph_in_function_decl): Stream in
	DECL_CHAIN of FUNCTION_DECL only if it's part of a RECORD_OR_UNION_TYPE
	(pph_read_tree): Stream in DECL_CHAIN of VAR_DECL only if it's part
	of a RECORD_OR_UNION_TYPE.
	* pph-streamer-out.c (pph_out_function_decl): Stream out
	DECL_CHAIN of FUNCTION_DECL only if it's part of a RECORD_OR_UNION_TYPE
	(pph_write_tree): Stream out DECL_CHAIN of VAR_DECL only if it's part
	of a RECORD_OR_UNION_TYPE.

diff --git a/gcc/cp/pph-streamer-in.c b/gcc/cp/pph-streamer-in.c
index 0bab93b..d78ee91 100644
--- a/gcc/cp/pph-streamer-in.c
+++ b/gcc/cp/pph-streamer-in.c
@@ -1396,7 +1396,8 @@ pph_in_function_decl (pph_stream *stream, tree fndecl)
   pph_in_lang_specific (stream, fndecl);
   DECL_SAVED_TREE (fndecl) = pph_in_tree (stream);
   DECL_STRUCT_FUNCTION (fndecl) = pph_in_struct_function (stream, fndecl);
-  DECL_CHAIN (fndecl) = pph_in_tree (stream);
+  if (DECL_CONTEXT (fndecl) && RECORD_OR_UNION_TYPE_P (DECL_CONTEXT (fndecl)))
+    DECL_CHAIN (fndecl) = pph_in_tree (stream);
   if (DECL_SAVED_TREE (fndecl))
     VEC_safe_push (tree, gc, stream->fns_to_expand, fndecl);
 }
@@ -1436,7 +1437,9 @@ pph_read_tree (struct lto_input_block *ib
ATTRIBUTE_UNUSED,
       DECL_INITIAL (expr) = pph_in_tree (stream);
       pph_in_lang_specific (stream, expr);
       /* DECL_CHAIN is handled by generic code, except for VAR_DECLs.  */
-      if (TREE_CODE (expr) == VAR_DECL)
+      if (TREE_CODE (expr) == VAR_DECL
+	  && DECL_CONTEXT (expr)
+	  && RECORD_OR_UNION_TYPE_P (DECL_CONTEXT (expr)))
 	DECL_CHAIN (expr) = pph_in_tree (stream);
       break;
 
diff --git a/gcc/cp/pph-streamer-out.c b/gcc/cp/pph-streamer-out.c
index 089bb13..d1e757f 100644
--- a/gcc/cp/pph-streamer-out.c
+++ b/gcc/cp/pph-streamer-out.c
@@ -1257,7 +1257,8 @@ pph_out_function_decl (pph_stream *stream, tree fndecl,
bool ref_p)
   pph_out_lang_specific (stream, fndecl, ref_p);
   pph_out_tree_or_ref_1 (stream, DECL_SAVED_TREE (fndecl), ref_p, 3);
   pph_out_struct_function (stream, DECL_STRUCT_FUNCTION (fndecl), ref_p);
-  pph_out_tree_or_ref_1 (stream, DECL_CHAIN (fndecl), ref_p, 3);
+  if (DECL_CONTEXT (fndecl) && RECORD_OR_UNION_TYPE_P (DECL_CONTEXT (fndecl)))
+    pph_out_tree_or_ref_1 (stream, DECL_CHAIN (fndecl), ref_p, 3);
 }
 
 /* Callback for writing ASTs to a stream.  This writes all the fields
@@ -1292,7 +1293,9 @@ pph_write_tree (struct output_block *ob, tree expr, bool
ref_p)
       pph_out_tree_or_ref_1 (stream, DECL_INITIAL (expr), ref_p, 3);
       pph_out_lang_specific (stream, expr, ref_p);
       /* DECL_CHAIN is handled by generic code, except for VAR_DECLs.  */
-      if (TREE_CODE (expr) == VAR_DECL)
+      if (TREE_CODE (expr) == VAR_DECL
+	  && DECL_CONTEXT (expr)
+	  && RECORD_OR_UNION_TYPE_P (DECL_CONTEXT (expr)))
 	pph_out_tree_or_ref_1 (stream, DECL_CHAIN (expr), ref_p, 3);
       break;
 

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

[Diego Novillo, 2011-07-12 19:21:44]

On Fri, Jul 8, 2011 at 21:20, Gabriel Charette <gchare@google.com> wrote:

> 2011-07-08  Gabriel Charette  <gchare@google.com>
>
>        * pph-streamer-in.c (pph_in_function_decl): Stream in
>        DECL_CHAIN of FUNCTION_DECL only if it's part of a RECORD_OR_UNION_TYPE
>        (pph_read_tree): Stream in DECL_CHAIN of VAR_DECL only if it's part
>        of a RECORD_OR_UNION_TYPE.
>        * pph-streamer-out.c (pph_out_function_decl): Stream out
>        DECL_CHAIN of FUNCTION_DECL only if it's part of a RECORD_OR_UNION_TYPE
>        (pph_write_tree): Stream out DECL_CHAIN of VAR_DECL only if it's part
>        of a RECORD_OR_UNION_TYPE.

Gab, do you still need this patch?  In principle, it doesn't make a
lot of sense to restrict when we save the DECL_CHAIN in this way.
It's not obvious what this would fix or help with.


Diego.

[Gabriel Charette, 2011-07-12 20:43:34]

The reason I put that patch out is that sometimes, when we stream an
actual chain, lto_input_chain is going to rebuild the new chain it's
meant to be, but then pph_read_tree (which is called after by the
name_hook to finish reading special parts of the tree) overwrites the
DECL_CHAIN that was introduced by lto_input_chain.

The only time we need to actually stream in/out the DECL_CHAIN is when
streaming unions/structs because from what I looked at in lto it looks
like we are not doing lto_output_chain, but lto_output_tree on the
first member of the fields' chain (not sure how that even works in
lto... but in pph we used to only get the first member of structs
streamed and streaming DECL_CHAIN was the fix for it...)

I introduced this fix because it broke my patch trying to stream out
the chains backwards (as it would overwrite the chain I was trying to
create backwards on input, I think this didn't show up before because
the chain being built on input was the same as the one existing on
output (thus overwriting with the same value...) )

Even if this doesn't break tests anymore, we probably still want this,
no point adding stuff to the pph image that is not needed...

Any idea why lto doesn't call lto_output_chain, but simply
lto_output_tree to output the chains for struct/union?

Gab

On Tue, Jul 12, 2011 at 12:21 PM, Diego Novillo <dnovillo@google.com> wrote:
> On Fri, Jul 8, 2011 at 21:20, Gabriel Charette <gchare@google.com> wrote:
>
>> 2011-07-08  Gabriel Charette  <gchare@google.com>
>>
>>        * pph-streamer-in.c (pph_in_function_decl): Stream in
>>        DECL_CHAIN of FUNCTION_DECL only if it's part of a
RECORD_OR_UNION_TYPE
>>        (pph_read_tree): Stream in DECL_CHAIN of VAR_DECL only if it's part
>>        of a RECORD_OR_UNION_TYPE.
>>        * pph-streamer-out.c (pph_out_function_decl): Stream out
>>        DECL_CHAIN of FUNCTION_DECL only if it's part of a
RECORD_OR_UNION_TYPE
>>        (pph_write_tree): Stream out DECL_CHAIN of VAR_DECL only if it's part
>>        of a RECORD_OR_UNION_TYPE.
>
> Gab, do you still need this patch?  In principle, it doesn't make a
> lot of sense to restrict when we save the DECL_CHAIN in this way.
> It's not obvious what this would fix or help with.
>
>
> Diego.
>

[Diego Novillo, 2011-07-12 22:32:35]

On 11-07-12 16:43 , Gabriel Charette wrote:

> Even if this doesn't break tests anymore, we probably still want this,
> no point adding stuff to the pph image that is not needed...

Actually, the reverse is true.  We want to write out the IL exactly as 
the original parser emitted it.  There are things we decide not to write 
because they are better re-generated when the pph image is being read 
(e.g., function numbers, DECL_RTL), but

> Any idea why lto doesn't call lto_output_chain, but simply
> lto_output_tree to output the chains for struct/union?

LTO did not need those chains because once in the middle-end they are 
not used.  We are working at the parser level, so we need them.  Perhaps 
we won't need to write these chains, but first I'd like to understand why.

Since we are streaming the chains backwards without new breakage, let's 
leave it out for now.


Diego.

[Gabriel Charette, 2011-07-13 00:17:30]

On Tue, Jul 12, 2011 at 3:32 PM, Diego Novillo <dnovillo@google.com> wrote:
> On 11-07-12 16:43 , Gabriel Charette wrote:
>
>> Even if this doesn't break tests anymore, we probably still want this,
>> no point adding stuff to the pph image that is not needed...
>
> Actually, the reverse is true.  We want to write out the IL exactly as the
> original parser emitted it.  There are things we decide not to write because
> they are better re-generated when the pph image is being read (e.g.,
> function numbers, DECL_RTL), but
>

Well so lto_input_chain, called from pph_in_chain for every single
chain, already reconstructs the DECL_CHAIN on input (DECL_CHAIN is
actually set to NULL before streaming it out each element in the chain
anyways).

The only case where that wasn't true was when we output structs (and
unions I think: we need to add a test for unions), because structs
don't seem to use output chain (I don't have the code in front of me,
but I know they would only output the tree (i.e. the tree was, in that
case, responsible for streaming its DECL_CHAIN)).

I'm surprised it no longer breaks... I'll have a look when I come back
on Friday if it's still a debated issue then.

Gab