Re: [PATCH] Add a new type attribute always_alias (PR79671)

[Richard Biener <rguenther@suse.de>]

On Thu, 6 Apr 2017, Bernd Edlinger wrote:

> On 04/06/17 09:47, Richard Biener wrote:
> > On Wed, 5 Apr 2017, Bernd Edlinger wrote:
> >
> >> On 04/05/17 19:22, Bernd Edlinger wrote:
> >>> On 04/05/17 18:08, Jakub Jelinek wrote:
> >>>
> >>> Yes, exactly.  I really want to reach the deadline for gcc-7.
> >>> Fixing the name is certainly the most important first step,
> >>> and if everybody agrees on "typeless_storage", for the name
> >>> I can start with adjusting the name, and look into how
> >>> to use a spare type-flag that should be a mechanical change.
> >>>
> >>
> >> Jakub, I just renamed the attribute and reworked the patch
> >> as you suggested, reg-testing is not yet completed, but
> >> it looks good so far.  I also added a few more tests.
> >>
> >> I have changed the documentation as Richi suggested, but
> >> I am not too sure what to say here.
> >
> > The alias.c changes are not sufficient.  I think what you want is
> > sth like
> >
> > Index: gcc/alias.c
> > ===================================================================
> > --- gcc/alias.c	(revision 246678)
> > +++ gcc/alias.c	(working copy)
> > @@ -136,6 +136,9 @@ struct GTY(()) alias_set_entry {
> >    bool is_pointer;
> >    /* Nonzero if is_pointer or if one of childs have has_pointer set.  */
> >    bool has_pointer;
> > +  /* Nonzero if we have a child serving as typeless storage (or are
> > +     such storage ourselves).  */
> > +  bool has_typeless_storage;
> >
> >    /* The children of the alias set.  These are not just the immediate
> >       children, but, in fact, all descendants.  So, if we have:
> > @@ -419,7 +422,8 @@ alias_set_subset_of (alias_set_type set1
> >    /* Check if set1 is a subset of set2.  */
> >    ase2 = get_alias_set_entry (set2);
> >    if (ase2 != 0
> > -      && (ase2->has_zero_child
> > +      && (ase2->has_typeless_storage
> > +	  || ase2->has_zero_child
> >  	  || (ase2->children && ase2->children->get (set1))))
> >      return true;
> >
> 
> I think get_alias_set(t) will return 0 for typeless_storage
> types, and therefore has_zero_child will be set anyway.
> I think both mean the same thing in the end, but it depends on
> what typeless_storage should actually mean, and we have
> not yet the same idea about it.

But has_zero_child does not do what we like it to because otherwise
in the PR using the char[] array member would have worked!

has_zero_child doesn't do that on purpose of course, but this means
returing alias-set zero for the typeless storage _member_ doesn't
suffice.

> > @@ -825,6 +829,7 @@ init_alias_set_entry (alias_set_type set
> >    ase->has_zero_child = false;
> >    ase->is_pointer = false;
> >    ase->has_pointer = false;
> > +  ase->has_typeless_storage = false;
> >    gcc_checking_assert (!get_alias_set_entry (set));
> >    (*alias_sets)[set] = ase;
> >    return ase;
> > @@ -955,6 +960,7 @@ get_alias_set (tree t)
> >       Just be pragmatic here and make sure the array and its element
> >       type get the same alias set assigned.  */
> >    else if (TREE_CODE (t) == ARRAY_TYPE
> > +	   && ! TYPE_TYPELESS_STORAGE (t)
> >  	   && (!TYPE_NONALIASED_COMPONENT (t)
> >  	       || TYPE_STRUCTURAL_EQUALITY_P (t)))
> >      set = get_alias_set (TREE_TYPE (t));
> > @@ -1094,6 +1100,15 @@ get_alias_set (tree t)
> >
> >    TYPE_ALIAS_SET (t) = set;
> >
> > +  if (TREE_CODE (t) == ARRAY_TYPE
> > +      && TYPE_TYPELESS_STORAGE (t))
> > +    {
> > +      alias_set_entry *ase = get_alias_set_entry (set);
> > +      if (!ase)
> > +	ase = init_alias_set_entry (set);
> > +      ase->has_typeless_storage = true;
> > +    }
> > +
> >    /* If this is an aggregate type or a complex type, we must record any
> >       component aliasing information.  */
> >    if (AGGREGATE_TYPE_P (t) || TREE_CODE (t) == COMPLEX_TYPE)
> > @@ -1173,6 +1188,8 @@ record_alias_subset (alias_set_type supe
> >  	    superset_entry->has_zero_child = true;
> >            if (subset_entry->has_pointer)
> >  	    superset_entry->has_pointer = true;
> > +	  if (subset_entry->has_typeless_storage)
> > +	    superset_entry->has_typeless_storage = true;
> >
> >  	  if (subset_entry->children)
> >  	    {
> >
> >
> > please also restrict TYPE_TYPELESS_STORAGE to ARRAY_TYPEs (otherwise
> > more complications will arise).
> >
> > Index: gcc/cp/class.c
> > ===================================================================
> > --- gcc/cp/class.c      (revision 246678)
> > +++ gcc/cp/class.c      (working copy)
> > @@ -2083,7 +2083,8 @@ fixup_attribute_variants (tree t)
> >    tree attrs = TYPE_ATTRIBUTES (t);
> >    unsigned align = TYPE_ALIGN (t);
> >    bool user_align = TYPE_USER_ALIGN (t);
> > -  bool may_alias = lookup_attribute ("may_alias", attrs);
> > +  bool may_alias = TYPE_TYPELESS_STORAGE (t)
> > +                  || lookup_attribute ("may_alias", attrs);
> >
> >    if (may_alias)
> >      fixup_may_alias (t);
> > @@ -7345,6 +7348,12 @@ finish_struct_1 (tree t)
> >       the class or perform any other required target modifications.  */
> >    targetm.cxx.adjust_class_at_definition (t);
> >
> > +  if (cxx_dialect >= cxx1z && cxx_type_contains_byte_buffer (t))
> > +    {
> > +      TYPE_TYPELESS_STORAGE (t) = 1;
> > +      fixup_attribute_variants (t);
> > ...
> >
> > I don't think you need all this given alias.c only looks at
> > TYPE_MAIN_VARIANTs.
> 
> I wanted to be able to declare a int __attribute__((typeless_storage))
> as in the test case, and the sample in the spec.  And that
> information is not in the TYPE_MAIN_VARIANT.  Therefore I look for
> typeless_storage before "t = TYPE_MAIN_VARIANT (t)".

As I said I believe this is a useless feature.  If you want something
typeless then the underlying type doesn't matter so we can as well
force it to be an array of char.  Makes our live simpler.  And
even makes the code portable to compilers that treat arrays of char
conservatively.

> >
> > Index: gcc/cp/decl.c
> > ===================================================================
> > --- gcc/cp/decl.c       (revision 246678)
> > +++ gcc/cp/decl.c       (working copy)
> > @@ -14081,10 +14081,11 @@ start_enum (tree name, tree enumtype, tree
> > underly
> >           enumtype = pushtag (name, enumtype, /*tag_scope=*/ts_current);
> >
> >           /* std::byte aliases anything.  */
> > -         if (enumtype != error_mark_node
> > +         if (cxx_dialect >= cxx1z
> > +             && enumtype != error_mark_node
> >               && TYPE_CONTEXT (enumtype) == std_node
> >               && !strcmp ("byte", TYPE_NAME_STRING (enumtype)))
> > -           TYPE_ALIAS_SET (enumtype) = 0;
> > +           TYPE_TYPELESS_STORAGE (enumtype) = 1;
> >         }
> >        else
> >
> > not needed (but also not sufficient - you need to handle arrays of
> > byte somewhere).
> 
> See cxx_type_contains_byte_buffer: this function looks recursively into
> structures and unions, and returns the information if the beast
> contains an array of unsigned char or std::byte.

But with a properly designed middle-end feature that's not needed.

There's technically no reason to pessimize TBAA for anything but
the typeless storage member of a structure.

> >
> > @@ -1491,6 +1491,7 @@ struct GTY(()) tree_type_common {
> >    unsigned needs_constructing_flag : 1;
> >    unsigned transparent_aggr_flag : 1;
> >    unsigned restrict_flag : 1;
> > +  unsigned typeless_storage_flag : 1;
> >    unsigned contains_placeholder_bits : 2;
> >
> >    ENUM_BITFIELD(machine_mode) mode : 8;
> >
> > bits are grouped in groups of 8 bits, this breaks it.
> >
> 
> Oh..., does this explain the problems that I had with this version???

No, just "cosmetics".

> > @@ -8041,7 +8041,8 @@ build_pointer_type_for_mode (tree to_type, machine
> >
> >    /* If the pointed-to type has the may_alias attribute set, force
> >       a TYPE_REF_CAN_ALIAS_ALL pointer to be generated.  */
> > -  if (lookup_attribute ("may_alias", TYPE_ATTRIBUTES (to_type)))
> > +  if (TYPE_TYPELESS_STORAGE (to_type)
> > +      || lookup_attribute ("may_alias", TYPE_ATTRIBUTES (to_type)))
> >      can_alias_all = true;
> >
> >    /* In some cases, languages will have things that aren't a POINTER_TYPE
> > @@ -8110,7 +8111,8 @@ build_reference_type_for_mode (tree to_type, machi
> >
> >    /* If the pointed-to type has the may_alias attribute set, force
> >       a TYPE_REF_CAN_ALIAS_ALL pointer to be generated.  */
> > -  if (lookup_attribute ("may_alias", TYPE_ATTRIBUTES (to_type)))
> > +  if (TYPE_TYPELESS_STORAGE (to_type)
> > +      || lookup_attribute ("may_alias", TYPE_ATTRIBUTES (to_type)))
> >      can_alias_all = true;
> >
> >    /* In some cases, languages will have things that aren't a
> >
> > not needed.
> >
> 
> You mean, because the get_alias_set (to_type) will be 0 anyways,
> and can_alias_all wont change the semantic?

Well, typeless_storage and may_alias are something different.  If
you require the above then your implementation of typeless_storage
is broken.

Richard.

> 
> Bernd.
> 
> > +/* Nonzero if the type should behave like a character type
> > +   with respect to aliasing sementics.  */
> > +#define TYPE_TYPELESS_STORAGE(NODE) \
> > +  (TYPE_CHECK (NODE)->type_common.typeless_storage_flag)
> >
> > ARRAY_TYPE_CHECK (NODE)->
> >
> > Richard.
> >
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)