This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: better wpa [1/n]: merge types during read-in


Hi,

On Wed, 20 Apr 2011, Richard Guenther wrote:

> > + /* A hashtable of trees that potentially refer to variables or functions
> > + Â Âthat must be replaced with their prevailing variant. Â*/
> > + static GTY((if_marked ("ggc_marked_p"), param_is (union tree_node))) htab_t
> > + Â tree_with_vars;
> > + /* A hashtable of top-level trees that can potentially be merged with trees
> > + Â Âfrom other compilation units. Â*/
> 
> It would have been nice to have the top-level tree merging as a separate
> patch, as I am not convinced it is correct, but see below ...

I'll split it out.

> > + Â Â Â {
> > + Â Â Â Â tree *t2 = (tree *) htab_find_slot (top_decls, t, INSERT);
> > + Â Â Â Â if (*t2)
> > + Â Â Â Â Â t = *t2;
> > + Â Â Â Â else
> > + Â Â Â Â Â *t2 = t;
> 
> I don't think we can share TREE_LISTs.  Where do they hang off
> when you do this?  Are not all but one of those dead?  All
> TREE_LIST manipulation routines I know of do not even think about
> the possibility of shared lists.

Well, it clearly works for the use in global trees :-)  The problem I 
solved with this is, that all trees are stored in the reader cache.  If 
(say) two function types, or enum types are merged, one of their 
TYPE_ARG_TYPES or TYPE_VALUES lists should be dead.  But as they're still 
referred from the cache they aren't collected.  I could remove them from 
the cache in a similar way to how I deal with the FIELD_DECLs.  Or I could 
store tree_list nodes not in the cache at all.  The latter seems more 
worthwhile to me, so I'm going to try this.

> > + /* Fix up fields of a field_decl T. Â*/
> > +
> > + static void
> > + lto_ft_field_decl (tree t)
> > + {
> > + Â lto_ft_decl_common (t);
> > + Â LTO_FIXUP_TYPE (DECL_FIELD_OFFSET (t));
> > + Â LTO_FIXUP_TYPE (DECL_BIT_FIELD_TYPE (t));
> > + Â LTO_FIXUP_TYPE (DECL_QUALIFIER (t));
> 
> here (and earlier) we had some no_fixup_p asserts.  What happened to
> them?

no_fixup_p checked only that the tree wasn't a type or a 
var/function_decl.

> In particular, don't we need to fixup the DECL_FIELD_BIT_OFFSET 
> integer-cst?

But I was indeed too eager to remove those lines, I should still deal with 
DECL_SECTION_NAME, DECL_FIELD_BIT_OFFSET and BINFO_OFFSET.  Thanks for 
catching this.

> > +
> > + Â Â Â /* First fixup the fields of T. Â*/
> > + Â Â Â     lto_fixup_types (t);
> > +
> > + Â Â Â /* Now try to find a canonical variant of T itself. Â*/
> > + Â Â Â if (TYPE_P (t))
> > + Â Â Â {
> 
> If t is a type, why fix up its field if it may not be the canonical variant?

Because type merging to work sometimes requires already canonicalized 
fields, at least that's what I found in investigating why some types 
weren't merged that should have been.  Hence I'm first canonicalizing all 
fields of everything and then see if something merged.

> > + Â Â Â Â if (t == oldt
> > + Â Â Â Â Â Â && TYPE_MAIN_VARIANT (t) != t)
> > + Â Â Â Â Â {
> > + Â Â Â Â Â Â /* If this is its own type, link it into the variant
> > + Â Â Â Â Â Â Â Âchain. Â*/
> > + Â Â Â Â Â Â TYPE_NEXT_VARIANT (t) = TYPE_NEXT_VARIANT (TYPE_MAIN_VARIANT (t));
> > + Â Â Â Â Â Â TYPE_NEXT_VARIANT (TYPE_MAIN_VARIANT (t)) = t;
> 
> Hmm - isn't this taken care of in lto_fixup_types?

Nope.  I've taken it out of the (old) lto_fixup_type, because (now that 
I've removed the seen pointer_set) it can be called for the same type 
multiple times, which would lead to it being included multiple times in 
the NEXT_VARIANT chain.  I found it more clear to do this kind of list 
manipulation at the place where we're sure to see every type only once.

> > + Â Â Â Â Â Â for (f1 = TYPE_FIELDS (t), f2 = TYPE_FIELDS (oldt);
> > + Â Â Â Â Â Â Â Â Âf1 && f2; f1 = TREE_CHAIN (f1), f2 = TREE_CHAIN (f2))
> > + Â Â Â Â Â Â Â if (TREE_CODE (f1) == FIELD_DECL
> > + Â Â Â Â Â Â Â Â Â && f1 != f2
> 
> This should always be true unless TYPE_FIELDS (t) == TYPE_FIELDS (oldt)

Think shared field_decl chains.  I'll have fixed up the chain for one of 
the type pairs already and can later come to a type referring exactly the 
same field_decls again.

> > + Â Â Â Â Â Â Â Â Â && lto_streamer_cache_lookup (cache, f2, &ix))
> 
> This lookup should always succeed, no?

Yes.  I'll assert this.

> > + Â Â Â Â Â Â Â Â {
> > + Â Â Â Â Â Â Â Â Â gcc_assert (DECL_NAME (f1) == DECL_NAME (f2));
> > + Â Â Â Â Â Â Â Â Â /* If we're going to replace an element which we'd
> > + Â Â Â Â Â Â Â Â Â Â Âstill visit in the next iterations, we wouldn't
> > + Â Â Â Â Â Â Â Â Â Â Âhandle it, so do it here. Â*/
> > + Â Â Â Â Â Â Â Â Â if (ix < i)
> > + Â Â Â Â Â Â Â Â Â Â lto_fixup_types (f2);
> 
> ?  But it's dead, no?

That is true, but it can refer to integer_cst which I can only fix up by 
walking the fields.  If I don't do that, then even though the field_decl 
will not be in the cache anymore, its integer_cst (and their non-fixed up 
types) will stay uncollected.

> > + Â Â Â Â Â Â Â Â Â lto_streamer_cache_insert_at (cache, f1, ix);
> > + Â Â Â Â Â Â Â Â }
> 
> Btw, nice.  Does this get rid of the need for the field-decl fixup code
> in input_gimple_stmt?

Hmm, it's gross but seems to me still required for the diagnostic and to 
emit the VIEW_CONVERT_EXPR, at least for invalid input code.  OTOH if the 
streamed out code ensures that a field_decl in a component_ref always is 
included in its DECL_CONTEXT, then the new merging should indeed make sure 
that this also holds after streaming in.  Do we have testcases 
specifically trying this code?  greping for "mismatching" in 
testsuite/ doesn't show anything relevant.


Ciao,
Michael.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]