Re: [PATCH, rs6000] Fix alignment of non-Altivec vector struct fields

[David Edelsohn <dje dot gcc at gmail dot com>]

On Wed, Jul 9, 2014 at 12:06 PM, Ulrich Weigand <uweigand@de.ibm.com> wrote:
> Hello,
>
> running the ABI compatibility test suite against another compiler showed
> strange effects caused by code in ADJUST_FIELD_ALIGN on rs6000:
>
> #define ADJUST_FIELD_ALIGN(FIELD, COMPUTED) \
>   ((TARGET_ALTIVEC && TREE_CODE (TREE_TYPE (FIELD)) == VECTOR_TYPE)    \
>    ? 128                                                               \
>    : (TARGET_64BIT                                                     \
>       && TARGET_ALIGN_NATURAL == 0                                     \
>       && TYPE_MODE (strip_array_types (TREE_TYPE (FIELD))) == DFmode)  \
>    ? MIN ((COMPUTED), 32)                                               \
>    : (COMPUTED))
>
> The first test for VECTOR_TYPE if TARGET_ALTIVEC causes the trouble.
> This has been present (in slight variations) ever since Altivec support
> was first merged by Aldy Hernandez back in 2001:
> https://gcc.gnu.org/ml/gcc-cvs/2001-11/msg00148.html
>
> Note that at this time, both ADJUST_FIELD_ALIGN and ROUND_TYPE_ALIGN
> contained that check.  The check was later removed from ROUND_TYPE_ALIGN
> in the following patch intended to fix PR 17961:
> https://gcc.gnu.org/ml/gcc-patches/2005-06/msg00920.html
>
> The check has no effect in most usual cases anyway, since Altivec vectors
> already carry a default alignment of 16 bytes.  However, the check *does*
> affect non-Altivec vectors created via attribute((vector_size)) with a
> size != 16.  When any of those is used as element of a struct, GCC on
> PowerPC will ignore the normal alignment and always force 16 byte alignment.
> (An explicit attribute((aligned)) however, is still respected.)  This
> behavior seems to have been unintended.
>
> In off-line discussions, we came to the conclusion that this check
> should have been removed from ADJUST_FIELD_ALIGN at the same time
> as it was removed from ROUND_TYPE_ALIGN.  This patch implements this.
>
> Note that the check has been copied over time into the instances of
> ADJUST_FIELD_ALIGN in sysv4.h, linux64.h, and freebsd64.h.  For
> consistency, the patch removes the check in all these places.
>
> Tested on powerpc64-linux and powerp64le-linux; built a cross-cc1
> for powerpc64-freebsd6.
>
> OK for mainline?
> [ It may also be useful to backport to the currently open branches,
> but the bug was already present in many releases that are no longer
> maintained ... ]

This is okay with me.

Do you want to explicitly ask rth if he has any further insight?

Thanks, David