Re: [PATCH] middle-end/114931 - type_hash_canon and structual equality types

[Richard Biener <rguenther@suse.de>]

> Am 03.05.2024 um 17:33 schrieb Martin Uecker <uecker@tugraz.at>:
> 
> Am Freitag, dem 03.05.2024 um 14:13 +0200 schrieb Richard Biener:
>> TYPE_STRUCTURAL_EQUALITY_P is part of our type system so we have
>> to make sure to include that into the type unification done via
>> type_hash_canon.  This requires the flag to be set before querying
>> the hash which is the biggest part of the patch.
> 
> I assume this does not affect structs / unions because they
> do not make this mechanism of type unification (each tagged type
> is a unique type), but only derived types that end up having
> TYPE_STRUCTURAL_EQUALITY_P because they are constructed from
> incomplete structs / unions before TYPE_CANONICAL is set.
> 
> I do not yet understand why this change is needed. Type
> identity should not be affected by setting TYPE_CANONICAL, so
> why do we need to keep such types separate?  I understand that we
> created some inconsistencies, but I do not see why this change
> is needed to fix it.  But I also haven't understood how we ended
> up with a TYPE_CANONICAL having TYPE_STRUCTURAL_EQUALITY_P in
> PR 114931 ...

Because we created the canonical function type before where one of its arguments had TYPE_STEUCTURAL_EQUALITY which makes the function type so.

Richard 

> 
> Martin
> 
> 
>> 
>> Bootstrapped and tested on x86_64-unknown-linux-gnu for all languages.
>> 
>> As said in the PR this merely makes sure to keep individual types
>> consistent with themselves.  We still will have a set of types
>> with TYPE_STRUCTURAL_EQUALITY_P and a set without that might be
>> otherwise identical.  That could be only avoided with changes in
>> the frontend.
>> 
>> OK for trunk?
>> 
>> Thanks,
>> Richard.
>> 
>>    PR middle-end/114931
>> gcc/
>>    * tree.cc (type_hash_canon_hash): Hash TYPE_STRUCTURAL_EQUALITY_P.
>>    (type_cache_hasher::equal): Compare TYPE_STRUCTURAL_EQUALITY_P.
>>    (build_array_type_1): Set TYPE_STRUCTURAL_EQUALITY_P before
>>    probing with type_hash_canon.
>>    (build_function_type): Likewise.
>>    (build_method_type_directly): Likewise.
>>    (build_offset_type): Likewise.
>>    (build_complex_type): Likewise.
>>    * attribs.cc (build_type_attribute_qual_variant): Likewise.
>> 
>> gcc/c-family/
>>    * c-common.cc (complete_array_type): Set TYPE_STRUCTURAL_EQUALITY_P
>>    before probing with type_hash_canon.
>> 
>> gcc/testsuite/
>>    * gcc.dg/pr114931.c: New testcase.
>> ---
>> gcc/attribs.cc                  | 20 +++++-----
>> gcc/c-family/c-common.cc        | 11 ++++--
>> gcc/testsuite/gcc.dg/pr114931.c | 10 +++++
>> gcc/tree.cc                     | 65 +++++++++++++++++++++++----------
>> 4 files changed, 74 insertions(+), 32 deletions(-)
>> create mode 100644 gcc/testsuite/gcc.dg/pr114931.c
>> 
>> diff --git a/gcc/attribs.cc b/gcc/attribs.cc
>> index 12ffc5f170a..3ab0b0fd87a 100644
>> --- a/gcc/attribs.cc
>> +++ b/gcc/attribs.cc
>> @@ -1336,6 +1336,16 @@ build_type_attribute_qual_variant (tree otype, tree attribute, int quals)
>>       tree dtype = ntype = build_distinct_type_copy (ttype);
>> 
>>       TYPE_ATTRIBUTES (ntype) = attribute;
>> +      /* If the target-dependent attributes make NTYPE different from
>> +     its canonical type, we will need to use structural equality
>> +     checks for this type.
>> +
>> +     We shouldn't get here for stripping attributes from a type;
>> +     the no-attribute type might not need structural comparison.  But
>> +     we can if was discarded from type_hash_table.  */
>> +      if (TYPE_STRUCTURAL_EQUALITY_P (ttype)
>> +      || !comp_type_attributes (ntype, ttype))
>> +    SET_TYPE_STRUCTURAL_EQUALITY (ntype);
>> 
>>       hashval_t hash = type_hash_canon_hash (ntype);
>>       ntype = type_hash_canon (hash, ntype);
>> @@ -1343,16 +1353,6 @@ build_type_attribute_qual_variant (tree otype, tree attribute, int quals)
>>       if (ntype != dtype)
>>    /* This variant was already in the hash table, don't mess with
>>       TYPE_CANONICAL.  */;
>> -      else if (TYPE_STRUCTURAL_EQUALITY_P (ttype)
>> -           || !comp_type_attributes (ntype, ttype))
>> -    /* If the target-dependent attributes make NTYPE different from
>> -       its canonical type, we will need to use structural equality
>> -       checks for this type.
>> -
>> -       We shouldn't get here for stripping attributes from a type;
>> -       the no-attribute type might not need structural comparison.  But
>> -       we can if was discarded from type_hash_table.  */
>> -    SET_TYPE_STRUCTURAL_EQUALITY (ntype);
>>       else if (TYPE_CANONICAL (ntype) == ntype)
>>    TYPE_CANONICAL (ntype) = TYPE_CANONICAL (ttype);
>> 
>> diff --git a/gcc/c-family/c-common.cc b/gcc/c-family/c-common.cc
>> index 01e3d247fc2..032dcb4b41d 100644
>> --- a/gcc/c-family/c-common.cc
>> +++ b/gcc/c-family/c-common.cc
>> @@ -7115,6 +7115,13 @@ complete_array_type (tree *ptype, tree initial_value, bool do_default)
>>   TYPE_TYPELESS_STORAGE (main_type) = TYPE_TYPELESS_STORAGE (type);
>>   layout_type (main_type);
>> 
>> +  /* Set TYPE_STRUCTURAL_EQUALITY_P early.  */
>> +  if (TYPE_STRUCTURAL_EQUALITY_P (TREE_TYPE (main_type))
>> +      || TYPE_STRUCTURAL_EQUALITY_P (TYPE_DOMAIN (main_type)))
>> +    SET_TYPE_STRUCTURAL_EQUALITY (main_type);
>> +  else
>> +    TYPE_CANONICAL (main_type) = main_type;
>> +
>>   /* Make sure we have the canonical MAIN_TYPE. */
>>   hashval_t hashcode = type_hash_canon_hash (main_type);
>>   main_type = type_hash_canon (hashcode, main_type);
>> @@ -7122,7 +7129,7 @@ complete_array_type (tree *ptype, tree initial_value, bool do_default)
>>   /* Fix the canonical type.  */
>>   if (TYPE_STRUCTURAL_EQUALITY_P (TREE_TYPE (main_type))
>>       || TYPE_STRUCTURAL_EQUALITY_P (TYPE_DOMAIN (main_type)))
>> -    SET_TYPE_STRUCTURAL_EQUALITY (main_type);
>> +    gcc_assert (TYPE_STRUCTURAL_EQUALITY_P (main_type));
>>   else if (TYPE_CANONICAL (TREE_TYPE (main_type)) != TREE_TYPE (main_type)
>>       || (TYPE_CANONICAL (TYPE_DOMAIN (main_type))
>>           != TYPE_DOMAIN (main_type)))
>> @@ -7130,8 +7137,6 @@ complete_array_type (tree *ptype, tree initial_value, bool do_default)
>>       = build_array_type (TYPE_CANONICAL (TREE_TYPE (main_type)),
>>              TYPE_CANONICAL (TYPE_DOMAIN (main_type)),
>>              TYPE_TYPELESS_STORAGE (main_type));
>> -  else
>> -    TYPE_CANONICAL (main_type) = main_type;
>> 
>>   if (quals == 0)
>>     type = main_type;
>> diff --git a/gcc/testsuite/gcc.dg/pr114931.c b/gcc/testsuite/gcc.dg/pr114931.c
>> new file mode 100644
>> index 00000000000..d690ed70e52
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.dg/pr114931.c
>> @@ -0,0 +1,10 @@
>> +/* { dg-do compile } */
>> +/* { dg-options "-std=c23" } */
>> +
>> +struct Tcl_Obj;
>> +void(Tcl_FreeInternalRepProc)(struct Tcl_Obj *);
>> +typedef struct Tcl_Obj {
>> +} Tcl_Obj;
>> +struct {
>> +  void (*tclFreeObj)(Tcl_Obj *);
>> +} Tcl_InitStubs;
>> diff --git a/gcc/tree.cc b/gcc/tree.cc
>> index 780662549fe..6564b002dc1 100644
>> --- a/gcc/tree.cc
>> +++ b/gcc/tree.cc
>> @@ -6012,6 +6012,8 @@ type_hash_canon_hash (tree type)
>> 
>>   hstate.add_int (TREE_CODE (type));
>> 
>> +  hstate.add_flag (TYPE_STRUCTURAL_EQUALITY_P (type));
>> +
>>   if (TREE_TYPE (type))
>>     hstate.add_object (TYPE_HASH (TREE_TYPE (type)));
>> 
>> @@ -6109,6 +6111,10 @@ type_cache_hasher::equal (type_hash *a, type_hash *b)
>>      || TYPE_MODE (a->type) != TYPE_MODE (b->type)))
>>     return false;
>> 
>> +  if (TYPE_STRUCTURAL_EQUALITY_P (a->type)
>> +      != TYPE_STRUCTURAL_EQUALITY_P (b->type))
>> +    return false;
>> +
>>   switch (TREE_CODE (a->type))
>>     {
>>     case VOID_TYPE:
>> @@ -7347,6 +7353,14 @@ build_array_type_1 (tree elt_type, tree index_type, bool typeless_storage,
>>   TYPE_DOMAIN (t) = index_type;
>>   TYPE_ADDR_SPACE (t) = TYPE_ADDR_SPACE (elt_type);
>>   TYPE_TYPELESS_STORAGE (t) = typeless_storage;
>> +
>> +  /* Set TYPE_STRUCTURAL_EQUALITY_P.  */
>> +  if (set_canonical
>> +      && (TYPE_STRUCTURAL_EQUALITY_P (elt_type)
>> +      || (index_type && TYPE_STRUCTURAL_EQUALITY_P (index_type))
>> +      || in_lto_p))
>> +    SET_TYPE_STRUCTURAL_EQUALITY (t);
>> +
>>   layout_type (t);
>> 
>>   if (shared)
>> @@ -7363,7 +7377,7 @@ build_array_type_1 (tree elt_type, tree index_type, bool typeless_storage,
>>       if (TYPE_STRUCTURAL_EQUALITY_P (elt_type)
>>      || (index_type && TYPE_STRUCTURAL_EQUALITY_P (index_type))
>>      || in_lto_p)
>> -    SET_TYPE_STRUCTURAL_EQUALITY (t);
>> +    gcc_unreachable ();
>>       else if (TYPE_CANONICAL (elt_type) != elt_type
>>           || (index_type && TYPE_CANONICAL (index_type) != index_type))
>>    TYPE_CANONICAL (t)
>> @@ -7510,21 +7524,25 @@ build_function_type (tree value_type, tree arg_types,
>>       TYPE_NO_NAMED_ARGS_STDARG_P (t) = 1;
>>     }
>> 
>> -  /* If we already have such a type, use the old one.  */
>> -  hashval_t hash = type_hash_canon_hash (t);
>> -  tree probe_type = t;
>> -  t = type_hash_canon (hash, t);
>> -  if (t != probe_type)
>> -    return t;
>> -
>>   /* Set up the canonical type. */
>>   any_structural_p   = TYPE_STRUCTURAL_EQUALITY_P (value_type);
>>   any_noncanonical_p = TYPE_CANONICAL (value_type) != value_type;
>>   canon_argtypes = maybe_canonicalize_argtypes (arg_types,
>>                        &any_structural_p,
>>                        &any_noncanonical_p);
>> +  /* Set TYPE_STRUCTURAL_EQUALITY_P early.  */
>>   if (any_structural_p)
>>     SET_TYPE_STRUCTURAL_EQUALITY (t);
>> +
>> +  /* If we already have such a type, use the old one.  */
>> +  hashval_t hash = type_hash_canon_hash (t);
>> +  tree probe_type = t;
>> +  t = type_hash_canon (hash, t);
>> +  if (t != probe_type)
>> +    return t;
>> +
>> +  if (any_structural_p)
>> +    gcc_assert (TYPE_STRUCTURAL_EQUALITY_P (t));
>>   else if (any_noncanonical_p)
>>     TYPE_CANONICAL (t) = build_function_type (TYPE_CANONICAL (value_type),
>>                          canon_argtypes);
>> @@ -7667,13 +7685,6 @@ build_method_type_directly (tree basetype,
>>   argtypes = tree_cons (NULL_TREE, ptype, argtypes);
>>   TYPE_ARG_TYPES (t) = argtypes;
>> 
>> -  /* If we already have such a type, use the old one.  */
>> -  hashval_t hash = type_hash_canon_hash (t);
>> -  tree probe_type = t;
>> -  t = type_hash_canon (hash, t);
>> -  if (t != probe_type)
>> -    return t;
>> -
>>   /* Set up the canonical type. */
>>   any_structural_p
>>     = (TYPE_STRUCTURAL_EQUALITY_P (basetype)
>> @@ -7684,8 +7695,20 @@ build_method_type_directly (tree basetype,
>>   canon_argtypes = maybe_canonicalize_argtypes (TREE_CHAIN (argtypes),
>>                        &any_structural_p,
>>                        &any_noncanonical_p);
>> +
>> +  /* Set TYPE_STRUCTURAL_EQUALITY_P early.  */
>>   if (any_structural_p)
>>     SET_TYPE_STRUCTURAL_EQUALITY (t);
>> +
>> +  /* If we already have such a type, use the old one.  */
>> +  hashval_t hash = type_hash_canon_hash (t);
>> +  tree probe_type = t;
>> +  t = type_hash_canon (hash, t);
>> +  if (t != probe_type)
>> +    return t;
>> +
>> +  if (any_structural_p)
>> +    gcc_assert (TYPE_STRUCTURAL_EQUALITY_P (t));
>>   else if (any_noncanonical_p)
>>     TYPE_CANONICAL (t)
>>       = build_method_type_directly (TYPE_CANONICAL (basetype),
>> @@ -7726,6 +7749,9 @@ build_offset_type (tree basetype, tree type)
>> 
>>   TYPE_OFFSET_BASETYPE (t) = TYPE_MAIN_VARIANT (basetype);
>>   TREE_TYPE (t) = type;
>> +  if (TYPE_STRUCTURAL_EQUALITY_P (basetype)
>> +      || TYPE_STRUCTURAL_EQUALITY_P (type))
>> +    SET_TYPE_STRUCTURAL_EQUALITY (t);
>> 
>>   /* If we already have such a type, use the old one.  */
>>   hashval_t hash = type_hash_canon_hash (t);
>> @@ -7741,7 +7767,7 @@ build_offset_type (tree basetype, tree type)
>>     {
>>       if (TYPE_STRUCTURAL_EQUALITY_P (basetype)
>>      || TYPE_STRUCTURAL_EQUALITY_P (type))
>> -    SET_TYPE_STRUCTURAL_EQUALITY (t);
>> +    gcc_unreachable ();
>>       else if (TYPE_CANONICAL (TYPE_MAIN_VARIANT (basetype)) != basetype
>>           || TYPE_CANONICAL (type) != type)
>>    TYPE_CANONICAL (t)
>> @@ -7770,6 +7796,8 @@ build_complex_type (tree component_type, bool named)
>>   tree probe = make_node (COMPLEX_TYPE);
>> 
>>   TREE_TYPE (probe) = TYPE_MAIN_VARIANT (component_type);
>> +  if (TYPE_STRUCTURAL_EQUALITY_P (TREE_TYPE (probe)))
>> +    SET_TYPE_STRUCTURAL_EQUALITY (probe);
>> 
>>   /* If we already have such a type, use the old one.  */
>>   hashval_t hash = type_hash_canon_hash (probe);
>> @@ -7781,11 +7809,10 @@ build_complex_type (tree component_type, bool named)
>>     out the type.  We need to check the canonicalization and
>>     maybe set the name.  */
>>       gcc_checking_assert (COMPLETE_TYPE_P (t)
>> -               && !TYPE_NAME (t)
>> -               && TYPE_CANONICAL (t) == t);
>> +               && !TYPE_NAME (t));
>> 
>>       if (TYPE_STRUCTURAL_EQUALITY_P (TREE_TYPE (t)))
>> -    SET_TYPE_STRUCTURAL_EQUALITY (t);
>> +    ;
>>       else if (TYPE_CANONICAL (TREE_TYPE (t)) != TREE_TYPE (t))
>>    TYPE_CANONICAL (t)
>>      = build_complex_type (TYPE_CANONICAL (TREE_TYPE (t)), named);
>