On Wed, Apr 20, 2011 at 3:17 PM, Michael Matz <m...@suse.de> wrote:
> 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.
Thanks.
>> > + {
>> > + 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.
That sounds like a bug in type-merging. You don't happen to have
a small testcase? ;)
>> > + 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.
Ok.
>> > + 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.
But only in case the first one is already equal. What I wanted to say is
that we shouldn't have partially shared chains, so
if (TYPE_FIELDS (t) != TYPE_FIELDS (oldt))
for (...)
if (TREE_CODE (f1) == FIELD_DECL)
...
should be enough, no? In fact, why restrict fixing up the cache to
FIELD_DECLs and not also do it for TYPE_DECLs or FUNCTION_DECLs
that may reside in this chain?
>> > + && 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.
Ick, ok ... maybe amend the comment.
>> > + 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.
The lto testsuite harness doesn't support dg-error/warning, so there
are no testcases. There are testcases that ICEd (type verification)
before introducing these fixups though.
Richard.
>
> Ciao,
> Michael.
