Re: TYPE_BINFO and canonical types at LTO

[Richard Biener <rguenther at suse dot de>]

On Sat, 15 Feb 2014, Jan Hubicka wrote:

> > > > This smells bad, since it is given a canonical type that is after the
> > > > structural equivalency merging that ignores BINFOs, so it may be completely
> > > > different class with completely different bases than the original.  Bases are
> > > > structuraly merged, too and may be exchanged for normal fields because
> > > > DECL_ARTIFICIAL (that separate bases and fields) does not seem to be part of
> > > > the canonical type definition in LTO.
> > > 
> > > Can you elaborate on that DECL_ARTIFICIAL thing?  That is, what is broken
> > > by considering all fields during that merging?
> > 
> > To make the code work with LTO, one can not merge 
> > struct B {struct A a}
> > struct B: A {}
> > 
> > these IMO differ only by DECL_ARTIFICIAL flag on the fields.
> > > 
> > > Note that the BINFO walk below only adds extra aliasing - it should
> > > be harmless correctness-wise, no?
> > 
> > If it is needed for the second case, then it we will produce wrong code with LTO
> > when we merge first and second case toghetr.  If it is not needed, then we are safe
> > but we don't really need the loop then.
> > 
> > Having the testcase where the BINFO walk is needed for corectness, I can turn
> > it into wrong code in LTO by interposing the class by structure.
> > 
> > I am rebuilding firefox with sanity checking that the second loop never adds anything
> > useful. Lets see.
> 
> The code really seems to be adding only cases of zero sized classes.

zero-size as in with no fields?  Then we can't do anything with such
classes and thus we don't need to record component aliases?

I'd say we try dropping this loop when stage1 opens again.

Richard.

>  I use the following hack in my tree. I do not know how to discover zero 
> sized class, so I test of unit size 1, but I think if there was other 
> cases, we would notice anyway.
> 
> The reason why I am looking into is that because I am trying to evaulate 
> how expensive BINFOs are. I am trying to keep only those that matters 
> for debug info and devirt, and avoid streaming others.
> 
> Honza
> 
> Index: alias.c
> ===================================================================
> --- alias.c	(revision 207777)
> +++ alias.c	(working copy)
> @@ -995,20 +996,40 @@ record_component_aliases (tree type)
>      case RECORD_TYPE:
>      case UNION_TYPE:
>      case QUAL_UNION_TYPE:
> -      /* Recursively record aliases for the base classes, if there are any.  */
> -      if (TYPE_BINFO (type))
> -	{
> -	  int i;
> -	  tree binfo, base_binfo;
> -
> -	  for (binfo = TYPE_BINFO (type), i = 0;
> -	       BINFO_BASE_ITERATE (binfo, i, base_binfo); i++)
> -	    record_alias_subset (superset,
> -				 get_alias_set (BINFO_TYPE (base_binfo)));
> -	}
> -      for (field = TYPE_FIELDS (type); field != 0; field = DECL_CHAIN (field))
> -	if (TREE_CODE (field) == FIELD_DECL && !DECL_NONADDRESSABLE_P (field))
> -	  record_alias_subset (superset, get_alias_set (TREE_TYPE (field)));
> +      {
> +#ifdef ENABLE_CHECKING
> +        bitmap_obstack my_obstack;
> +        bitmap_obstack_initialize (&my_obstack); 
> +	bitmap added = BITMAP_ALLOC (&my_obstack);
> +#endif
> +	alias_set_type t;
> +	for (field = TYPE_FIELDS (type); field != 0; field = DECL_CHAIN (field))
> +	  if (TREE_CODE (field) == FIELD_DECL && !DECL_NONADDRESSABLE_P (field))
> +	    {
> +	      t = get_alias_set (TREE_TYPE (field));
> +#ifdef ENABLE_CHECKING
> +	      bitmap_set_bit (added, t);
> +#endif
> +	      record_alias_subset (superset, t);
> +	    }
> +#ifdef ENABLE_CHECKING
> +	/* Recursively record aliases for the base classes, if there are any.  */
> +	if (!in_lto_p && TYPE_BINFO (type))
> +	  {
> +	    int i;
> +	    tree binfo, base_binfo;
> +
> +	    for (binfo = TYPE_BINFO (type), i = 0;
> +		 BINFO_BASE_ITERATE (binfo, i, base_binfo); i++)
> +		if (!bitmap_bit_p (added, get_alias_set (BINFO_TYPE (base_binfo))))
> +		{
> +		  gcc_assert (integer_onep (TYPE_SIZE_UNIT (BINFO_TYPE (base_binfo))));
> +		}
> +	  }
> +	BITMAP_FREE (added);
> +  bitmap_obstack_release (&my_obstack);
> +#endif
> +      }
>        break;
>  
>      case COMPLEX_TYPE: