Am Freitag, dem 03.05.2024 um 18:23 +0200 schrieb Richard Biener:
>
> > Am 03.05.2024 um 17:33 schrieb Martin Uecker <uec...@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);
> >
