On Wed, Mar 24, 2021 at 3:46 PM David Edelsohn <dje....@gmail.com> wrote:
>
> On Wed, Mar 24, 2021 at 11:30 AM Iain Sandoe <idsan...@googlemail.com> wrote:
> >
> > David Edelsohn via Gcc-patches <gcc-patches@gcc.gnu.org> wrote:
> >
> > > On Wed, Mar 24, 2021 at 3:51 AM Richard Biener
> > > <richard.guent...@gmail.com> wrote:
> > >> On Wed, Mar 24, 2021 at 3:03 AM David Edelsohn <dje....@gmail.com> wrote:
> > >>> On Mon, Mar 22, 2021 at 4:10 AM Richard Biener
> > >>> <richard.guent...@gmail.com> wrote:
> > >>>
> > >>>> 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?
> > >>>
> > >>> rs6000_special_round_type_align and rs6000_special_adjust_field_align
> > >>> both can have early exits to handle some easy cases. Thanks for
> > >>> pointing that out.
> > >>>
> > >>> struct A { struct { struct { ... { double x; } } } };
> > >>> struct B { struct A; struct A; struct A; struct A; ...; };
> > >>>
> > >>> is a particularly ugly situation.
> > >>>
> > >>> When I originally had implemented this in GCC, the recursive nature of
> > >>> the requirement was not clear. Changing the alignment for a type
> > >>> (struct) in two different contexts (bare versus member) is bizarre,
> > >>> but that is what IBM XL compilers implement.
> > >>>
> > >>> If this becomes a time-sink for for in real use cases, one could
> > >>> create a side cache of the type with the previously calculated
> > >>> alignment value. Or are there some preferred, available flag bit in
> > >>> the tree that can record that the type alignment has been checked and
> > >>> either use TYPE_ALIGN or use 32? Maybe protected_flag or
> > >>> side_effects_flag or nothrow_flag?
> > >>
> > >> I think type alignment is finalized once a type is laid out which means
> > >> checking COMPLETE_TYPE_P (type) (see layout_type()s early out).
> > >
> > > Yes, this primarily is a problem for fields, not types.
> > >
> > >> But then to lay out B we still need, for each field of type A, look
> > >> recursively at the first "real" member and check its field alignment?
> > >> While we know that A is laid out, it's alignment as-a-field is still
> > >> unknown and is not cached anywhere, right?
> > >
> > > Correct. The alignment of the bare type is set, but there is no
> > > separate information of its type (alignment) as a field.
> > >
> > >> So while early outs (as I suggested using some bounds on the types
> > >> alignment) are possible the worst case will still be present, and indeed,
> > >> caching the alignment as-a-field somewhere is the only way to "fix"
> > >> this :/ (but I also guess it likely doesn't matter in practice ...)
> > >
> > > The record alignment can exit if the proposed alignment is >=64 and
> > > the field alignment can exit if the proposed alignment is <=32.
> > >
> > > I guess that I also could test if the record or field type mode is
> > > DFmode, DCmode or BLKmode, but most are BLKmode and I don't know if
> > > that catches many more cases.
> > >
> > >> If this particular case is always overriding field alignment with a
> > >> special
> > >> value then a single bit would be enough to do this and I guess a
> > >> (new?) target hook called at layout_type time to compute such properties
> > >> would be OK (to avoid requiring another bit to see whether the bit was
> > >> already computed). There's also the possibility to use a target specific
> > >> attribute to store such information.
> > >
> > > How would the new target hook know if the value already was computed?
> > >
> > >> I guess doing some early outs should avoid most real-world slowdowns.
> > >> Btw, does XLC "behave" with the problematic case?
> > >
> > > XL produces the correct result. I haven't specifically tested for
> > > quadratic behavior.
> > >
> > > The new LLVM support for AIX adds some additional members to the
> > > common part of the LLVM class that describes alignment layout to
> > > distinguish between the different types of alignment. The information
> > > is recorded once and not recomputed.
> > >
> > > I have been bootstrapping with variants of the patch for over a week
> > > and haven't noticed any particular change in bootstrap time, either
> > > before or after the early exits.
> > >
> > > One possibility is to commit the current patch and see if anyone
> > > complains about compile time performance.
> >
> > It seems that this might be converging with what Darwin has to do - except
> > Darwin has to deal with long long as well as double.
> >
> > Darwin also has to deal with user-mandated (attribute-wise) alignment or
> > packed cases; not required for AIX?
>
> Hi, Ian
>
> Yes, the AIX adjustment to types is looking more like Darwin. I
> already had added support for packed.
>
> One difference and where Darwin seems to make more sense, is that it
> didn't revert the alignment of structs/classes when used as a field.
>
> Darwin uses AGGREGATE_TYPE_P, which includes ARRAY_TYPE in addition to
> RECORD_TYPE. Do you understand the interaction between that and
> stripping arrays? What is the implication of accepting ARRAY_TYPE
> when walking the fields?
With the early exit for packed structures, it is not practical to
refactor the code. The following is the revised version of the patch.
Thanks, David
diff --git a/gcc/config/rs6000/aix.h b/gcc/config/rs6000/aix.h
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 616dae35bae..f5c71d7925b 100644
--- a/gcc/config/rs6000/rs6000.c
+++ b/gcc/config/rs6000/rs6000.c
@@ -7853,7 +7853,52 @@ rs6000_special_adjust_field_align_p (tree type, unsigned
int computed)
return false;
}
-/* AIX increases natural record alignment to doubleword if the first
+/* AIX word-aligns FP doubles but doubleword-aligns 64-bit ints. */
+
+unsigned int
+rs6000_special_adjust_field_align (tree type, unsigned int computed)
+{
+ if (computed <= 32)
+ return computed;
+
+ /* Strip initial arrays. */
+ if (TREE_CODE (type) == ARRAY_TYPE)
+ while (TREE_CODE (type) == ARRAY_TYPE)
+ type = TREE_TYPE (type);
+
+ /* If RECORD or UNION, recursively find the first field. */
+ while (AGGREGATE_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)
+ break;
+
+ /* A packed field does not contribute any extra alignment. */
+ if (DECL_PACKED (field))
+ return computed;
+
+ type = TREE_TYPE (field);
+
+ /* Strip arrays. */
+ while (TREE_CODE (type) == ARRAY_TYPE)
+ type = TREE_TYPE (type);
+ }
+
+ if (! AGGREGATE_TYPE_P (type) && 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 innermost first
field is an FP double while the FP fields remain word aligned. */
unsigned int
@@ -7861,24 +7906,38 @@ 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 (TYPE_PACKED (type) || align >= 64)
+ return align;
- if (field != NULL && field != type)
+ /* If RECORD or UNION, recursively find the first field. */
+ do
{
+ 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)
+ break;
+
+ /* A packed field does not contribute any extra alignment. */
+ if (DECL_PACKED (field))
+ return align;
+
type = TREE_TYPE (field);
+
+ /* Strip arrays. */
while (TREE_CODE (type) == ARRAY_TYPE)
type = TREE_TYPE (type);
+ } while (AGGREGATE_TYPE_P (type));
- if (type != error_mark_node
- && (TYPE_MODE (type) == DFmode || TYPE_MODE (type) == DCmode))
- align = MAX (align, 64);
- }
+ if (! AGGREGATE_TYPE_P (type) && type != error_mark_node
+ && (TYPE_MODE (type) == DFmode || TYPE_MODE (type) == DCmode))
+ align = MAX (align, 64);
return align;
}
