[PATCH] lto: Fix up lto_fixup_prevailing_type [PR108910]

[PATCH] lto: Fix up lto_fixup_prevailing_type [PR108910]

[Jakub Jelinek]

Hi!

Without LTO, TYPE_POINTER_TO/TYPE_REFERENCE_TO chains are only maintained
inside of build_{pointer,reference}_type_for_mode and those routines
ensure that the pointer/reference type added to the chain is really
unqualified (including address space), without extra user alignment
and has just one entry for each of the TYPE_MODE/TYPE_REF_CAN_ALIAS_ALL
pair (unless something would modify the types in place, but that would
be wrong).

Now, LTO adds stuff to these chains in lto_fixup_prevailing_type but
doesn't guarantee that.  The testcase in the PR (which I'm not including
for testsuite because when (I hope) the aarch64 backend bug will be fixed,
the testcase would work either way) shows a case where user has
TYPE_USER_ALIGN type with very high alignment, as there aren't enough
pointers to float in the code left that one becomes the prevailing one,
lto_fixup_prevailing_type puts it into the TYPE_POINTER_TO chain of
float and later on during expansion of __builtin_cexpif expander
uses build_pointer_type (float_type_node) to emit a sincosf call and
instead of getting a normal pointer type gets this non-standard one.

The following patch fixes that by not adding into those chains
qualified or user aligned types and by making sure that some type
for the TYPE_MODE/TYPE_REF_CAN_ALIAS_ALL combination (e.g. from lto1
initialization) isn't there already before adding a new one.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2023-02-28  Jakub Jelinek  <jakub@redhat.com>

	PR target/108910
	* lto-common.cc (lto_fixup_prevailing_type): Don't add t to
	TYPE_POINTER_TO or TYPE_REFERENCE_TO chain if it has non-zero
	TYPE_QUALS, or TYPE_USER_ALIGN or some other type with the
	same TYPE_MODE and TYPE_REF_CAN_ALIAS_ALL flag is already
	present.

--- gcc/lto/lto-common.cc.jj	2023-01-16 11:52:16.165732856 +0100
+++ gcc/lto/lto-common.cc	2023-02-28 01:42:51.006764018 +0100
@@ -984,21 +984,35 @@ lto_fixup_prevailing_type (tree t)
       TYPE_NEXT_VARIANT (t) = TYPE_NEXT_VARIANT (mv);
       TYPE_NEXT_VARIANT (mv) = t;
     }
-
-  /* The following reconstructs the pointer chains
-     of the new pointed-to type if we are a main variant.  We do
-     not stream those so they are broken before fixup.  */
-  if (TREE_CODE (t) == POINTER_TYPE
-      && TYPE_MAIN_VARIANT (t) == t)
-    {
-      TYPE_NEXT_PTR_TO (t) = TYPE_POINTER_TO (TREE_TYPE (t));
-      TYPE_POINTER_TO (TREE_TYPE (t)) = t;
-    }
-  else if (TREE_CODE (t) == REFERENCE_TYPE
-	   && TYPE_MAIN_VARIANT (t) == t)
+  else if (!TYPE_QUALS (t) && !TYPE_USER_ALIGN (t))
     {
-      TYPE_NEXT_REF_TO (t) = TYPE_REFERENCE_TO (TREE_TYPE (t));
-      TYPE_REFERENCE_TO (TREE_TYPE (t)) = t;
+      /* The following reconstructs the pointer chains
+	 of the new pointed-to type if we are a main variant.  We do
+	 not stream those so they are broken before fixup.
+	 Don't add it if despite being main variant it is
+	 qualified or user aligned type.  Don't add it if there
+	 is something in the chain already.  */
+      tree *p = NULL;
+      if (TREE_CODE (t) == POINTER_TYPE)
+	p = &TYPE_POINTER_TO (TREE_TYPE (t));
+      else if (TREE_CODE (t) == REFERENCE_TYPE)
+	p = &TYPE_REFERENCE_TO (TREE_TYPE (t));
+      if (p)
+	{
+	  tree t2;
+	  for (t2 = *p; t2; t2 = TYPE_NEXT_PTR_TO (t2))
+	    if (TYPE_MODE (t2) == TYPE_MODE (t)
+		&& TYPE_REF_CAN_ALIAS_ALL (t2) == TYPE_REF_CAN_ALIAS_ALL (t))
+	      break;
+	  if (t2 == NULL_TREE)
+	    {
+	      if (TREE_CODE (t) == POINTER_TYPE)
+		TYPE_NEXT_PTR_TO (t) = *p;
+	      else
+		TYPE_NEXT_REF_TO (t) = *p;
+	      *p = t;
+	    }
+	}
     }
 }
 

	Jakub

Re: [PATCH] lto: Fix up lto_fixup_prevailing_type [PR108910]

[Richard Biener]

On Tue, 28 Feb 2023, Jakub Jelinek wrote:

> Hi!
> 
> Without LTO, TYPE_POINTER_TO/TYPE_REFERENCE_TO chains are only maintained
> inside of build_{pointer,reference}_type_for_mode and those routines
> ensure that the pointer/reference type added to the chain is really
> unqualified (including address space), without extra user alignment
> and has just one entry for each of the TYPE_MODE/TYPE_REF_CAN_ALIAS_ALL
> pair (unless something would modify the types in place, but that would
> be wrong).

Is that so?  I can't find any code verifying that (verify_type?).
Of course build_{pointer,reference}_type_for_mode will always build
unqualified pointers, but then the LTO code does

  /* The following reconstructs the pointer chains
     of the new pointed-to type if we are a main variant.  We do
     not stream those so they are broken before fixup.  */
  if (TREE_CODE (t) == POINTER_TYPE
      && TYPE_MAIN_VARIANT (t) == t)
    {
      TYPE_NEXT_PTR_TO (t) = TYPE_POINTER_TO (TREE_TYPE (t));
      TYPE_POINTER_TO (TREE_TYPE (t)) = t;
    }
  else if (TREE_CODE (t) == REFERENCE_TYPE
           && TYPE_MAIN_VARIANT (t) == t)
    {
      TYPE_NEXT_REF_TO (t) = TYPE_REFERENCE_TO (TREE_TYPE (t));
      TYPE_REFERENCE_TO (TREE_TYPE (t)) = t;
    }

which was supposed to ensure only putting unqualified pointers
(not pointed to types!) to the chain.  So to me the question is
rather why a type with TYPE_USER_ALIGN is a the main variant - that's
what looks wrong here?

Richard.

> Now, LTO adds stuff to these chains in lto_fixup_prevailing_type but
> doesn't guarantee that.  The testcase in the PR (which I'm not including
> for testsuite because when (I hope) the aarch64 backend bug will be fixed,
> the testcase would work either way) shows a case where user has
> TYPE_USER_ALIGN type with very high alignment, as there aren't enough
> pointers to float in the code left that one becomes the prevailing one,
> lto_fixup_prevailing_type puts it into the TYPE_POINTER_TO chain of
> float and later on during expansion of __builtin_cexpif expander
> uses build_pointer_type (float_type_node) to emit a sincosf call and
> instead of getting a normal pointer type gets this non-standard one.
> 
> The following patch fixes that by not adding into those chains
> qualified or user aligned types and by making sure that some type
> for the TYPE_MODE/TYPE_REF_CAN_ALIAS_ALL combination (e.g. from lto1
> initialization) isn't there already before adding a new one.
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> 
> 2023-02-28  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR target/108910
> 	* lto-common.cc (lto_fixup_prevailing_type): Don't add t to
> 	TYPE_POINTER_TO or TYPE_REFERENCE_TO chain if it has non-zero
> 	TYPE_QUALS, or TYPE_USER_ALIGN or some other type with the
> 	same TYPE_MODE and TYPE_REF_CAN_ALIAS_ALL flag is already
> 	present.
> 
> --- gcc/lto/lto-common.cc.jj	2023-01-16 11:52:16.165732856 +0100
> +++ gcc/lto/lto-common.cc	2023-02-28 01:42:51.006764018 +0100
> @@ -984,21 +984,35 @@ lto_fixup_prevailing_type (tree t)
>        TYPE_NEXT_VARIANT (t) = TYPE_NEXT_VARIANT (mv);
>        TYPE_NEXT_VARIANT (mv) = t;
>      }
> -
> -  /* The following reconstructs the pointer chains
> -     of the new pointed-to type if we are a main variant.  We do
> -     not stream those so they are broken before fixup.  */
> -  if (TREE_CODE (t) == POINTER_TYPE
> -      && TYPE_MAIN_VARIANT (t) == t)
> -    {
> -      TYPE_NEXT_PTR_TO (t) = TYPE_POINTER_TO (TREE_TYPE (t));
> -      TYPE_POINTER_TO (TREE_TYPE (t)) = t;
> -    }
> -  else if (TREE_CODE (t) == REFERENCE_TYPE
> -	   && TYPE_MAIN_VARIANT (t) == t)
> +  else if (!TYPE_QUALS (t) && !TYPE_USER_ALIGN (t))
>      {
> -      TYPE_NEXT_REF_TO (t) = TYPE_REFERENCE_TO (TREE_TYPE (t));
> -      TYPE_REFERENCE_TO (TREE_TYPE (t)) = t;
> +      /* The following reconstructs the pointer chains
> +	 of the new pointed-to type if we are a main variant.  We do
> +	 not stream those so they are broken before fixup.
> +	 Don't add it if despite being main variant it is
> +	 qualified or user aligned type.  Don't add it if there
> +	 is something in the chain already.  */
> +      tree *p = NULL;
> +      if (TREE_CODE (t) == POINTER_TYPE)
> +	p = &TYPE_POINTER_TO (TREE_TYPE (t));
> +      else if (TREE_CODE (t) == REFERENCE_TYPE)
> +	p = &TYPE_REFERENCE_TO (TREE_TYPE (t));
> +      if (p)
> +	{
> +	  tree t2;
> +	  for (t2 = *p; t2; t2 = TYPE_NEXT_PTR_TO (t2))
> +	    if (TYPE_MODE (t2) == TYPE_MODE (t)
> +		&& TYPE_REF_CAN_ALIAS_ALL (t2) == TYPE_REF_CAN_ALIAS_ALL (t))
> +	      break;
> +	  if (t2 == NULL_TREE)
> +	    {
> +	      if (TREE_CODE (t) == POINTER_TYPE)
> +		TYPE_NEXT_PTR_TO (t) = *p;
> +	      else
> +		TYPE_NEXT_REF_TO (t) = *p;
> +	      *p = t;
> +	    }
> +	}
>      }
>  }
>  
> 
> 	Jakub
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg,
Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman;
HRB 36809 (AG Nuernberg)

Re: [PATCH] lto: Fix up lto_fixup_prevailing_type [PR108910]

[Jakub Jelinek]

On Tue, Feb 28, 2023 at 08:58:20AM +0000, Richard Biener wrote:
> > Without LTO, TYPE_POINTER_TO/TYPE_REFERENCE_TO chains are only maintained
> > inside of build_{pointer,reference}_type_for_mode and those routines
> > ensure that the pointer/reference type added to the chain is really
> > unqualified (including address space), without extra user alignment
> > and has just one entry for each of the TYPE_MODE/TYPE_REF_CAN_ALIAS_ALL
> > pair (unless something would modify the types in place, but that would
> > be wrong).
> 
> Is that so?  I can't find any code verifying that (verify_type?).
> Of course build_{pointer,reference}_type_for_mode will always build
> unqualified pointers, but then the LTO code does
> 
>   /* The following reconstructs the pointer chains
>      of the new pointed-to type if we are a main variant.  We do
>      not stream those so they are broken before fixup.  */
>   if (TREE_CODE (t) == POINTER_TYPE
>       && TYPE_MAIN_VARIANT (t) == t)
>     {
>       TYPE_NEXT_PTR_TO (t) = TYPE_POINTER_TO (TREE_TYPE (t));
>       TYPE_POINTER_TO (TREE_TYPE (t)) = t;
>     }
>   else if (TREE_CODE (t) == REFERENCE_TYPE
>            && TYPE_MAIN_VARIANT (t) == t)
>     {
>       TYPE_NEXT_REF_TO (t) = TYPE_REFERENCE_TO (TREE_TYPE (t));
>       TYPE_REFERENCE_TO (TREE_TYPE (t)) = t;
>     }
> 
> which was supposed to ensure only putting unqualified pointers
> (not pointed to types!) to the chain.  So to me the question is
> rather why a type with TYPE_USER_ALIGN is a the main variant - that's
> what looks wrong here?

First of all, it is unclear how the above ensures that type isn't
added to those chains even when it already is there (or some similar type).
Say lto1 during initialization needs build_pointer_type (float_type_node)
and later on lto_fixup_prevailing_type is called on some float *.

I'll try to debug why those user aligned types become TYPE_MAIN_VARIANT
though...

	Jakub

Re: [PATCH] lto: Fix up lto_fixup_prevailing_type [PR108910]

[Richard Biener]

On Tue, 28 Feb 2023, Jakub Jelinek wrote:

> On Tue, Feb 28, 2023 at 08:58:20AM +0000, Richard Biener wrote:
> > > Without LTO, TYPE_POINTER_TO/TYPE_REFERENCE_TO chains are only maintained
> > > inside of build_{pointer,reference}_type_for_mode and those routines
> > > ensure that the pointer/reference type added to the chain is really
> > > unqualified (including address space), without extra user alignment
> > > and has just one entry for each of the TYPE_MODE/TYPE_REF_CAN_ALIAS_ALL
> > > pair (unless something would modify the types in place, but that would
> > > be wrong).
> > 
> > Is that so?  I can't find any code verifying that (verify_type?).
> > Of course build_{pointer,reference}_type_for_mode will always build
> > unqualified pointers, but then the LTO code does
> > 
> >   /* The following reconstructs the pointer chains
> >      of the new pointed-to type if we are a main variant.  We do
> >      not stream those so they are broken before fixup.  */
> >   if (TREE_CODE (t) == POINTER_TYPE
> >       && TYPE_MAIN_VARIANT (t) == t)
> >     {
> >       TYPE_NEXT_PTR_TO (t) = TYPE_POINTER_TO (TREE_TYPE (t));
> >       TYPE_POINTER_TO (TREE_TYPE (t)) = t;
> >     }
> >   else if (TREE_CODE (t) == REFERENCE_TYPE
> >            && TYPE_MAIN_VARIANT (t) == t)
> >     {
> >       TYPE_NEXT_REF_TO (t) = TYPE_REFERENCE_TO (TREE_TYPE (t));
> >       TYPE_REFERENCE_TO (TREE_TYPE (t)) = t;
> >     }
> > 
> > which was supposed to ensure only putting unqualified pointers
> > (not pointed to types!) to the chain.  So to me the question is
> > rather why a type with TYPE_USER_ALIGN is a the main variant - that's
> > what looks wrong here?
> 
> First of all, it is unclear how the above ensures that type isn't
> added to those chains even when it already is there (or some similar type).
> Say lto1 during initialization needs build_pointer_type (float_type_node)
> and later on lto_fixup_prevailing_type is called on some float *.

We try to make sure to put all built types into the type merging
machinery, so I think it shouldn't happen - but then I cannot rule
it out.  I'm also not sure whether duplicates would really pose
a problem here (other than waste).  If we want to make sure we should
probably enhance verify_types to verify the TYPE_POINTER_TO chain ...

> I'll try to debug why those user aligned types become TYPE_MAIN_VARIANT
> though...
> 
> 	Jakub
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg,
Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman;
HRB 36809 (AG Nuernberg)

Re: [PATCH] lto: Fix up lto_fixup_prevailing_type [PR108910]

[Jakub Jelinek]

[-- Attachment #1: Type: text/plain, Size: 4381 bytes --]

On Tue, Feb 28, 2023 at 09:43:37AM +0000, Richard Biener wrote:
> We try to make sure to put all built types into the type merging
> machinery, so I think it shouldn't happen - but then I cannot rule
> it out.  I'm also not sure whether duplicates would really pose
> a problem here (other than waste).  If we want to make sure we should
> probably enhance verify_types to verify the TYPE_POINTER_TO chain ...
> 
> > I'll try to debug why those user aligned types become TYPE_MAIN_VARIANT
> > though...

Ok, so this happens already in the FEs when trying to add the attribute:
#0  build_distinct_type_copy (type=<pointer_type 0x7fffea0219d8>) at ../../gcc/tree.cc:5735
#1  0x0000000000c2327b in build_type_attribute_qual_variant (otype=<pointer_type 0x7fffea0219d8>, attribute=<tree_list 0x7fffea01e938>, quals=0) at ../../gcc/attribs.cc:1298
#2  0x0000000000c245b0 in build_type_attribute_variant (ttype=<pointer_type 0x7fffea0219d8>, attribute=<tree_list 0x7fffea01e938>) at ../../gcc/attribs.cc:1591
#3  0x0000000000c22325 in decl_attributes (node=0x7fffffffd008, attributes=<tree_list 0x7fffea01e910>, flags=1, last_decl=<tree 0x0>) at ../../gcc/attribs.cc:964

So, perhaps the !TYPE_QUALS (t) could be just an assert, but maybe next to
the !TYPE_USER_ALIGN (t) (or just instead of?) we need !TYPE_ATTRIBUTES (t).
Because addition of attributes (but anything else that causes
build_distinct_type_copy rather than build_variant_type_copy) will create
new TYPE_MAIN_VARIANTS.
Looking around, TYPE_REF_IS_RVALUE references also create distinct types,
and while the C++ FE sticks them into the TYPE_REFERENCE_TO chain, it
ensures they go after the corresponding !TYPE_REF_IS_RVALUE entry, so
perhaps LTO should !TYPE_REF_IS_RVALUE for REFERENCE_TYPEs.
Other uses of build_distinct_type_copy in the FEs are mostly related to
ARRAY_TYPEs (in C FE as well as c-common).  Asan uses it solely on integral
types, etc.  For attributes, big question is if it when we set
*no_addr_attrs = true we still tweak some things on the type (not in place)
or not.

So, here are two possible variant patches which fix the ICE on the
testcase too.

2023-02-28  Jakub Jelinek  <jakub@redhat.com>

	PR target/108910
	* lto-common.cc (lto_fixup_prevailing_type): Don't add t to
	TYPE_POINTER_TO or TYPE_REFERENCE_TO chain if it has 
	TYPE_ATTRIBUTES, or is TYPE_REF_IS_RVALUE, or some other type
	with the same TYPE_MODE and TYPE_REF_CAN_ALIAS_ALL flag is already
	present.

--- gcc/lto/lto-common.cc.jj	2023-01-16 11:52:16.165732856 +0100
+++ gcc/lto/lto-common.cc	2023-02-28 12:30:37.014471255 +0100
@@ -984,21 +984,36 @@ lto_fixup_prevailing_type (tree t)
       TYPE_NEXT_VARIANT (t) = TYPE_NEXT_VARIANT (mv);
       TYPE_NEXT_VARIANT (mv) = t;
     }
-
-  /* The following reconstructs the pointer chains
-     of the new pointed-to type if we are a main variant.  We do
-     not stream those so they are broken before fixup.  */
-  if (TREE_CODE (t) == POINTER_TYPE
-      && TYPE_MAIN_VARIANT (t) == t)
-    {
-      TYPE_NEXT_PTR_TO (t) = TYPE_POINTER_TO (TREE_TYPE (t));
-      TYPE_POINTER_TO (TREE_TYPE (t)) = t;
-    }
-  else if (TREE_CODE (t) == REFERENCE_TYPE
-	   && TYPE_MAIN_VARIANT (t) == t)
+  else if (!TYPE_ATTRIBUTES (t))
     {
-      TYPE_NEXT_REF_TO (t) = TYPE_REFERENCE_TO (TREE_TYPE (t));
-      TYPE_REFERENCE_TO (TREE_TYPE (t)) = t;
+      /* The following reconstructs the pointer chains
+	 of the new pointed-to type if we are a main variant.  We do
+	 not stream those so they are broken before fixup.
+	 Don't add it if despite being main variant it has
+	 attributes (then it was created with build_distinct_type_copy).
+	 Similarly don't add TYPE_REF_IS_RVALUE REFERENCE_TYPEs.
+	 Don't add it if there is something in the chain already.  */
+      tree *p = NULL;
+      if (TREE_CODE (t) == POINTER_TYPE)
+	p = &TYPE_POINTER_TO (TREE_TYPE (t));
+      else if (TREE_CODE (t) == REFERENCE_TYPE && !TYPE_REF_IS_RVALUE (t))
+	p = &TYPE_REFERENCE_TO (TREE_TYPE (t));
+      if (p)
+	{
+	  tree t2;
+	  for (t2 = *p; t2; t2 = TYPE_NEXT_PTR_TO (t2))
+	    if (TYPE_MODE (t2) == TYPE_MODE (t)
+		&& TYPE_REF_CAN_ALIAS_ALL (t2) == TYPE_REF_CAN_ALIAS_ALL (t))
+	      break;
+	  if (t2 == NULL_TREE)
+	    {
+	      if (TREE_CODE (t) == POINTER_TYPE)
+		TYPE_NEXT_PTR_TO (t) = *p;
+	      else
+		TYPE_NEXT_REF_TO (t) = *p;
+	      *p = t;
+	    }
+	}
     }
 }
 


	Jakub

[-- Attachment #2: R819 --]
[-- Type: text/plain, Size: 1881 bytes --]

2023-02-28  Jakub Jelinek  <jakub@redhat.com>

	PR target/108910
	* lto-common.cc (lto_fixup_prevailing_type): Don't add t to
	TYPE_POINTER_TO or TYPE_REFERENCE_TO chain if it has 
	TYPE_ATTRIBUTES or is TYPE_REF_IS_RVALUE.

--- gcc/lto/lto-common.cc.jj	2023-01-16 11:52:16.165732856 +0100
+++ gcc/lto/lto-common.cc	2023-02-28 12:34:33.698038478 +0100
@@ -984,21 +984,25 @@ lto_fixup_prevailing_type (tree t)
       TYPE_NEXT_VARIANT (t) = TYPE_NEXT_VARIANT (mv);
       TYPE_NEXT_VARIANT (mv) = t;
     }
-
-  /* The following reconstructs the pointer chains
-     of the new pointed-to type if we are a main variant.  We do
-     not stream those so they are broken before fixup.  */
-  if (TREE_CODE (t) == POINTER_TYPE
-      && TYPE_MAIN_VARIANT (t) == t)
-    {
-      TYPE_NEXT_PTR_TO (t) = TYPE_POINTER_TO (TREE_TYPE (t));
-      TYPE_POINTER_TO (TREE_TYPE (t)) = t;
-    }
-  else if (TREE_CODE (t) == REFERENCE_TYPE
-	   && TYPE_MAIN_VARIANT (t) == t)
+  else if (!TYPE_ATTRIBUTES (t))
     {
-      TYPE_NEXT_REF_TO (t) = TYPE_REFERENCE_TO (TREE_TYPE (t));
-      TYPE_REFERENCE_TO (TREE_TYPE (t)) = t;
+      /* The following reconstructs the pointer chains
+	 of the new pointed-to type if we are a main variant.  We do
+	 not stream those so they are broken before fixup.
+	 Don't add it if despite being main variant it has
+	 attributes (then it was created with build_distinct_type_copy).
+	 Similarly don't add TYPE_REF_IS_RVALUE REFERENCE_TYPEs.
+	 Don't add it if there is something in the chain already.  */
+      if (TREE_CODE (t) == POINTER_TYPE)
+	{
+	  TYPE_NEXT_PTR_TO (t) = TYPE_POINTER_TO (TREE_TYPE (t));
+	  TYPE_POINTER_TO (TREE_TYPE (t)) = t;
+	}
+      else if (TREE_CODE (t) == REFERENCE_TYPE && !TYPE_REF_IS_RVALUE (t))
+	{
+	  TYPE_NEXT_REF_TO (t) = TYPE_REFERENCE_TO (TREE_TYPE (t));
+	  TYPE_REFERENCE_TO (TREE_TYPE (t)) = t;
+	}
     }
 }
 

Re: [PATCH] lto: Fix up lto_fixup_prevailing_type [PR108910]

[Richard Biener]

On Tue, 28 Feb 2023, Jakub Jelinek wrote:

> On Tue, Feb 28, 2023 at 09:43:37AM +0000, Richard Biener wrote:
> > We try to make sure to put all built types into the type merging
> > machinery, so I think it shouldn't happen - but then I cannot rule
> > it out.  I'm also not sure whether duplicates would really pose
> > a problem here (other than waste).  If we want to make sure we should
> > probably enhance verify_types to verify the TYPE_POINTER_TO chain ...
> > 
> > > I'll try to debug why those user aligned types become TYPE_MAIN_VARIANT
> > > though...
> 
> Ok, so this happens already in the FEs when trying to add the attribute:
> #0  build_distinct_type_copy (type=<pointer_type 0x7fffea0219d8>) at ../../gcc/tree.cc:5735
> #1  0x0000000000c2327b in build_type_attribute_qual_variant (otype=<pointer_type 0x7fffea0219d8>, attribute=<tree_list 0x7fffea01e938>, quals=0) at ../../gcc/attribs.cc:1298
> #2  0x0000000000c245b0 in build_type_attribute_variant (ttype=<pointer_type 0x7fffea0219d8>, attribute=<tree_list 0x7fffea01e938>) at ../../gcc/attribs.cc:1591
> #3  0x0000000000c22325 in decl_attributes (node=0x7fffffffd008, attributes=<tree_list 0x7fffea01e910>, flags=1, last_decl=<tree 0x0>) at ../../gcc/attribs.cc:964
> 
> So, perhaps the !TYPE_QUALS (t) could be just an assert, but maybe next to
> the !TYPE_USER_ALIGN (t) (or just instead of?) we need !TYPE_ATTRIBUTES (t).
> Because addition of attributes (but anything else that causes
> build_distinct_type_copy rather than build_variant_type_copy) will create
> new TYPE_MAIN_VARIANTS.
> Looking around, TYPE_REF_IS_RVALUE references also create distinct types,
> and while the C++ FE sticks them into the TYPE_REFERENCE_TO chain, it
> ensures they go after the corresponding !TYPE_REF_IS_RVALUE entry, so
> perhaps LTO should !TYPE_REF_IS_RVALUE for REFERENCE_TYPEs.
> Other uses of build_distinct_type_copy in the FEs are mostly related to
> ARRAY_TYPEs (in C FE as well as c-common).  Asan uses it solely on integral
> types, etc.  For attributes, big question is if it when we set
> *no_addr_attrs = true we still tweak some things on the type (not in place)
> or not.
> 
> So, here are two possible variant patches which fix the ICE on the
> testcase too.
> 
> 2023-02-28  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR target/108910
> 	* lto-common.cc (lto_fixup_prevailing_type): Don't add t to
> 	TYPE_POINTER_TO or TYPE_REFERENCE_TO chain if it has 
> 	TYPE_ATTRIBUTES, or is TYPE_REF_IS_RVALUE, or some other type
> 	with the same TYPE_MODE and TYPE_REF_CAN_ALIAS_ALL flag is already
> 	present.
> 
> --- gcc/lto/lto-common.cc.jj	2023-01-16 11:52:16.165732856 +0100
> +++ gcc/lto/lto-common.cc	2023-02-28 12:30:37.014471255 +0100
> @@ -984,21 +984,36 @@ lto_fixup_prevailing_type (tree t)
>        TYPE_NEXT_VARIANT (t) = TYPE_NEXT_VARIANT (mv);
>        TYPE_NEXT_VARIANT (mv) = t;
>      }
> -
> -  /* The following reconstructs the pointer chains
> -     of the new pointed-to type if we are a main variant.  We do
> -     not stream those so they are broken before fixup.  */
> -  if (TREE_CODE (t) == POINTER_TYPE
> -      && TYPE_MAIN_VARIANT (t) == t)
> -    {
> -      TYPE_NEXT_PTR_TO (t) = TYPE_POINTER_TO (TREE_TYPE (t));
> -      TYPE_POINTER_TO (TREE_TYPE (t)) = t;
> -    }
> -  else if (TREE_CODE (t) == REFERENCE_TYPE
> -	   && TYPE_MAIN_VARIANT (t) == t)
> +  else if (!TYPE_ATTRIBUTES (t))
>      {
> -      TYPE_NEXT_REF_TO (t) = TYPE_REFERENCE_TO (TREE_TYPE (t));
> -      TYPE_REFERENCE_TO (TREE_TYPE (t)) = t;
> +      /* The following reconstructs the pointer chains
> +	 of the new pointed-to type if we are a main variant.  We do
> +	 not stream those so they are broken before fixup.
> +	 Don't add it if despite being main variant it has
> +	 attributes (then it was created with build_distinct_type_copy).
> +	 Similarly don't add TYPE_REF_IS_RVALUE REFERENCE_TYPEs.
> +	 Don't add it if there is something in the chain already.  */
> +      tree *p = NULL;
> +      if (TREE_CODE (t) == POINTER_TYPE)
> +	p = &TYPE_POINTER_TO (TREE_TYPE (t));
> +      else if (TREE_CODE (t) == REFERENCE_TYPE && !TYPE_REF_IS_RVALUE (t))
> +	p = &TYPE_REFERENCE_TO (TREE_TYPE (t));
> +      if (p)
> +	{
> +	  tree t2;
> +	  for (t2 = *p; t2; t2 = TYPE_NEXT_PTR_TO (t2))
> +	    if (TYPE_MODE (t2) == TYPE_MODE (t)
> +		&& TYPE_REF_CAN_ALIAS_ALL (t2) == TYPE_REF_CAN_ALIAS_ALL (t))
> +	      break;

Can we elide this searching please?  Having duplicated should be harmless
unless proved otherwise.

OK with that change.

Richard.

> +	  if (t2 == NULL_TREE)
> +	    {
> +	      if (TREE_CODE (t) == POINTER_TYPE)
> +		TYPE_NEXT_PTR_TO (t) = *p;
> +	      else
> +		TYPE_NEXT_REF_TO (t) = *p;
> +	      *p = t;
> +	    }
> +	}
>      }
>  }
>  
> 
> 
> 	Jakub
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg,
Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman;
HRB 36809 (AG Nuernberg)

Re: [PATCH] lto: Fix up lto_fixup_prevailing_type [PR108910]

[Jakub Jelinek]

On Tue, Feb 28, 2023 at 12:05:20PM +0000, Richard Biener via Gcc-patches wrote:
> > +	p = &TYPE_POINTER_TO (TREE_TYPE (t));
> > +      else if (TREE_CODE (t) == REFERENCE_TYPE && !TYPE_REF_IS_RVALUE (t))
> > +	p = &TYPE_REFERENCE_TO (TREE_TYPE (t));
> > +      if (p)
> > +	{
> > +	  tree t2;
> > +	  for (t2 = *p; t2; t2 = TYPE_NEXT_PTR_TO (t2))
> > +	    if (TYPE_MODE (t2) == TYPE_MODE (t)
> > +		&& TYPE_REF_CAN_ALIAS_ALL (t2) == TYPE_REF_CAN_ALIAS_ALL (t))
> > +	      break;
> 
> Can we elide this searching please?  Having duplicated should be harmless
> unless proved otherwise.

I've posted 2 patches (one inlined, another attached), the second one
didn't do this at all.
Having (too many) duplicates would be harmful because build_pointer_type
etc. walk up to the whole length of list all the time.  When the list
length is bounded (say at most 2 modes - ptr_mode/Pmode, times 2 (the
can alias all bool), then it doesn't hurt, if it could in theory be
arbitrarily long, it would be a compile time problem.
But given that we with the !TYPE_ATTRIBUTES/!TYPE_REF_IS_RVALUE change
don't have a testcase that would show it is a problem actually ever
encounterable, the second patch without this is fine I think.
> 
> OK with that change.

	Jakub

Re: [PATCH] lto: Fix up lto_fixup_prevailing_type [PR108910]

[Richard Biener]

On Tue, 28 Feb 2023, Jakub Jelinek wrote:

> On Tue, Feb 28, 2023 at 12:05:20PM +0000, Richard Biener via Gcc-patches wrote:
> > > +	p = &TYPE_POINTER_TO (TREE_TYPE (t));
> > > +      else if (TREE_CODE (t) == REFERENCE_TYPE && !TYPE_REF_IS_RVALUE (t))
> > > +	p = &TYPE_REFERENCE_TO (TREE_TYPE (t));
> > > +      if (p)
> > > +	{
> > > +	  tree t2;
> > > +	  for (t2 = *p; t2; t2 = TYPE_NEXT_PTR_TO (t2))
> > > +	    if (TYPE_MODE (t2) == TYPE_MODE (t)
> > > +		&& TYPE_REF_CAN_ALIAS_ALL (t2) == TYPE_REF_CAN_ALIAS_ALL (t))
> > > +	      break;
> > 
> > Can we elide this searching please?  Having duplicated should be harmless
> > unless proved otherwise.
> 
> I've posted 2 patches (one inlined, another attached), the second one
> didn't do this at all.

Ah, didn't notice that.

> Having (too many) duplicates would be harmful because build_pointer_type
> etc. walk up to the whole length of list all the time.  When the list
> length is bounded (say at most 2 modes - ptr_mode/Pmode, times 2 (the
> can alias all bool), then it doesn't hurt, if it could in theory be
> arbitrarily long, it would be a compile time problem.
> But given that we with the !TYPE_ATTRIBUTES/!TYPE_REF_IS_RVALUE change
> don't have a testcase that would show it is a problem actually ever
> encounterable, the second patch without this is fine I think.

I agree the attached patch is fine.

Richard.