This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH 1/2][ARM] PR/65956 AAPCS update for alignment attribute


On July 3, 2015 10:43:30 PM GMT+02:00, Richard Earnshaw <Richard.Earnshaw@foss.arm.com> wrote:
>On 03/07/15 19:24, Richard Biener wrote:
>> On July 3, 2015 6:11:13 PM GMT+02:00, Richard Earnshaw
><Richard.Earnshaw@foss.arm.com> wrote:
>>> On 03/07/15 16:26, Alan Lawrence wrote:
>>>> These include tests of structs, scalars, and vectors - only
>>>> general-purpose registers are affected by the ABI rules for
>>> alignment,
>>>> but we can restrict the vector test to use the base AAPCS.
>>>>
>>>> Prior to this patch, align2.c, align3.c and align_rec1.c were
>failing
>>>> (the latter showing an internal inconsistency, the first two merely
>>> that
>>>> GCC did not obey the new ABI).
>>>>
>>>> With this patch, the align_rec2.c fails, and also
>>>> gcc.c-torture/execute/20040709-1.c at -O0 only, both because of a
>>> latent
>>>> bug where we can emit strd/ldrd on an odd-numbered register in ARM
>>>> state, fixed by the second patch.
>>>>
>>>> gcc/ChangeLog:
>>>>
>>>>     * config/arm/arm.c (arm_needs_doubleword_align): Drop any outer
>>>>     alignment attribute, exploring one level down for aggregates.
>>>>
>>>> gcc/testsuite/ChangeLog:
>>>>
>>>>     * gcc.target/arm/aapcs/align1.c: New.
>>>>     * gcc.target/arm/aapcs/align_rec1.c: New.
>>>>     * gcc.target/arm/aapcs/align2.c: New.
>>>>     * gcc.target/arm/aapcs/align_rec2.c: New.
>>>>     * gcc.target/arm/aapcs/align3.c: New.
>>>>     * gcc.target/arm/aapcs/align_rec3.c: New.
>>>>     * gcc.target/arm/aapcs/align4.c: New.
>>>>     * gcc.target/arm/aapcs/align_rec4.c: New.
>>>>     * gcc.target/arm/aapcs/align_vararg1.c: New.
>>>>     * gcc.target/arm/aapcs/align_vararg2.c: New.
>>>>
>>>> arm_overalign_1.patch
>>>>
>>>>
>>>> diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
>>>> index
>>>
>04663999224c8c8eb8e2d10b0ec634db6ce5027e..ee57d30617a2f7e1cd63ca013fe5655a01027581
>>> 100644
>>>> --- a/gcc/config/arm/arm.c
>>>> +++ b/gcc/config/arm/arm.c
>>>> @@ -6020,8 +6020,17 @@ arm_init_cumulative_args (CUMULATIVE_ARGS
>>> *pcum, tree fntype,
>>>>  static bool
>>>>  arm_needs_doubleword_align (machine_mode mode, const_tree type)
>>>>  {
>>>> -  return (GET_MODE_ALIGNMENT (mode) > PARM_BOUNDARY
>>>> -	  || (type && TYPE_ALIGN (type) > PARM_BOUNDARY));
>>>> +  if (!type)
>>>> +    return PARM_BOUNDARY < GET_MODE_ALIGNMENT (mode);
>>>> +
>>>> +  if (!AGGREGATE_TYPE_P (type))
>>>> +    return TYPE_ALIGN (TYPE_MAIN_VARIANT (type)) > PARM_BOUNDARY;
>>>> +
>>>> +  for (tree field = TYPE_FIELDS (type); field; field = DECL_CHAIN
>>> (field))
>>>> +    if (DECL_ALIGN (field) > PARM_BOUNDARY)
>>>> +      return true;

I also believe this loop is equivalent to checking TYPE_ALIGN of the aggregate type?

I'll double check your wording in the abi document, but it seems to be unclear whether packed and not packed structs should be passed the same (considering layout differences).  OTOH the above function is only relevant for register passing? (Likewise the abi document changes?)

>> 
>> Is this behavior correct for unions or aggregates with record or
>union members?
>
>Yes, at least that was my intention.  It's an error in the wording of
>the proposed change, which I think should say "composite types" not
>"aggregate types".
>
>R.
>
>> 
>>>
>>> Technically this is incorrect since AGGREGATE_TYPE_P includes
>>> ARRAY_TYPE
>>> and ARRAY_TYPE doesn't have TYPE_FIELDS.  I doubt we could reach
>that
>>> case though (unless there's a language that allows passing arrays by
>>> value).
>>>
>>> For array types I think you need to check TYPE_ALIGN (TREE_TYPE
>>> (type)).
>>>
>>> R.
>>>
>>>> +  return false;
>>>>  }
>>>>  
>>>>  
>>>> diff --git a/gcc/testsuite/gcc.target/arm/aapcs/align1.c
>>> b/gcc/testsuite/gcc.target/arm/aapcs/align1.c
>>>> new file mode 100644
>>>> index
>>>
>0000000000000000000000000000000000000000..8981d57c3eaf0bd89d224bec79ff8a45627a0a89
>>>> --- /dev/null
>>>> +++ b/gcc/testsuite/gcc.target/arm/aapcs/align1.c
>>>> @@ -0,0 +1,29 @@
>>>> +/* Test AAPCS layout (alignment).  */
>>>> +
>>>> +/* { dg-do run { target arm_eabi } } */
>>>> +/* { dg-require-effective-target arm32 } */
>>>> +/* { dg-options "-O" } */
>>>> +
>>>> +#ifndef IN_FRAMEWORK
>>>> +#define TESTFILE "align1.c"
>>>> +
>>>> +typedef __attribute__((aligned (8))) int alignedint;
>>>> +
>>>> +alignedint a = 11;
>>>> +alignedint b = 13;
>>>> +alignedint c = 17;
>>>> +alignedint d = 19;
>>>> +alignedint e = 23;
>>>> +alignedint f = 29;
>>>> +
>>>> +#include "abitest.h"
>>>> +#else
>>>> +  ARG (alignedint, a, R0)
>>>> +  /* Attribute suggests R2, but we should use only natural
>>> alignment:  */
>>>> +  ARG (alignedint, b, R1)
>>>> +  ARG (alignedint, c, R2)
>>>> +  ARG (alignedint, d, R3)
>>>> +  ARG (alignedint, e, STACK)
>>>> +  /* Attribute would suggest STACK + 8 but should be ignored:  */
>>>> +  LAST_ARG (alignedint, f, STACK + 4)
>>>> +#endif
>>>> diff --git a/gcc/testsuite/gcc.target/arm/aapcs/align2.c
>>> b/gcc/testsuite/gcc.target/arm/aapcs/align2.c
>>>> new file mode 100644
>>>> index
>>>
>0000000000000000000000000000000000000000..992da53c606c793f25278152406582bb993719d2
>>>> --- /dev/null
>>>> +++ b/gcc/testsuite/gcc.target/arm/aapcs/align2.c
>>>> @@ -0,0 +1,30 @@
>>>> +/* Test AAPCS layout (alignment).  */
>>>> +
>>>> +/* { dg-do run { target arm_eabi } } */
>>>> +/* { dg-require-effective-target arm32 } */
>>>> +/* { dg-options "-O" } */
>>>> +
>>>> +#ifndef IN_FRAMEWORK
>>>> +#define TESTFILE "align2.c"
>>>> +
>>>> +/* The underlying struct here has alignment 4.  */
>>>> +typedef struct __attribute__((aligned (8)))
>>>> +  {
>>>> +    int x;
>>>> +    int y;
>>>> +  } overaligned;
>>>> +
>>>> +/* A couple of instances, at 8-byte-aligned memory locations.  */
>>>> +overaligned a = { 2, 3 };
>>>> +overaligned b = { 5, 8 };
>>>> +
>>>> +#include "abitest.h"
>>>> +#else
>>>> +  ARG (int, 7, R0)
>>>> +  /* Alignment should be 4.  */
>>>> +  ARG (overaligned, a, R1)
>>>> +  ARG (int, 9, R3)
>>>> +  ARG (int, 10, STACK)
>>>> +  /* Alignment should be 4.  */
>>>> +  LAST_ARG (overaligned, b, STACK + 4)
>>>> +#endif
>>>> diff --git a/gcc/testsuite/gcc.target/arm/aapcs/align3.c
>>> b/gcc/testsuite/gcc.target/arm/aapcs/align3.c
>>>> new file mode 100644
>>>> index
>>>
>0000000000000000000000000000000000000000..81ad3f587a95aae52ec601ce5a60b198e5351edf
>>>> --- /dev/null
>>>> +++ b/gcc/testsuite/gcc.target/arm/aapcs/align3.c
>>>> @@ -0,0 +1,42 @@
>>>> +/* Test AAPCS layout (alignment).  */
>>>> +
>>>> +/* { dg-do run { target arm_eabi } } */
>>>> +/* { dg-require-effective-target arm32 } */
>>>> +/* { dg-options "-O3" } */
>>>> +
>>>> +#ifndef IN_FRAMEWORK
>>>> +#define TESTFILE "align3.c"
>>>> +
>>>> +/* Struct will be aligned to 8.  */
>>>> +struct s
>>>> +  {
>>>> +    int x;
>>>> +    /* 4 bytes padding here.  */
>>>> +    __attribute__((aligned (8))) int y;
>>>> +    /* 4 bytes padding here.  */
>>>> +  };
>>>> +
>>>> +typedef struct s __attribute__((aligned (4))) underaligned;
>>>> +
>>>> +#define EXPECTED_STRUCT_SIZE 16
>>>> +extern void link_failure (void);
>>>> +int
>>>> +foo ()
>>>> +{
>>>> +  /* Optimization gets rid of this before linking.  */
>>>> +  if (sizeof (struct s) != EXPECTED_STRUCT_SIZE)
>>>> +    link_failure ();
>>>> +}
>>>> +
>>>> +underaligned a = { 1, 4 };
>>>> +underaligned b = { 9, 16 };
>>>> +
>>>> +#include "abitest.h"
>>>> +#else
>>>> +  ARG (int, 3, R0)
>>>> +  /* Object alignment is 8, so split between 2 regs and 8 on
>stack. 
>>> */
>>>> +  ARG (underaligned, a, R2)
>>>> +  ARG (int, 6, STACK + 8)
>>>> +  /* Object alignment is 8, so skip over STACK + 12.  */
>>>> +  LAST_ARG (underaligned, b, STACK + 16)
>>>> +#endif
>>>> diff --git a/gcc/testsuite/gcc.target/arm/aapcs/align4.c
>>> b/gcc/testsuite/gcc.target/arm/aapcs/align4.c
>>>> new file mode 100644
>>>> index
>>>
>0000000000000000000000000000000000000000..5535c55b8ac895ea31e468fd5474a71c232d2fea
>>>> --- /dev/null
>>>> +++ b/gcc/testsuite/gcc.target/arm/aapcs/align4.c
>>>> @@ -0,0 +1,29 @@
>>>> +/* Test AAPCS layout (alignment) - passing vectors in GPRs.  */
>>>> +
>>>> +/* { dg-do run { target arm_eabi } } */
>>>> +/* { dg-require-effective-target arm32 } */
>>>> +/* { dg-require-effective-target arm_neon_ok  } */
>>>> +/* { dg-options "-O" } */
>>>> +/* { dg-add-options arm_neon } */
>>>> +
>>>> +#ifndef IN_FRAMEWORK
>>>> +#define TESTFILE "align4.c"
>>>> +
>>>> +#define PCSATTR __attribute__((pcs("aapcs")))
>>>> +
>>>> +#include <arm_neon.h>
>>>> +
>>>> +typedef __attribute__((aligned (4))) int32x2_t unalignedvec;
>>>> +
>>>> +unalignedvec a = {11, 13};
>>>> +unalignedvec b = {17, 19};
>>>> +
>>>> +#include "abitest.h"
>>>> +#else
>>>> +  ARG (int, 2, R0)
>>>> +  /* Attribute suggests R1, but we should use natural alignment: 
>*/
>>>> +  ARG (unalignedvec, a, R2)
>>>> +  ARG (int, 6, STACK)
>>>> +  /* Attribute would suggest STACK + 4 but should be ignored:  */
>>>> +  LAST_ARG (unalignedvec, b, STACK + 8)
>>>> +#endif
>>>> diff --git a/gcc/testsuite/gcc.target/arm/aapcs/align_rec1.c
>>> b/gcc/testsuite/gcc.target/arm/aapcs/align_rec1.c
>>>> new file mode 100644
>>>> index
>>>
>0000000000000000000000000000000000000000..2e42baefb5877f28b763cc302fd4ef728fb3f72c
>>>> --- /dev/null
>>>> +++ b/gcc/testsuite/gcc.target/arm/aapcs/align_rec1.c
>>>> @@ -0,0 +1,36 @@
>>>> +/* Test AAPCS layout (alignment) for callee.  */
>>>> +
>>>> +/* { dg-do run { target arm_eabi } } */
>>>> +/* { dg-require-effective-target arm32 } */
>>>> +/* { dg-options "-O2 -fno-inline" } */
>>>> +
>>>> +extern void abort (void);
>>>> +
>>>> +typedef __attribute__((aligned (8))) int alignedint;
>>>> +
>>>> +alignedint a = 11;
>>>> +alignedint b = 13;
>>>> +alignedint c = 17;
>>>> +alignedint d = 19;
>>>> +alignedint e = 23;
>>>> +alignedint f = 29;
>>>> +
>>>> +void
>>>> +foo (alignedint r0, alignedint r1, alignedint r2, alignedint r3,
>>>> +     alignedint stack, alignedint stack4)
>>>> +{
>>>> +  if (r0 != a
>>>> +      || r1 != b
>>>> +      || r2 != c
>>>> +      || r3 != d
>>>> +      || stack != e
>>>> +      || stack4 !=f)
>>>> +    abort ();
>>>> +}
>>>> +
>>>> +int
>>>> +main (int argc, char **argv)
>>>> +{
>>>> +  foo (a, b, c, d, e, f);
>>>> +  return 0;
>>>> +}
>>>> diff --git a/gcc/testsuite/gcc.target/arm/aapcs/align_rec2.c
>>> b/gcc/testsuite/gcc.target/arm/aapcs/align_rec2.c
>>>> new file mode 100644
>>>> index
>>>
>0000000000000000000000000000000000000000..a00da508443f6c350dac610851d111d0685f2853
>>>> --- /dev/null
>>>> +++ b/gcc/testsuite/gcc.target/arm/aapcs/align_rec2.c
>>>> @@ -0,0 +1,41 @@
>>>> +/* Test AAPCS layout (alignment) for callee.  */
>>>> +
>>>> +/* { dg-do run { target arm_eabi } } */
>>>> +/* { dg-require-effective-target arm32 } */
>>>> +/* { dg-options "-O2 -fno-inline" } */
>>>> +
>>>> +extern int memcmp (const void *s1, const void *s2, __SIZE_TYPE__
>n);
>>>> +extern void abort (void);
>>>> +
>>>> +typedef struct __attribute__((aligned (8)))
>>>> +  {
>>>> +    int x;
>>>> +    int y;
>>>> +  } overaligned;
>>>> +
>>>> +overaligned a = { 2, 3 };
>>>> +overaligned b = { 5, 8 };
>>>> +
>>>> +void
>>>> +f (int r0, overaligned r1, int r3, int stack, overaligned stack4)
>>>> +{
>>>> +  if (r0 != 7 || r3 != 9 || stack != 10)
>>>> +    abort ();
>>>> +  if (memcmp ((void *) &r1, (void *)&a, sizeof (overaligned)))
>>>> +    abort ();
>>>> +  if (memcmp ((void *)&stack4, (void *)&b, sizeof (overaligned)))
>>>> +    abort ();
>>>> +  int addr = ((int) &stack4) & 7;
>>>> +  if (addr != 0)
>>>> +    {
>>>> +      __builtin_printf ("Alignment was %d\n", addr);
>>>> +      abort ();
>>>> +    }
>>>> +}
>>>> +
>>>> +int
>>>> +main (int argc, char **argv)
>>>> +{
>>>> +  f (7, a, 9, 10, b);
>>>> +  return 0;
>>>> +}
>>>> diff --git a/gcc/testsuite/gcc.target/arm/aapcs/align_rec3.c
>>> b/gcc/testsuite/gcc.target/arm/aapcs/align_rec3.c
>>>> new file mode 100644
>>>> index
>>>
>0000000000000000000000000000000000000000..2184cb76a6a7f68c59b39c12ec6472ac7b561794
>>>> --- /dev/null
>>>> +++ b/gcc/testsuite/gcc.target/arm/aapcs/align_rec3.c
>>>> @@ -0,0 +1,43 @@
>>>> +/* Test AAPCS layout (alignment) for callee.  */
>>>> +
>>>> +/* { dg-do run { target arm_eabi } } */
>>>> +/* { dg-require-effective-target arm32 } */
>>>> +/* { dg-options "-O2 -fno-inline" } */
>>>> +
>>>> +/* Test AAPCS layout (alignment) for callee.  */
>>>> +
>>>> +extern int memcmp (const void *s1, const void *s2, __SIZE_TYPE__
>n);
>>>> +extern void abort (void);
>>>> +
>>>> +
>>>> +/* Struct will be aligned to 8.  */
>>>> +struct s
>>>> +  {
>>>> +    int x;
>>>> +    /* 4 bytes padding here.  */
>>>> +    __attribute__((aligned (8))) int y;
>>>> +    /* 4 bytes padding here.  */
>>>> +  };
>>>> +
>>>> +typedef struct s __attribute__((aligned (4))) underaligned;
>>>> +
>>>> +underaligned a = { 1, 4 };
>>>> +underaligned b = { 9, 16 };
>>>> +
>>>> +void
>>>> +f (int r0, underaligned r2, int stack8, underaligned stack16)
>>>> +{
>>>> +  if (r0 != 3 || stack8 != 6)
>>>> +    abort ();
>>>> +  if (memcmp ((void *) &r2, (void *)&a, sizeof (underaligned)))
>>>> +    abort ();
>>>> +  if (memcmp ((void *)&stack16, (void *)&b, sizeof
>(underaligned)))
>>>> +    abort ();
>>>> +}
>>>> +
>>>> +int
>>>> +main (int argc, char **argv)
>>>> +{
>>>> +  f (3, a, 6, b);
>>>> +  return 0;
>>>> +}
>>>> diff --git a/gcc/testsuite/gcc.target/arm/aapcs/align_rec4.c
>>> b/gcc/testsuite/gcc.target/arm/aapcs/align_rec4.c
>>>> new file mode 100644
>>>> index
>>>
>0000000000000000000000000000000000000000..907b90af70f7ce2ded456d08d6471462e64fa15c
>>>> --- /dev/null
>>>> +++ b/gcc/testsuite/gcc.target/arm/aapcs/align_rec4.c
>>>> @@ -0,0 +1,33 @@
>>>> +/* Test AAPCS layout (alignment) for callee.  */
>>>> +
>>>> +/* { dg-do run { target arm_eabi } } */
>>>> +/* { dg-require-effective-target arm32 } */
>>>> +/* { dg-require-effective-target arm_neon_ok } */
>>>> +/* { dg-options "-O -fno-inline" } */
>>>> +/* { dg-add-options arm_neon } */
>>>> +
>>>> +#include <arm_neon.h>
>>>> +
>>>> +extern int memcmp (const void *s1, const void *s2, __SIZE_TYPE__
>n);
>>>> +extern void abort (void);
>>>> +
>>>> +typedef __attribute__((aligned (4))) int32x4_t unalignedvec;
>>>> +
>>>> +unalignedvec a = {11, 13};
>>>> +unalignedvec b = {17, 19};
>>>> +
>>>> +void
>>>> +foo (int r0, unalignedvec r2, int s0, unalignedvec s8)
>>>> +{
>>>> +  if (r0 != 2 || s0 != 6
>>>> +      || memcmp ( (void *) &r2, (void *) &a, 16)
>>>> +      || memcmp ( (void *) &s8, (void *) &b, 16))
>>>> +    abort ();
>>>> +}
>>>> +
>>>> +int
>>>> +main (int argc, char **argv)
>>>> +{
>>>> +  foo (2, a, 6, b);
>>>> +  return 0;
>>>> +}
>>>> diff --git a/gcc/testsuite/gcc.target/arm/aapcs/align_vaarg_1.c
>>> b/gcc/testsuite/gcc.target/arm/aapcs/align_vaarg_1.c
>>>> new file mode 100644
>>>> index
>>>
>0000000000000000000000000000000000000000..daa321415998df658814d853a15284ae2125cb1e
>>>> --- /dev/null
>>>> +++ b/gcc/testsuite/gcc.target/arm/aapcs/align_vaarg_1.c
>>>> @@ -0,0 +1,36 @@
>>>> +/* Test AAPCS layout (alignment of varargs) for callee.  */
>>>> +
>>>> +/* { dg-do run { target arm_eabi } } */
>>>> +/* { dg-require-effective-target arm32 } */
>>>> +/* { dg-options "-O2 -fno-inline" } */
>>>> +
>>>> +#include <stdarg.h>
>>>> +
>>>> +extern void abort (void);
>>>> +
>>>> +typedef __attribute__((aligned (8))) int alignedint;
>>>> +
>>>> +void
>>>> +foo (int i, ...)
>>>> +{
>>>> +  va_list va;
>>>> +  va_start (va, i);
>>>> +  /* Arguments should be passed in the same registers as if they
>>> were ints.  */
>>>> +  while (i-- > 0)
>>>> +    if (va_arg (va, int) != i)
>>>> +      abort ();
>>>> +  va_end (va);
>>>> +}
>>>> +
>>>> +int
>>>> +main (int argc, char **argv)
>>>> +{
>>>> +  alignedint a = 5;
>>>> +  alignedint b = 4;
>>>> +  alignedint c = 3;
>>>> +  alignedint d = 2;
>>>> +  alignedint e = 1;
>>>> +  alignedint f = 0;
>>>> +  foo (a, b, c, d, e, f);
>>>> +  return 0;
>>>> +}
>>>> diff --git a/gcc/testsuite/gcc.target/arm/aapcs/align_vaarg_2.c
>>> b/gcc/testsuite/gcc.target/arm/aapcs/align_vaarg_2.c
>>>> new file mode 100644
>>>> index
>>>
>0000000000000000000000000000000000000000..b0c923b97edbdf7ee75ce0d2ad868a16f49485fd
>>>> --- /dev/null
>>>> +++ b/gcc/testsuite/gcc.target/arm/aapcs/align_vaarg_2.c
>>>> @@ -0,0 +1,30 @@
>>>> +/* Test AAPCS layout (alignment of varargs) for callee.  */
>>>> +
>>>> +/* { dg-do run { target arm_eabi } } */
>>>> +/* { dg-require-effective-target arm32 } */
>>>> +/* { dg-options "-O2 -fno-inline" } */
>>>> +
>>>> +#include <stdarg.h>
>>>> +
>>>> +extern void abort (void);
>>>> +
>>>> +typedef __attribute__((aligned (8))) int alignedint;
>>>> +
>>>> +void
>>>> +foo (int i, ...)
>>>> +{
>>>> +  va_list va;
>>>> +  va_start (va, i);
>>>> +  /* alignedint should be pulled out of regs/stack just like an
>int.
>>> */
>>>> +  while (i-- > 0)
>>>> +    if (va_arg (va, alignedint) != i)
>>>> +      abort ();
>>>> +  va_end (va);
>>>> +}
>>>> +
>>>> +int
>>>> +main (int argc, char **argv)
>>>> +{
>>>> +  foo (5, 4, 3, 2, 1, 0);
>>>> +  return 0;
>>>> +}
>>>>
>> 
>> 



Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]