Fix SPEC gcc micompile with LTO

Hi,
this patch fixes the miscompare I introduced to spec2006 GCC benchmark
when build with LTO.
The problem is that fld_incomplete_type_of builds new pointer type to
incomplete type rather than complete but it ends up giving wrong type
canonical.

This patch also improves TBAA with early opts because we do no lose info
by producing incomplete variants. 
Note that build_pointer_type may return existing type and in that case I
overwrite TYPE_CANONICAL of it, but I believe it should be harmless
because all pointers to a given type should have canonicals constructed
same way.

lto-bootstrapped/regtested x86_64-linux.

Honza
	* gcc.dg/lto/tbaa-1.c: New testcase.
	* tree.c (fld_incomplete_type_of): Copy TYPE_CANONICAL while creating
	pointer type.

On Mon, 5 Nov 2018, Jan Hubicka wrote:

> Hi,
> this patch fixes the miscompare I introduced to spec2006 GCC benchmark
> when build with LTO.
> The problem is that fld_incomplete_type_of builds new pointer type to
> incomplete type rather than complete but it ends up giving wrong type
> canonical.
> 
> This patch also improves TBAA with early opts because we do no lose info
> by producing incomplete variants. 
> Note that build_pointer_type may return existing type and in that case I
> overwrite TYPE_CANONICAL of it, but I believe it should be harmless
> because all pointers to a given type should have canonicals constructed
> same way.
> 
> lto-bootstrapped/regtested x86_64-linux.
> 
> Honza
> 	* gcc.dg/lto/tbaa-1.c: New testcase.
> 	* tree.c (fld_incomplete_type_of): Copy TYPE_CANONICAL while creating
> 	pointer type.
> Index: testsuite/gcc.dg/lto/tbaa-1.c
> ===================================================================
> --- testsuite/gcc.dg/lto/tbaa-1.c	(nonexistent)
> +++ testsuite/gcc.dg/lto/tbaa-1.c	(working copy)
> @@ -0,0 +1,41 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -flto -fdump-tree-evrp" } */
> +typedef struct rtx_def *rtx;
> +typedef struct cselib_val_struct
> +{
> +  union
> +  {
> +  } u;
> +  struct elt_loc_list *locs;
> +}
> +cselib_val;
> +struct elt_loc_list
> +{
> +  struct elt_loc_list *next;
> +  rtx loc;
> +};
> +static int n_useless_values;
> +unchain_one_elt_loc_list (pl)
> +     struct elt_loc_list **pl;
> +{
> +  struct elt_loc_list *l = *pl;
> +  *pl = l->next;
> +}
> +
> +discard_useless_locs (x, info)
> +     void **x;
> +{
> +  cselib_val *v = (cselib_val *) * x;
> +  struct elt_loc_list **p = &v->locs;
> +  int had_locs = v->locs != 0;
> +  while (*p)
> +    {
> +      unchain_one_elt_loc_list (p);
> +      p = &(*p)->next;
> +    }
> +  if (had_locs && v->locs == 0)
> +    {
> +      n_useless_values++;
> +    }
> +}
> +/* { dg-final { scan-tree-dump-times "n_useless_values" 2 "evrp" } } */                 
> Index: tree.c
> ===================================================================
> --- tree.c	(revision 265766)
> +++ tree.c	(working copy)
> @@ -5146,6 +5146,7 @@ fld_incomplete_type_of (tree t, struct f
>  	  else
>  	    first = build_reference_type_for_mode (t2, TYPE_MODE (t),
>  						TYPE_REF_CAN_ALIAS_ALL (t));
> +	  TYPE_CANONICAL (first) = TYPE_CANONICAL (TYPE_MAIN_VARIANT (t));

Hmm, this _should_ be a no-op.  Can you, before that line, add

  gcc_assert (TYPE_CANONICAL (t2) != t2
              && TYPE_CANONICAL (t2) == TYPE_CANONICAL (TREE_TYPE (t)));

?  That is, the incomplete variant should share TYPE_CANONICAL with
the pointed-to type and be _not_ the canonical leader (otherwise
all other pointer types are bogus).


>  	  add_tree_to_fld_list (first, fld);
>  	  return fld_type_variant (first, t, fld);
>  	}
> 
>

> Hmm, this _should_ be a no-op.  Can you, before that line, add
> 
>   gcc_assert (TYPE_CANONICAL (t2) != t2
>               && TYPE_CANONICAL (t2) == TYPE_CANONICAL (TREE_TYPE (t)));
> 
> ?  That is, the incomplete variant should share TYPE_CANONICAL with
> the pointed-to type and be _not_ the canonical leader (otherwise
> all other pointer types are bogus).

It looks like good idea.  I am re-checking with that change that already
found a bug - build_distinct_type_variant actually resets TYPE_CANONICAL
which I have missed earlier. So I am testing

Index: tree.c
===================================================================
--- tree.c	(revision 265807)
+++ tree.c	(working copy)
@@ -5146,6 +5146,9 @@ fld_incomplete_type_of (tree t, struct f
 	  else
 	    first = build_reference_type_for_mode (t2, TYPE_MODE (t),
 						TYPE_REF_CAN_ALIAS_ALL (t));
+	  gcc_assert (TYPE_CANONICAL (t2) != t2
+		      && TYPE_CANONICAL (t2) == TYPE_CANONICAL (TREE_TYPE (t)));
+	  TYPE_CANONICAL (first) = TYPE_CANONICAL (TYPE_MAIN_VARIANT (t));
 	  add_tree_to_fld_list (first, fld);
 	  return fld_type_variant (first, t, fld);
 	}
@@ -5169,6 +5172,7 @@ fld_incomplete_type_of (tree t, struct f
 	  SET_TYPE_MODE (copy, VOIDmode);
 	  SET_TYPE_ALIGN (copy, BITS_PER_UNIT);
 	  TYPE_SIZE_UNIT (copy) = NULL;
+	  TYPE_CANONICAL (copy) = t;
 	  if (AGGREGATE_TYPE_P (t))
 	    {
 	      TYPE_FIELDS (copy) = NULL;

Honza

On Mon, 5 Nov 2018, Jan Hubicka wrote:

> > Hmm, this _should_ be a no-op.  Can you, before that line, add
> > 
> >   gcc_assert (TYPE_CANONICAL (t2) != t2
> >               && TYPE_CANONICAL (t2) == TYPE_CANONICAL (TREE_TYPE (t)));
> > 
> > ?  That is, the incomplete variant should share TYPE_CANONICAL with
> > the pointed-to type and be _not_ the canonical leader (otherwise
> > all other pointer types are bogus).
> 
> It looks like good idea.  I am re-checking with that change that already
> found a bug - build_distinct_type_variant actually resets TYPE_CANONICAL
> which I have missed earlier. So I am testing
> 
> Index: tree.c
> ===================================================================
> --- tree.c	(revision 265807)
> +++ tree.c	(working copy)
> @@ -5146,6 +5146,9 @@ fld_incomplete_type_of (tree t, struct f
>  	  else
>  	    first = build_reference_type_for_mode (t2, TYPE_MODE (t),
>  						TYPE_REF_CAN_ALIAS_ALL (t));
> +	  gcc_assert (TYPE_CANONICAL (t2) != t2
> +		      && TYPE_CANONICAL (t2) == TYPE_CANONICAL (TREE_TYPE (t)));
> +	  TYPE_CANONICAL (first) = TYPE_CANONICAL (TYPE_MAIN_VARIANT (t));

as said the TYPE_CANONICAL assign should be already done exactly this
way in build_{poitner,reference}_for_mode.  So you should be able to
drop this from the patch.

>  	  add_tree_to_fld_list (first, fld);
>  	  return fld_type_variant (first, t, fld);
>  	}
> @@ -5169,6 +5172,7 @@ fld_incomplete_type_of (tree t, struct f
>  	  SET_TYPE_MODE (copy, VOIDmode);
>  	  SET_TYPE_ALIGN (copy, BITS_PER_UNIT);
>  	  TYPE_SIZE_UNIT (copy) = NULL;
> +	  TYPE_CANONICAL (copy) = t;

Or use build_variant_type_copy in the first place?  But you do not
seme to queue the new types in the variant list?

>  	  if (AGGREGATE_TYPE_P (t))
>  	    {
>  	      TYPE_FIELDS (copy) = NULL;
> 
> Honza
> 
>

> > +	  gcc_assert (TYPE_CANONICAL (t2) != t2
> > +		      && TYPE_CANONICAL (t2) == TYPE_CANONICAL (TREE_TYPE (t)));
> > +	  TYPE_CANONICAL (first) = TYPE_CANONICAL (TYPE_MAIN_VARIANT (t));
> 
> as said the TYPE_CANONICAL assign should be already done exactly this
> way in build_{poitner,reference}_for_mode.  So you should be able to
> drop this from the patch.

OK, I have turned this into a sanity check and re-testing.
> 
> >  	  add_tree_to_fld_list (first, fld);
> >  	  return fld_type_variant (first, t, fld);
> >  	}
> > @@ -5169,6 +5172,7 @@ fld_incomplete_type_of (tree t, struct f
> >  	  SET_TYPE_MODE (copy, VOIDmode);
> >  	  SET_TYPE_ALIGN (copy, BITS_PER_UNIT);
> >  	  TYPE_SIZE_UNIT (copy) = NULL;
> > +	  TYPE_CANONICAL (copy) = t;
> 
> Or use build_variant_type_copy in the first place?  But you do not
> seme to queue the new types in the variant list?

build_variant_type_copy would set TYPE_MAIN_VARAINT (copy) to be T.
I do not want this to happen since streaming would then pick complete
type as well.

Honza

Hi,
this is patch I ended up testing.  It ensures that canonical types of
copies I create are same as of originals C++ FE has its own refernece
type construction (cp_build_reference_type) and it creates additional
pointer types with TYPE_REF_IS_RVALUE set and it has different
TYPE_CANONICAL.

Obviously we do not see this in middle-end and we end up merging the
types despite fact they have different TYPE_CANONICAL.
I guess I can immitate the behaviour in fld_incomplete_type_of by 
implementing my own variant of build_pointer_type that also matches
TYPE_CANONICAL of the pointer it creates.  I wonder if there are better
solutions?

Honza

Index: tree.c
===================================================================
--- tree.c	(revision 265807)
+++ tree.c	(working copy)
@@ -5118,6 +5118,7 @@ fld_type_variant (tree first, tree t, st
   TYPE_ADDR_SPACE (v) = TYPE_ADDR_SPACE (t);
   TYPE_NAME (v) = TYPE_NAME (t);
   TYPE_ATTRIBUTES (v) = TYPE_ATTRIBUTES (t);
+  TYPE_CANONICAL (v) = TYPE_CANONICAL (t);
   add_tree_to_fld_list (v, fld);
   return v;
 }
@@ -5146,6 +5147,10 @@ fld_incomplete_type_of (tree t, struct f
 	  else
 	    first = build_reference_type_for_mode (t2, TYPE_MODE (t),
 						TYPE_REF_CAN_ALIAS_ALL (t));
+	  gcc_assert (TYPE_CANONICAL (t2) != t2
+		      && TYPE_CANONICAL (t2) == TYPE_CANONICAL (TREE_TYPE (t))
+		      && TYPE_CANONICAL (first)
+			 == TYPE_CANONICAL (TYPE_MAIN_VARIANT (t)));
 	  add_tree_to_fld_list (first, fld);
 	  return fld_type_variant (first, t, fld);
 	}
@@ -5169,6 +5174,7 @@ fld_incomplete_type_of (tree t, struct f
 	  SET_TYPE_MODE (copy, VOIDmode);
 	  SET_TYPE_ALIGN (copy, BITS_PER_UNIT);
 	  TYPE_SIZE_UNIT (copy) = NULL;
+	  TYPE_CANONICAL (copy) = TYPE_CANONICAL (t);
 	  if (AGGREGATE_TYPE_P (t))
 	    {
 	      TYPE_FIELDS (copy) = NULL;

> Hi,
> this is patch I ended up testing.  It ensures that canonical types of
> copies I create are same as of originals C++ FE has its own refernece
piece of mail got lost rendering the paragraph unreadable.  I wanted to say:

This is patch I ended up testing.  It ensures that canonical types of
copies I create are same as of originals.  It however ICEs building
auto-profile.c because C++ FE has its own reference  type construction
(cp_build_reference_type) and it creates additional pointer types with
TYPE_REF_IS_RVALUE set and it has different TYPE_CANONICAL.
> 
> Obviously we do not see this in middle-end and we end up merging the
> types despite fact they have different TYPE_CANONICAL.
> I guess I can immitate the behaviour in fld_incomplete_type_of by 
> implementing my own variant of build_pointer_type that also matches
> TYPE_CANONICAL of the pointer it creates.  I wonder if there are better
> solutions?
> 
> Honza
> 
> Index: tree.c
> ===================================================================
> --- tree.c	(revision 265807)
> +++ tree.c	(working copy)
> @@ -5118,6 +5118,7 @@ fld_type_variant (tree first, tree t, st
>    TYPE_ADDR_SPACE (v) = TYPE_ADDR_SPACE (t);
>    TYPE_NAME (v) = TYPE_NAME (t);
>    TYPE_ATTRIBUTES (v) = TYPE_ATTRIBUTES (t);
> +  TYPE_CANONICAL (v) = TYPE_CANONICAL (t);
>    add_tree_to_fld_list (v, fld);
>    return v;
>  }
> @@ -5146,6 +5147,10 @@ fld_incomplete_type_of (tree t, struct f
>  	  else
>  	    first = build_reference_type_for_mode (t2, TYPE_MODE (t),
>  						TYPE_REF_CAN_ALIAS_ALL (t));
> +	  gcc_assert (TYPE_CANONICAL (t2) != t2
> +		      && TYPE_CANONICAL (t2) == TYPE_CANONICAL (TREE_TYPE (t))
> +		      && TYPE_CANONICAL (first)
> +			 == TYPE_CANONICAL (TYPE_MAIN_VARIANT (t)));
>  	  add_tree_to_fld_list (first, fld);
>  	  return fld_type_variant (first, t, fld);
>  	}
> @@ -5169,6 +5174,7 @@ fld_incomplete_type_of (tree t, struct f
>  	  SET_TYPE_MODE (copy, VOIDmode);
>  	  SET_TYPE_ALIGN (copy, BITS_PER_UNIT);
>  	  TYPE_SIZE_UNIT (copy) = NULL;
> +	  TYPE_CANONICAL (copy) = TYPE_CANONICAL (t);
>  	  if (AGGREGATE_TYPE_P (t))
>  	    {
>  	      TYPE_FIELDS (copy) = NULL;

On November 5, 2018 5:11:09 PM GMT+01:00, Jan Hubicka <hubicka@ucw.cz> wrote:
>> Hi,
>> this is patch I ended up testing.  It ensures that canonical types of
>> copies I create are same as of originals C++ FE has its own refernece
>piece of mail got lost rendering the paragraph unreadable.  I wanted to
>say:
>
>This is patch I ended up testing.  It ensures that canonical types of
>copies I create are same as of originals.  It however ICEs building
>auto-profile.c because C++ FE has its own reference  type construction
>(cp_build_reference_type) and it creates additional pointer types with
>TYPE_REF_IS_RVALUE set and it has different TYPE_CANONICAL.

Hmm. I guess we need to fix that, otherwise alias will be broken (or you need to resort to a FE specific routine for pointer building). 

Richard. 

>> Obviously we do not see this in middle-end and we end up merging the
>> types despite fact they have different TYPE_CANONICAL.
>> I guess I can immitate the behaviour in fld_incomplete_type_of by 
>> implementing my own variant of build_pointer_type that also matches
>> TYPE_CANONICAL of the pointer it creates.  I wonder if there are
>better
>> solutions?
>> 
>> Honza
>> 
>> Index: tree.c
>> ===================================================================
>> --- tree.c	(revision 265807)
>> +++ tree.c	(working copy)
>> @@ -5118,6 +5118,7 @@ fld_type_variant (tree first, tree t, st
>>    TYPE_ADDR_SPACE (v) = TYPE_ADDR_SPACE (t);
>>    TYPE_NAME (v) = TYPE_NAME (t);
>>    TYPE_ATTRIBUTES (v) = TYPE_ATTRIBUTES (t);
>> +  TYPE_CANONICAL (v) = TYPE_CANONICAL (t);
>>    add_tree_to_fld_list (v, fld);
>>    return v;
>>  }
>> @@ -5146,6 +5147,10 @@ fld_incomplete_type_of (tree t, struct f
>>  	  else
>>  	    first = build_reference_type_for_mode (t2, TYPE_MODE (t),
>>  						TYPE_REF_CAN_ALIAS_ALL (t));
>> +	  gcc_assert (TYPE_CANONICAL (t2) != t2
>> +		      && TYPE_CANONICAL (t2) == TYPE_CANONICAL (TREE_TYPE (t))
>> +		      && TYPE_CANONICAL (first)
>> +			 == TYPE_CANONICAL (TYPE_MAIN_VARIANT (t)));
>>  	  add_tree_to_fld_list (first, fld);
>>  	  return fld_type_variant (first, t, fld);
>>  	}
>> @@ -5169,6 +5174,7 @@ fld_incomplete_type_of (tree t, struct f
>>  	  SET_TYPE_MODE (copy, VOIDmode);
>>  	  SET_TYPE_ALIGN (copy, BITS_PER_UNIT);
>>  	  TYPE_SIZE_UNIT (copy) = NULL;
>> +	  TYPE_CANONICAL (copy) = TYPE_CANONICAL (t);
>>  	  if (AGGREGATE_TYPE_P (t))
>>  	    {
>>  	      TYPE_FIELDS (copy) = NULL;

Hi,
this is updated version of the patch. As discussed on IRC, C++ has two
different types of references (rvalue and normal). They have different
canonical type, but this does not affect outcome of get_alias_set
because it rebuilds the pointer/reference types from scratch.

The effect of this patch is to turn both into one type when pointer is
reuilt that should be safe. So I simply relaxed the sanity check that
type canonicals needs to be same for pointers.

lto-bootstrapped/regtested x86_64-linux, plan to commit it shortly.

Honza

	* gcc.dg/lto/tbaa-1.c: New testcase.

	* tree.c (fld_type_variant): Copy canonical type.
	(fld_incomplete_type_of): Check that canonical types looks sane;
	copy canonical type.
	(verify_type): Accept when incomplete type has complete canonical type.
Index: testsuite/gcc.dg/lto/tbaa-1.c
===================================================================
--- testsuite/gcc.dg/lto/tbaa-1.c	(nonexistent)
+++ testsuite/gcc.dg/lto/tbaa-1.c	(working copy)
@@ -0,0 +1,41 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -flto -fdump-tree-evrp" } */
+typedef struct rtx_def *rtx;
+typedef struct cselib_val_struct
+{
+  union
+  {
+  } u;
+  struct elt_loc_list *locs;
+}
+cselib_val;
+struct elt_loc_list
+{
+  struct elt_loc_list *next;
+  rtx loc;
+};
+static int n_useless_values;
+unchain_one_elt_loc_list (pl)
+     struct elt_loc_list **pl;
+{
+  struct elt_loc_list *l = *pl;
+  *pl = l->next;
+}
+
+discard_useless_locs (x, info)
+     void **x;
+{
+  cselib_val *v = (cselib_val *) * x;
+  struct elt_loc_list **p = &v->locs;
+  int had_locs = v->locs != 0;
+  while (*p)
+    {
+      unchain_one_elt_loc_list (p);
+      p = &(*p)->next;
+    }
+  if (had_locs && v->locs == 0)
+    {
+      n_useless_values++;
+    }
+}
+/* { dg-final { scan-tree-dump-times "n_useless_values" 2 "evrp" } } */                 
Index: tree.c
===================================================================
--- tree.c	(revision 265807)
+++ tree.c	(working copy)
@@ -5118,6 +5118,7 @@ fld_type_variant (tree first, tree t, st
   TYPE_ADDR_SPACE (v) = TYPE_ADDR_SPACE (t);
   TYPE_NAME (v) = TYPE_NAME (t);
   TYPE_ATTRIBUTES (v) = TYPE_ATTRIBUTES (t);
+  TYPE_CANONICAL (v) = TYPE_CANONICAL (t);
   add_tree_to_fld_list (v, fld);
   return v;
 }
@@ -5146,6 +5147,8 @@ fld_incomplete_type_of (tree t, struct f
 	  else
 	    first = build_reference_type_for_mode (t2, TYPE_MODE (t),
 						TYPE_REF_CAN_ALIAS_ALL (t));
+	  gcc_assert (TYPE_CANONICAL (t2) != t2
+		      && TYPE_CANONICAL (t2) == TYPE_CANONICAL (TREE_TYPE (t)));
 	  add_tree_to_fld_list (first, fld);
 	  return fld_type_variant (first, t, fld);
 	}
@@ -5169,6 +5174,7 @@ fld_incomplete_type_of (tree t, struct f
 	  SET_TYPE_MODE (copy, VOIDmode);
 	  SET_TYPE_ALIGN (copy, BITS_PER_UNIT);
 	  TYPE_SIZE_UNIT (copy) = NULL;
+	  TYPE_CANONICAL (copy) = TYPE_CANONICAL (t);
 	  if (AGGREGATE_TYPE_P (t))
 	    {
 	      TYPE_FIELDS (copy) = NULL;
@@ -13880,7 +13893,8 @@ verify_type (const_tree t)
 	      with variably sized arrays because their sizes possibly
 	      gimplified to different variables.  */
 	   && !variably_modified_type_p (ct, NULL)
-	   && !gimple_canonical_types_compatible_p (t, ct, false))
+	   && !gimple_canonical_types_compatible_p (t, ct, false)
+	   && COMPLETE_TYPE_P (t))
     {
       error ("TYPE_CANONICAL is not compatible");
       debug_tree (ct);