Fix canonical types of atomic types

Hi,
build_qualified_type adjusts alignment of atomic types to one of minimal
alignment needed for atomic operations (I think it does so). For packed
structures this leads to type variant to be created and alignment to be
updated later.

If you call again build_qualified_type on packed structures, it won't
reuse existing type because check_base_type will compare alignment of
the base type (which is not atomic and has smaller alignment) and will
end up creating new variant.

When constructing a canonical types C frontned relies on types being
shared and this eventually leads to ice in type simplification.

I think it is easiest to teach check_base_type about minimal alignment.

Bootstrapped/regtested x86_64-linux.
	PR lto/88585
	* tree.c (find_atomic_core_type): Forward declare.
	(check_base_type): Correctly compare alignments of atomic types.

PING^1

On 2/10/19 6:21 PM, Jan Hubicka wrote:
> Hi,
> build_qualified_type adjusts alignment of atomic types to one of minimal
> alignment needed for atomic operations (I think it does so). For packed
> structures this leads to type variant to be created and alignment to be
> updated later.
> 
> If you call again build_qualified_type on packed structures, it won't
> reuse existing type because check_base_type will compare alignment of
> the base type (which is not atomic and has smaller alignment) and will
> end up creating new variant.
> 
> When constructing a canonical types C frontned relies on types being
> shared and this eventually leads to ice in type simplification.
> 
> I think it is easiest to teach check_base_type about minimal alignment.
> 
> Bootstrapped/regtested x86_64-linux.
> 	PR lto/88585
> 	* tree.c (find_atomic_core_type): Forward declare.
> 	(check_base_type): Correctly compare alignments of atomic types.
> Index: tree.c
> ===================================================================
> --- tree.c	(revision 268742)
> +++ tree.c	(working copy)
> @@ -6329,18 +6329,33 @@ check_lang_type (const_tree cand, const_
>    return lang_hooks.types.type_hash_eq (cand, base);
>  }
>  
> +static tree find_atomic_core_type (const_tree type);
> +
>  /* Returns true iff unqualified CAND and BASE are equivalent.  */
>  
>  bool
>  check_base_type (const_tree cand, const_tree base)
>  {
> -  return (TYPE_NAME (cand) == TYPE_NAME (base)
> -	  /* Apparently this is needed for Objective-C.  */
> -	  && TYPE_CONTEXT (cand) == TYPE_CONTEXT (base)
> -	  /* Check alignment.  */
> -	  && TYPE_ALIGN (cand) == TYPE_ALIGN (base)
> -	  && attribute_list_equal (TYPE_ATTRIBUTES (cand),
> -				   TYPE_ATTRIBUTES (base)));
> +  if (TYPE_NAME (cand) != TYPE_NAME (base)
> +      /* Apparently this is needed for Objective-C.  */
> +      || TYPE_CONTEXT (cand) != TYPE_CONTEXT (base)
> +      || !attribute_list_equal (TYPE_ATTRIBUTES (cand),
> +			        TYPE_ATTRIBUTES (base)))
> +    return false;
> +  /* Check alignment.  */
> +  if (TYPE_ALIGN (cand) == TYPE_ALIGN (base))
> +    return true;
> +  /* Atomic types increase minimal alignment.  We must to do so as well
> +     or we get duplicated canonical types. See PR88686.  */
> +  if ((TYPE_QUALS (cand) & TYPE_QUAL_ATOMIC))
> +    {
> +      /* See if this object can map to a basic atomic type.  */
> +      tree atomic_type = find_atomic_core_type (cand);
> +      if (TYPE_ALIGN (atomic_type) == TYPE_ALIGN (cand)
> +	  && TYPE_ALIGN (base) < TYPE_ALIGN (cand))
> +       return true;
> +    }
> +  return false;
>  }
>  
>  /* Returns true iff CAND is equivalent to BASE with TYPE_QUALS.  */
> @@ -6373,7 +6388,7 @@ check_aligned_type (const_tree cand, con
>     atomic types, and returns that core atomic type.  */
>  
>  static tree
> -find_atomic_core_type (tree type)
> +find_atomic_core_type (const_tree type)
>  {
>    tree base_atomic_type;
>  
>

On Sun, Feb 10, 2019 at 6:21 PM Jan Hubicka <hubicka@ucw.cz> wrote:
>
> Hi,
> build_qualified_type adjusts alignment of atomic types to one of minimal
> alignment needed for atomic operations (I think it does so). For packed
> structures this leads to type variant to be created and alignment to be
> updated later.
>
> If you call again build_qualified_type on packed structures, it won't
> reuse existing type because check_base_type will compare alignment of
> the base type (which is not atomic and has smaller alignment) and will
> end up creating new variant.
>
> When constructing a canonical types C frontned relies on types being
> shared and this eventually leads to ice in type simplification.
>
> I think it is easiest to teach check_base_type about minimal alignment.
>
> Bootstrapped/regtested x86_64-linux.
>         PR lto/88585
>         * tree.c (find_atomic_core_type): Forward declare.
>         (check_base_type): Correctly compare alignments of atomic types.
> Index: tree.c
> ===================================================================
> --- tree.c      (revision 268742)
> +++ tree.c      (working copy)
> @@ -6329,18 +6329,33 @@ check_lang_type (const_tree cand, const_
>    return lang_hooks.types.type_hash_eq (cand, base);
>  }
>
> +static tree find_atomic_core_type (const_tree type);
> +

Just move find_atomic_core_type please.

>  /* Returns true iff unqualified CAND and BASE are equivalent.  */
>
>  bool
>  check_base_type (const_tree cand, const_tree base)
>  {
> -  return (TYPE_NAME (cand) == TYPE_NAME (base)
> -         /* Apparently this is needed for Objective-C.  */
> -         && TYPE_CONTEXT (cand) == TYPE_CONTEXT (base)
> -         /* Check alignment.  */
> -         && TYPE_ALIGN (cand) == TYPE_ALIGN (base)
> -         && attribute_list_equal (TYPE_ATTRIBUTES (cand),
> -                                  TYPE_ATTRIBUTES (base)));
> +  if (TYPE_NAME (cand) != TYPE_NAME (base)
> +      /* Apparently this is needed for Objective-C.  */
> +      || TYPE_CONTEXT (cand) != TYPE_CONTEXT (base)
> +      || !attribute_list_equal (TYPE_ATTRIBUTES (cand),
> +                               TYPE_ATTRIBUTES (base)))
> +    return false;
> +  /* Check alignment.  */
> +  if (TYPE_ALIGN (cand) == TYPE_ALIGN (base))
> +    return true;
> +  /* Atomic types increase minimal alignment.  We must to do so as well
> +     or we get duplicated canonical types. See PR88686.  */
> +  if ((TYPE_QUALS (cand) & TYPE_QUAL_ATOMIC))
> +    {
> +      /* See if this object can map to a basic atomic type.  */
> +      tree atomic_type = find_atomic_core_type (cand);

build_qualified_type handles the case this function returns NULL,
I think you should as well.

> +      if (TYPE_ALIGN (atomic_type) == TYPE_ALIGN (cand)
> +         && TYPE_ALIGN (base) < TYPE_ALIGN (cand))

Why the second condition?  build_qualified_type simply does

      if (((type_quals & TYPE_QUAL_ATOMIC) == TYPE_QUAL_ATOMIC))
        {
          /* See if this object can map to a basic atomic type.  */
          tree atomic_type = find_atomic_core_type (type);
          if (atomic_type)
            {
              /* Ensure the alignment of this type is compatible with
                 the required alignment of the atomic type.  */
              if (TYPE_ALIGN (atomic_type) > TYPE_ALIGN (t))
                SET_TYPE_ALIGN (t, TYPE_ALIGN (atomic_type));

without caring for TYPE_ALIGN of the base type.

Note we seem to happily accept build_aligned_type that produce
under-aligned atomics :/

Richard.

> +       return true;
> +    }
> +  return false;
>  }
>
>  /* Returns true iff CAND is equivalent to BASE with TYPE_QUALS.  */
> @@ -6373,7 +6388,7 @@ check_aligned_type (const_tree cand, con
>     atomic types, and returns that core atomic type.  */
>
>  static tree
> -find_atomic_core_type (tree type)
> +find_atomic_core_type (const_tree type)
>  {
>    tree base_atomic_type;
>

> > +  /* Atomic types increase minimal alignment.  We must to do so as well
> > +     or we get duplicated canonical types. See PR88686.  */
> > +  if ((TYPE_QUALS (cand) & TYPE_QUAL_ATOMIC))
> > +    {
> > +      /* See if this object can map to a basic atomic type.  */
> > +      tree atomic_type = find_atomic_core_type (cand);
> 
> build_qualified_type handles the case this function returns NULL,
> I think you should as well.
Done.
> 
> > +      if (TYPE_ALIGN (atomic_type) == TYPE_ALIGN (cand)
> > +         && TYPE_ALIGN (base) < TYPE_ALIGN (cand))
> 
> Why the second condition?  build_qualified_type simply does
> 
>       if (((type_quals & TYPE_QUAL_ATOMIC) == TYPE_QUAL_ATOMIC))
>         {
>           /* See if this object can map to a basic atomic type.  */
>           tree atomic_type = find_atomic_core_type (type);
>           if (atomic_type)
>             {
>               /* Ensure the alignment of this type is compatible with
>                  the required alignment of the atomic type.  */
>               if (TYPE_ALIGN (atomic_type) > TYPE_ALIGN (t))
>                 SET_TYPE_ALIGN (t, TYPE_ALIGN (atomic_type));
> 
> without caring for TYPE_ALIGN of the base type.A

It only increases the alignment if necessary but if the alignment
is kept, it is handled by the earlier code. So i have dropped this.
> 
> Note we seem to happily accept build_aligned_type that produce
> under-aligned atomics :/

Yep :(

This is updated patch I re-tested x86_64-linux and comitted.

Index: ChangeLog
===================================================================
--- ChangeLog	(revision 269281)
+++ ChangeLog	(working copy)
@@ -1,3 +1,9 @@
+2019-02-28  Jan Hubicka  <hubicka@ucw.cz>
+
+	PR lto/88585
+	* tree.c (find_atomic_core_type): Move ahead in file.
+	(check_base_type): Correctly compare alignments of atomic types.
+
 2019-02-28  H.J. Lu  <hongjiu.lu@intel.com>
 
 	PR target/89455
Index: tree.c
===================================================================
--- tree.c	(revision 269280)
+++ tree.c	(working copy)
@@ -6329,51 +6329,11 @@ check_lang_type (const_tree cand, const_
   return lang_hooks.types.type_hash_eq (cand, base);
 }
 
-/* Returns true iff unqualified CAND and BASE are equivalent.  */
-
-bool
-check_base_type (const_tree cand, const_tree base)
-{
-  return (TYPE_NAME (cand) == TYPE_NAME (base)
-	  /* Apparently this is needed for Objective-C.  */
-	  && TYPE_CONTEXT (cand) == TYPE_CONTEXT (base)
-	  /* Check alignment.  */
-	  && TYPE_ALIGN (cand) == TYPE_ALIGN (base)
-	  && attribute_list_equal (TYPE_ATTRIBUTES (cand),
-				   TYPE_ATTRIBUTES (base)));
-}
-
-/* Returns true iff CAND is equivalent to BASE with TYPE_QUALS.  */
-
-bool
-check_qualified_type (const_tree cand, const_tree base, int type_quals)
-{
-  return (TYPE_QUALS (cand) == type_quals
-	  && check_base_type (cand, base)
-	  && check_lang_type (cand, base));
-}
-
-/* Returns true iff CAND is equivalent to BASE with ALIGN.  */
-
-static bool
-check_aligned_type (const_tree cand, const_tree base, unsigned int align)
-{
-  return (TYPE_QUALS (cand) == TYPE_QUALS (base)
-	  && TYPE_NAME (cand) == TYPE_NAME (base)
-	  /* Apparently this is needed for Objective-C.  */
-	  && TYPE_CONTEXT (cand) == TYPE_CONTEXT (base)
-	  /* Check alignment.  */
-	  && TYPE_ALIGN (cand) == align
-	  && attribute_list_equal (TYPE_ATTRIBUTES (cand),
-				   TYPE_ATTRIBUTES (base))
-	  && check_lang_type (cand, base));
-}
-
 /* This function checks to see if TYPE matches the size one of the built-in 
    atomic types, and returns that core atomic type.  */
 
 static tree
-find_atomic_core_type (tree type)
+find_atomic_core_type (const_tree type)
 {
   tree base_atomic_type;
 
@@ -6410,6 +6370,58 @@ find_atomic_core_type (tree type)
   return base_atomic_type;
 }
 
+/* Returns true iff unqualified CAND and BASE are equivalent.  */
+
+bool
+check_base_type (const_tree cand, const_tree base)
+{
+  if (TYPE_NAME (cand) != TYPE_NAME (base)
+      /* Apparently this is needed for Objective-C.  */
+      || TYPE_CONTEXT (cand) != TYPE_CONTEXT (base)
+      || !attribute_list_equal (TYPE_ATTRIBUTES (cand),
+			        TYPE_ATTRIBUTES (base)))
+    return false;
+  /* Check alignment.  */
+  if (TYPE_ALIGN (cand) == TYPE_ALIGN (base))
+    return true;
+  /* Atomic types increase minimal alignment.  We must to do so as well
+     or we get duplicated canonical types. See PR88686.  */
+  if ((TYPE_QUALS (cand) & TYPE_QUAL_ATOMIC))
+    {
+      /* See if this object can map to a basic atomic type.  */
+      tree atomic_type = find_atomic_core_type (cand);
+      if (atomic_type && TYPE_ALIGN (atomic_type) == TYPE_ALIGN (cand))
+       return true;
+    }
+  return false;
+}
+
+/* Returns true iff CAND is equivalent to BASE with TYPE_QUALS.  */
+
+bool
+check_qualified_type (const_tree cand, const_tree base, int type_quals)
+{
+  return (TYPE_QUALS (cand) == type_quals
+	  && check_base_type (cand, base)
+	  && check_lang_type (cand, base));
+}
+
+/* Returns true iff CAND is equivalent to BASE with ALIGN.  */
+
+static bool
+check_aligned_type (const_tree cand, const_tree base, unsigned int align)
+{
+  return (TYPE_QUALS (cand) == TYPE_QUALS (base)
+	  && TYPE_NAME (cand) == TYPE_NAME (base)
+	  /* Apparently this is needed for Objective-C.  */
+	  && TYPE_CONTEXT (cand) == TYPE_CONTEXT (base)
+	  /* Check alignment.  */
+	  && TYPE_ALIGN (cand) == align
+	  && attribute_list_equal (TYPE_ATTRIBUTES (cand),
+				   TYPE_ATTRIBUTES (base))
+	  && check_lang_type (cand, base));
+}
+
 /* Return a version of the TYPE, qualified as indicated by the
    TYPE_QUALS, if one exists.  If no qualified version exists yet,
    return NULL_TREE.  */