Re: Check canonical types in verify_type

[Richard Biener <rguenther at suse dot de>]

On Mon, 18 May 2015, Jan Hubicka wrote:

> > > +      if (!comp_type_attributes (t1, t2))
> > > +	return false;
> > > 
> > > Because I think the TBAA does not care about attribute lists.  I suppose this
> > > is kind-of harmless because of:
> > 
> > I think that was about attributes like 'packed', but those should have
> > their effects applied at stor-layout time.  So yes, I think this can be
> > dropped.
> 
> It does test the attribute list for functions only.  I think packed should
> be detected from memory layout, so I will test patch dropping this.
> > 
> > > +      /* For canonical type comparisons we do not want to build SCCs
> > > +	 so we cannot compare pointed-to types.  But we can, for now,
> > > +	 require the same pointed-to type kind and match what
> > > +	 useless_type_conversion_p would do.  */
> > > +      if (POINTER_TYPE_P (t1))
> > > +	{
> > > +	  if (TYPE_ADDR_SPACE (TREE_TYPE (t1))
> > > +	      != TYPE_ADDR_SPACE (TREE_TYPE (t2)))
> > > +	    return false;
> > > +
> > > +	  if (TREE_CODE (TREE_TYPE (t1)) != TREE_CODE (TREE_TYPE (t2)))
> > > +	    return false;
> > > +	}
> > > 
> > > Is the reason for not making differences between differnt pointer types
> > > really just lazyness to not deal with SCCs?  For that it is easy to add
> > > set of visited type pairs, just like odr_types_equivalent_p does.
> > 
> > Well - TBAA doesn't care about pointed-to types.  See get_alias_set
> > 
> >   else if (POINTER_TYPE_P (t)
> >            && t != ptr_type_node)
> >     set = get_alias_set (ptr_type_node);
> 
> Hmm, I see, interesting hack.  For the first part of comment, I see that
> qualifiers needs to be ignored, but I do not see why we put
> short * and int * pointers to same class.

For the reason that people are very lazy.  For example GCC has code
punning void ** and void * and void * to Type * (and vice versa).  People
just don't expect pointers to be in different alias sets (and there is
little gained with doing that).

> The complete/incomplete type fun with LTO can be solved for C++ but indeed I
> see why pointer to incomplete type needs to be considered compatible with every
> other pointer to structure.  Can't this be dealt with by adding correct edges
> into the aliasing dag?

I don't think so, without the dag degenerating.  We've had this mess
before (complete vs. incomplete structs and so).  It didn't work very 
well.

> > 
> > so the above would at most matter for pointer-type fields of aggregates.
> > And no, we don't want to deal with SCCs - I doubt it would bring any 
> > benefit and the issue here is that I am worried about SCCs being present
> > dependent on the order of streaming (well, the code pre-dates SCC
> > streaming).
> > 
> > > Because we now stream in SCC order anyway, most of time we won't even
> > > need to populate it.  Shall I implement this?
> > 
> > You can certainly try - but I think for cross-language interoperability
> > we want to treat
> > 
> >   struct X { void *p; }
> >   struct Y { void *p; }
> > 
> > and
> > 
> >   struct X { Y *p; }
> >   struct Y { X *q; } 
> > 
> > as compatible so I don't think we can use SCCs to partition alias sets
> > further (that is, we _do_ have to treat pointers conservatively).  The
> > current code is as far as we can get IMHO (well, even the current code
> > is too strict in making struct { void *p } and struct { int *p } not
> > have the same alias-set.
> 
> I will give it a try.  Do you have some examples where the above matters
> for cross-language TBAA?

I don't remember if I added a testcase, so no.  It's just very clear to me
that the look of pointer members may be very different (consider 
reference vs. pointer or pointer to array vs pointer to element).

> > > Other issue I run into is that for Ada bootstrap we have variadic type whose
> > > canonical types are having different temporary set as size.  I think this is
> > > valid and perhaps gimple_canonical_types_compatible_p should consider
> > > variadic arrays to be compatible with any array of compatible type?
> > 
> > Those are all local types and thus the strict equality compare should be
> > ok?  Not sure if we can do in C sth like
> > 
> > void foo (int n)
> > {
> >   struct { int i; int a[n]; } a;
> >   struct { int i; int a[n]; } b;
> > }
> > 
> > and if we'd have to assign the same alias-sets to 'a' and 'b'.
> 
> No idea here either. I wonder if the types are intedned to be TBAA compatible
> across two calls of the function.  In that case we may introduce multiple copies
> of body by early inlining already that may be a problem.

No idea.  Well, the canonical type machinery was supposed to be 
conservative but here we're obviously not 100% conservative.

> > > I am not quite convinced we get variadic types right at LTO time, because
> > > they bypass canonical type calculation anyway and their canonical type
> > > is set by TYPE_MAIN_VARIANT in lto_read_body_or_constructor which I think
> > > is not safe.  I will look for a testcase.
> > 
> > That is because if they are streamed locally they do not enter type
> > merging, but they still go via gimple_register_canonical_type, so I'm not
> > sure where you see they always get their main variant as canonical type.
> 
> I tought these are handled by:
>       /* And fixup types we streamed locally.  */
>         {
>           struct streamer_tree_cache_d *cache = data_in->reader_cache;
>           unsigned len = cache->nodes.length ();
>           unsigned i;
>           for (i = len; i-- > from;)
>             {
>               tree t = streamer_tree_cache_get_tree (cache, i);
>               if (t == NULL_TREE)
>                 continue;
> 
>               if (TYPE_P (t))
>                 {
>                   gcc_assert (TYPE_CANONICAL (t) == NULL_TREE);
>                   TYPE_CANONICAL (t) = TYPE_MAIN_VARIANT (t);
>                   if (TYPE_MAIN_VARIANT (t) != t)
>                     {
>                       gcc_assert (TYPE_NEXT_VARIANT (t) == NULL_TREE);
>                       TYPE_NEXT_VARIANT (t)
>                         = TYPE_NEXT_VARIANT (TYPE_MAIN_VARIANT (t));
>                       TYPE_NEXT_VARIANT (TYPE_MAIN_VARIANT (t)) = t;
>                     }
>                 }
>             }
>         }
> 
> which does not do registering.

That's your code, not mine - but yes, it looks like local types are
streamed when we stream the trees refering to them.  And the canonical
type hash is probably teared down at this point.  The code should
probably call lto_fixup_prevailing_type to also fix 
pointer-to/reference-to chains (and it will deal with main variants
as well).  IMHO the canonical type setting here is ok (during inlining
when we copy variable-length types we should end up sharing TYPE_CANONICAL
IMHO).  AFAICS we do, via copy_node which doesn't reset TYPE_CANONICAL.

Richard.

> > 
> > > I also added check that TYPE_CANONICAL agrees for type variants.  I think it
> > > should because few times in the middle-end uses TYPE_MAIN_VARIANT and seem to
> > > expect this to happen:
> > 
> > Yes.  Otherwise we'd run in circles for code that does
> > 
> >  type = TYPE_CANONICAL (TYPE_MAIN_VARIANT (type));
> > 
> > because the result might not be a main variant.  That's something to
> > verify (both TYPE_CANONICAL and TYPE_MAIN_VARIANT should form a tree). 
> > 
> > So here you'd verify that
> > 
> >     TYPE_MAIN_VARIANT (ct) == ct
> >     && TYPE_CANONICAL (TYPE_MAIN_VARIANT (t)) == ct
> 
> OK will do that incrementally and debug the issues. 
> I believe I tried the second test and it failed with Ada.
> > 
> > > +  /* Method and function types can not be used to address memory and thus
> > > +     TYPE_CANONICAL really matters only for determining useless conversions.
> > > +
> > > +     FIXME: C++ FE does not agree with gimple_canonical_types_compatible_p
> > > +     here.  gimple_canonical_types_compatible_p calls comp_type_attributes
> > > +     while for C++ FE the attributes does not make difference.  */
> > > +  else if (TREE_CODE (t) == FUNCTION_TYPE || TREE_CODE (t) == METHOD_TYPE)
> > > +    ;
> > > +  else if (t != ct && !gimple_canonical_types_compatible_p (t, ct, false)
> > > +	   /* FIXME: gimple_canonical_types_compatible_p can not compare types
> > > +	      with variably sized arrays because their sizes possibly
> > > +	      gimplified to different variables.  */
> > > +	   && !variably_modified_type_p (ct, NULL))
> > 
> > So with LTO this check should never trigger.  I'd probably check
> > !variably_modified_type_p (ct, NULL) first.
> 
> It does - we eventually stream in the bodies and the variably modified types
> and call verifier on that.
> 
> Thanks!
> Honza
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Dilip Upmanyu, Graham Norton, HRB 21284 (AG Nuernberg)