diff mbox

Fix gnat.dg/aliasing2.adb regression

Message ID 201011290824.07093.ebotcazou@adacore.com
State New
Headers show

Commit Message

Eric Botcazou Nov. 29, 2010, 7:24 a.m. UTC
The test doesn't pass anymore because the flag TYPE_NONALIASED_COMPONENT isn't 
effective anymore in Ada.  It technically still works but all array types are 
now given alias set 0 in Ada; this is because all unconstrained array types 
are now TYPE_STRUCTURAL_EQUALITY_P, as their index type also is.

The latter setting was changed when build_range_type and build_index_type were 
merged; the former didn't set TYPE_STRUCTURAL_EQUALITY_P, the latter did.

I don't think we need to set TYPE_STRUCTURAL_EQUALITY_P on range types with 
self-referential bounds as they cannot be merged in any case.  This is enough 
to fix the regression.

LTO-bootstrapped/regested on x86_64-suse-linux, OK for the mainline?


2010-11-29  Eric Botcazou  <ebotcazou@adacore.com>

	* tree.c (build_range_type_1): Do not set TYPE_STRUCTURAL_EQUALITY_P
	because of self-referential bounds.

Comments

Richard Biener Nov. 29, 2010, 10:27 a.m. UTC | #1
On Mon, Nov 29, 2010 at 8:24 AM, Eric Botcazou <ebotcazou@adacore.com> wrote:
> The test doesn't pass anymore because the flag TYPE_NONALIASED_COMPONENT isn't
> effective anymore in Ada.  It technically still works but all array types are
> now given alias set 0 in Ada; this is because all unconstrained array types
> are now TYPE_STRUCTURAL_EQUALITY_P, as their index type also is.
>
> The latter setting was changed when build_range_type and build_index_type were
> merged; the former didn't set TYPE_STRUCTURAL_EQUALITY_P, the latter did.
>
> I don't think we need to set TYPE_STRUCTURAL_EQUALITY_P on range types with
> self-referential bounds as they cannot be merged in any case.  This is enough
> to fix the regression.

Well.  The frontend needs to make sure to merge all same copies of
them then.  OTOH range types shouldn't be "in memory" anyway,
so why do we ask for the alias-set of the range type in the first place?

Thus, I think that we should instead in build_array_type_1 drop the
TYPE_CANONICAL handling of the index_type completely, like

  if (TYPE_CANONICAL (t) == t)
    {
      if (TYPE_STRUCTURAL_EQUALITY_P (elt_type))
        SET_TYPE_STRUCTURAL_EQUALITY (t);
      else if (TYPE_CANONICAL (elt_type) != elt_type
        TYPE_CANONICAL (t)
          = build_array_type_1 (TYPE_CANONICAL (elt_type),
                                index_type, shared);
    }

which then means the TYPE_CANONICAL setting of range_types
isn't relevant anywhere and we don't need to bother about it.

At least the middle-end cares for canonical types only to determine
if assignments of two vars are valid (and we shouldn't have any
instances of range-types in the IL) and to make sure similar types
get the same alias-set (we shouldn't have range-type instances in
memory either, and arrays are special-cased in get_alias_set).

Which also means that build_range_type_1 can just do

  if (!shared
      || (TYPE_MIN_VALUE (itype)
          && TREE_CODE (TYPE_MIN_VALUE (itype)) != INTEGER_CST)
      || (TYPE_MAX_VALUE (itype)
          && TREE_CODE (TYPE_MAX_VALUE (itype)) != INTEGER_CST))
    return itype;

> LTO-bootstrapped/regested on x86_64-suse-linux, OK for the mainline?

Sooo - can you check sth along the above instead?

Thanks,
Richard.

>
> 2010-11-29  Eric Botcazou  <ebotcazou@adacore.com>
>
>        * tree.c (build_range_type_1): Do not set TYPE_STRUCTURAL_EQUALITY_P
>        because of self-referential bounds.
>
>
> --
> Eric Botcazou
>
Eric Botcazou Nov. 29, 2010, 10:49 a.m. UTC | #2
> Well.  The frontend needs to make sure to merge all same copies of
> them then.

This doesn't really make sense for type_contains_placeholder_p types.  You 
cannot merge these things as they contain PLACEHOLDER_EXPRs that rely on 
pointer equality to be instantiated.

> OTOH range types shouldn't be "in memory" anyway, so why do we ask for the
> alias-set of the range type in the first place? 

We don't, we ask for the alias set of array types only.

> Thus, I think that we should instead in build_array_type_1 drop the
> TYPE_CANONICAL handling of the index_type completely, like
>
>   if (TYPE_CANONICAL (t) == t)
>     {
>       if (TYPE_STRUCTURAL_EQUALITY_P (elt_type))
>         SET_TYPE_STRUCTURAL_EQUALITY (t);
>       else if (TYPE_CANONICAL (elt_type) != elt_type
>         TYPE_CANONICAL (t)
>           = build_array_type_1 (TYPE_CANONICAL (elt_type),
>                                 index_type, shared);
>     }
>
> which then means the TYPE_CANONICAL setting of range_types
> isn't relevant anywhere and we don't need to bother about it.

OK, fine with me.

> At least the middle-end cares for canonical types only to determine
> if assignments of two vars are valid (and we shouldn't have any
> instances of range-types in the IL) and to make sure similar types
> get the same alias-set (we shouldn't have range-type instances in
> memory either, and arrays are special-cased in get_alias_set).

I'm not sure about the "no instances of range-types in the IL" in Ada.

> Sooo - can you check sth along the above instead?

I can at least test the build_array_type_1 change in isolation.
diff mbox

Patch

Index: tree.c
===================================================================
--- tree.c	(revision 167201)
+++ tree.c	(working copy)
@@ -7110,9 +7110,11 @@  build_range_type_1 (tree type, tree lowv
   TYPE_USER_ALIGN (itype) = TYPE_USER_ALIGN (type);
 
   if ((TYPE_MIN_VALUE (itype)
-       && TREE_CODE (TYPE_MIN_VALUE (itype)) != INTEGER_CST)
+       && TREE_CODE (TYPE_MIN_VALUE (itype)) != INTEGER_CST
+       && !CONTAINS_PLACEHOLDER_P (TYPE_MIN_VALUE (itype)))
       || (TYPE_MAX_VALUE (itype)
-	  && TREE_CODE (TYPE_MAX_VALUE (itype)) != INTEGER_CST))
+	  && TREE_CODE (TYPE_MAX_VALUE (itype)) != INTEGER_CST
+	  && !CONTAINS_PLACEHOLDER_P (TYPE_MAX_VALUE (itype))))
     {
       /* Since we cannot reliably merge this type, we need to compare it using
 	 structural equality checks.  */