Re: Check canonical types in verify_type

[Jan Hubicka <hubicka@ucw.cz>]

> > +      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.

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?
> 
> 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?
> 
> > 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.
> 
> > 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.
> 
> > 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