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.
