[PATCH] AIX struct alignment (PR 99557)

[Richard Biener]

On Mon, Mar 22, 2021 at 9:06 AM Richard Biener
<richard.guenther@gmail.com> wrote:
>
> On Mon, Mar 22, 2021 at 3:04 AM David Edelsohn via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
> >
> > The AIX power alignment rules apply the natural alignment of the
> > "first member" if it is of a floating-point data type (or is an aggregate
> > whose recursively "first" member or element is such a type). The alignment
> > associated with these types for subsequent members use an alignment value
> > where the floating-point data type is considered to have 4-byte alignment.
> >
> > GCC had been stripping array type but had not recursively looked
> > within structs and unions.  This also applies to classes and
> > subclasses and, therefore, becomes more prominent with C++.
> >
> > For example,
> >
> > struct A {
> >   double x[2];
> >   int y;
> > };
> > struct B {
> >   int i;
> >   struct A a;
> > };
> >
> > struct A has double-word alignment when referenced independently, but
> > word alignment and offset within struct B despite the alignment of
> > struct A.  If struct A were the first member of struct B, struct B
> > would have double-word alignment.  One must search for the innermost
> > first member to increase the alignment if double and then search for
> > the innermost first member to reduce the alignment if the TYPE had
> > double-word alignment solely because the innermost first member was
> > double.
> >
> > This patch recursively looks through the first member to apply the
> > double-word alignment to the struct / union as a whole and to apply
> > the word alignment to the struct or union as a member within a struct
> > or union.
> >
> > This is an ABI change for GCCon AIX, but GCC on AIX had not correctly
> > implemented the AIX ABI and had not been compatible with the IBM XL
> > compiler.
> >
> > If anyone can double-check that the patch walks the fields correctly
> > and handles the error conditions correctly, it would be appreciated.
> >
> > Bootstrapped on powerpc-ibm-aix7.2.3.0.
> >
> > Thanks, David
> >
> >             * config/rs6000/aix.h (ADJUST_FIELD_ALIGN): Call function.
> >             * config/rs6000/rs6000-protos.h: Declare.
> >             * config/rs6000/rs6000.c: Define.
> >
> > index 2db50c8007f..7fccb31307b 100644
> > --- a/gcc/config/rs6000/aix.h
> > +++ b/gcc/config/rs6000/aix.h
> > @@ -223,10 +223,8 @@
> >  /* This now supports a natural alignment mode.  */
> >  /* AIX word-aligns FP doubles but doubleword-aligns 64-bit ints.  */
> >  #define ADJUST_FIELD_ALIGN(FIELD, TYPE, COMPUTED) \
> > -  ((TARGET_ALIGN_NATURAL == 0                                          \
> > -    && (TYPE_MODE (strip_array_types (TYPE)) == DFmode                 \
> > -       || TYPE_MODE (strip_array_types (TYPE)) == DCmode))             \
> > -   ? MIN ((COMPUTED), 32)                                              \
> > +  (TARGET_ALIGN_NATURAL == 0                                           \
> > +   ? rs6000_special_adjust_field_align (TYPE, COMPUTED)
> > \
> >     : (COMPUTED))
> >
> >  /* AIX increases natural record alignment to doubleword if the first
> > diff --git a/gcc/config/rs6000/rs6000-protos.h
> > b/gcc/config/rs6000/rs6000-protos.h
> > index 203660b0a78..c44fd3d0263 100644
> > --- a/gcc/config/rs6000/rs6000-protos.h
> > +++ b/gcc/config/rs6000/rs6000-protos.h
> > @@ -227,6 +227,7 @@ address_is_prefixed (rtx addr,
> >  #ifdef TREE_CODE
> >  extern unsigned int rs6000_data_alignment (tree, unsigned int, enum data_align)
> > ;
> >  extern bool rs6000_special_adjust_field_align_p (tree, unsigned int);
> > +extern unsigned int rs6000_special_adjust_field_align (tree, unsigned int);
> >  extern unsigned int rs6000_special_round_type_align (tree, unsigned int,
> >                                                      unsigned int);
> >  extern unsigned int darwin_rs6000_special_round_type_align (tree, unsigned int,
> > diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c
> > index 712dd1c460b..eed51e8d4a2 100644
> > --- a/gcc/config/rs6000/rs6000.c
> > +++ b/gcc/config/rs6000/rs6000.c
> > @@ -7856,6 +7856,41 @@ rs6000_special_adjust_field_align_p (tree type, unsigned
> > int computed)
> >    return false;
> >  }
> >
> > +/* AIX word-aligns FP doubles but doubleword-aligns 64-bit ints.  */
> > +
> > +unsigned int
> > +rs6000_special_adjust_field_align (tree type, unsigned int computed)
> > +{
> > +  /* If RECORD or UNION, recursively find the first field. */
> > +  while (TREE_CODE (type) == RECORD_TYPE
> > +        || TREE_CODE (type) == UNION_TYPE
> > +        || TREE_CODE (type) == QUAL_UNION_TYPE)
>
> RECORD_OR_UNION_TYPE_P (type)
>
> > +    {
> > +      tree field = TYPE_FIELDS (type);
> > +
> > +      /* Skip all non field decls */
> > +      while (field != NULL
> > +            && (TREE_CODE (field) != FIELD_DECL
> > +                || DECL_FIELD_ABI_IGNORED (field)))
> > +       field = DECL_CHAIN (field);
> > +
> > +      if (field != NULL && field != type)
>
> when is the field (a FIELD_DECL) ever pointer equal to the containing
> record type?  Unless you ment to check for sth specific I suggest
> to drop field != type.
>
> > +       type = TREE_TYPE (field);
> > +      else
> > +       break;
> > +    }
> > +
> > +  /* Strip arrays.  */
> > +  while (TREE_CODE (type) == ARRAY_TYPE)
> > +    type = TREE_TYPE (type);
> > +
> > +  if (type != error_mark_node
> > +      && (TYPE_MODE (type) == DFmode || TYPE_MODE (type) == DCmode))
> > +    computed = MIN (computed, 32);
> > +
> > +  return computed;
> > +}
> > +
> >  /* AIX increases natural record alignment to doubleword if the first
> >     field is an FP double while the FP fields remain word aligned.  */
> > @@ -7864,25 +7899,33 @@ rs6000_special_round_type_align (tree type,unsigned int
> >  computed,
> >                                  unsigned int specified)
> >  {
> >    unsigned int align = MAX (computed, specified);
> > -  tree field = TYPE_FIELDS (type);
> >
> > -  /* Skip all non field decls */
> > -  while (field != NULL
> > -        && (TREE_CODE (field) != FIELD_DECL
> > -            || DECL_FIELD_ABI_IGNORED (field)))
> > -    field = DECL_CHAIN (field);
> > -
> > -  if (field != NULL && field != type)
> > +  /* If RECORD or UNION, recursively find the first field. */
> > +  while (TREE_CODE (type) == RECORD_TYPE
> > +        || TREE_CODE (type) == UNION_TYPE
> > +        || TREE_CODE (type) == QUAL_UNION_TYPE)
>
> same as above
>
> >      {
> > -      type = TREE_TYPE (field);
> > -      while (TREE_CODE (type) == ARRAY_TYPE)
> > -       type = TREE_TYPE (type);
> > +      tree field = TYPE_FIELDS (type);
> > +
> > +      /* Skip all non field decls */
> > +      while (field != NULL
> > +            && (TREE_CODE (field) != FIELD_DECL
> > +                || DECL_FIELD_ABI_IGNORED (field)))
> > +       field = DECL_CHAIN (field);
> >
> > -      if (type != error_mark_node
> > -         && (TYPE_MODE (type) == DFmode || TYPE_MODE (type) == DCmode))
> > -       align = MAX (align, 64);
> > +      if (field != NULL && field != type)
> > +       type = TREE_TYPE (field);
> > +      else
> > +       break;
> >      }
> >
> > +    while (TREE_CODE (type) == ARRAY_TYPE)
> > +      type = TREE_TYPE (type);
> > +
> > +    if (type != error_mark_node
> > +       && (TYPE_MODE (type) == DFmode || TYPE_MODE (type) == DCmode))
> > +      align = MAX (align, 64);
>
> it looks like the changes are duplicate and could see some factoring.

Oh, and for a type like

 struct { struct { struct { ... { double x; } } } } } };

the layout now looks quadratic in work (each field layout will look at
the nest rooted at it
up to the bottom).  It looks to me as we require(?) the field types to
be laid out and thus
at least an early out checking the type alignment to be >= 64 can work?

Richard.

> Richard.
>
> > +
> > +
> >    return align;
> >  }