On Mon, Oct 8, 2012 at 11:18 AM, Jan Hubicka <hubi...@ucw.cz> wrote:
>> On Sun, Oct 7, 2012 at 7:22 PM, Jan Hubicka <hubi...@ucw.cz> wrote:
>> >> On Sun, Oct 7, 2012 at 5:15 PM, Jan Hubicka <hubi...@ucw.cz> wrote:
>> >> > Hi,
>> >> > I added a santy check that after fixup all types that lost in the
>> >> > merging are
>> >> > really dead. And it turns out we have some zombies around.
>> >> >
>> >> > INTEGER_CST needs special care because it is special cased by the
>> >> > streamer. We also
>> >> > do not want to do inplace modificaitons on it because that would
>> >> > corrupt the hashtable
>> >> > used by tree.c's sharing code
>> >> >
>> >> > Bootstrapped/regtested x86_64-linux, OK?
>> >>
>> >> No, I don't think we want to fixup INTEGER_CSTs this way. Instead we
>> >> want to fixup
>> >> them where they end up used unfixed.
>> >
>> > Erm, I think it is what the patch does?
>>
>> Ah, indeed.
>>
>> > It replaces pointers to integer_cst with type that did not survive by
>> > pointer
>> > to new integer_cst. (with the optimization that INTEGER_CST with overflow
>> > is changed in place because it is allowed to do so).
>>
>> Btw ...
>>
>> >> > @@ -1526,6 +1549,11 @@ lto_ft_type (tree t)
>> >> > LTO_FIXUP_TREE (t->type_non_common.binfo);
>> >> >
>> >> > LTO_FIXUP_TREE (TYPE_CONTEXT (t));
>> >> > +
>> >> > + if (TREE_CODE (t) == METHOD_TYPE)
>> >> > + TYPE_METHOD_BASETYPE (t);
>> >> > + if (TREE_CODE (t) == OFFSET_TYPE)
>> >> > + TYPE_OFFSET_BASETYPE (t);
>>
>> that looks like a no-op to me ... (both are TYPE_MAXVAL which
>> is already fixed up).
>
> Ah, indeed. They were result of experimenting with the stale pointers to the
> obsoletted types and field decls. I now understand where they come from. The
> reason is twofold.
>
> 1) after merging records we replace field decls in the cache
> by new ones. This however does not mean that they die, because
> the existing pointers to them are not replaced.
> I have WIP patch for that that however require one extra pass
> over the list of all trees.
Yes, I think this is also why we do
/* ??? Not sure the above is all relevant in this
path canonicalizing TYPE_FIELDS to that of the
main variant. */
if (ix < i)
lto_fixup_types (f2);
streamer_tree_cache_insert_at (cache, f1, ix);
something I dislike as well and something we should try to address in a
more formal way.
> 2) As we query the type_hash while we are rewritting the types,
> we run into instability of the hashtable. This manifests itself
> as an ICE when one adds sanity check that while merging function
> types their arg types are equivalent, too.
> This ICEs compiling i.e. sqlite but I did not really managed to
> reduce this. I tracked it down to the argument type being inserted
> into gimple_type_hash but at the time we query the new argument type,
> the original is no longer found despite their hashes are equivalent.
> The problem is hidden when things fit into the leader cache,
> so one needs rather big testcase.
Ugh. For reduction you can disable those caches though. The above
means there is a disconnect between hashing and comparing.
Maybe it's something weird with the early out
if (TYPE_ARG_TYPES (t1) == TYPE_ARG_TYPES (t2))
goto same_types;
?
> So I tried to register all gimple types first. Use TREE_VISITED within
> the merging code to mark that type is not a leader and then TREE_CHAIN
> to point to the leader. This avoids need to re-query the hashtable
> from the later fixups. We only look for types with TREEE_VISITED
> and replace them by TREE_CHAIN.
TREE_CHAIN is unused for types? But we probably shouldn't add a new
use ...
> This has two passes. First we compute the main variants and mark
> field_decls and type_decls for merging and in last pass we finally do
> fixup on what remained in the table.
>
> This allows me to poison pointers in the removed types in a way
> so the GGC would ICE if they stayed reachable.
> I however need the extra pass because
> 1) I can not update the type_decls/field_decls while registering
> types or I run into the hash table problems
> 2) I can not merge the second two passes because at the time
> I find type/field decls equialent there may be earlier pointers
> to them.
You need to "merge" all trees reachable from the one you start at once
(what I'm working on from time to time - work per tree "SCC", in a DFS
walk).
Richard.
> Honza
