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

[Martin Uecker <uecker@tugraz.at>]

Am Freitag, dem 03.05.2024 um 18:23 +0200 schrieb Richard Biener:
> 
> > 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.

So build_function_type when called recursively for creating
a TYPE_CANONICAL found some type with TYPE_STRUCTURAL_EQUALITY.

And the plan is to separate those in the hash table so that this
cannot happen?

Couldn't we instead lazily update TYPE_CANONICAL at this point? 


Martin

> 
> 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);
> >