aarch64: Fix va_arg alignment handling (PR target/105549)
Commit Message
While working on enabling DFP for AArch64, I noticed new failures in
gcc.dg/compat/struct-layout-1.exp (t028) which were not actually
caused by DFP types handling. These tests are generated during 'make
check' and enabling DFP made generation different (not sure if new
non-DFP tests are generated, or if existing ones are generated
differently, the tests in question are huge and difficult to compare).
Anyway, I reduced the problem to what I attach at the end of the new
gcc.target/aarch64/aapcs64/va_arg-17.c test and rewrote it in the same
scheme as other va_arg* AArch64 tests.
This is a tough case mixing bitfields and alignment, where
aarch64_gimplify_va_arg_expr did not follow the exact same rule as
aarch64_layout_arg. When the va_arg parameter uses only one general
register, we do not want to introduce double-word alignment.
The fix is thus very small, and this patch adds a new test
(va_arg-17.c), which contains the reduced offending testcase from
struct-layout-1.exp for reference.
2022-04-25 Christophe Lyon <christophe.lyon@arm.com>
gcc/
PR target/105549
* config/aarch64/aarch64.cc (aarch64_gimplify_va_arg_expr): Fix
alignment of single-register parameters.
gcc/testssuite/
PR target/105549
* gcc.target/aarch64/aapcs64/va_arg-17.c: New.
---
gcc/config/aarch64/aarch64.cc | 4 +-
.../gcc.target/aarch64/aapcs64/va_arg-17.c | 105 ++++++++++++++++++
2 files changed, 108 insertions(+), 1 deletion(-)
create mode 100644 gcc/testsuite/gcc.target/aarch64/aapcs64/va_arg-17.c
Comments
I've reworked my patch for this PR, so this one is obsolete.
The new one is:
https://gcc.gnu.org/pipermail/gcc-patches/2022-June/596326.html
On 5/10/22 17:02, Christophe Lyon wrote:
> While working on enabling DFP for AArch64, I noticed new failures in
> gcc.dg/compat/struct-layout-1.exp (t028) which were not actually
> caused by DFP types handling. These tests are generated during 'make
> check' and enabling DFP made generation different (not sure if new
> non-DFP tests are generated, or if existing ones are generated
> differently, the tests in question are huge and difficult to compare).
>
> Anyway, I reduced the problem to what I attach at the end of the new
> gcc.target/aarch64/aapcs64/va_arg-17.c test and rewrote it in the same
> scheme as other va_arg* AArch64 tests.
>
> This is a tough case mixing bitfields and alignment, where
> aarch64_gimplify_va_arg_expr did not follow the exact same rule as
> aarch64_layout_arg. When the va_arg parameter uses only one general
> register, we do not want to introduce double-word alignment.
>
> The fix is thus very small, and this patch adds a new test
> (va_arg-17.c), which contains the reduced offending testcase from
> struct-layout-1.exp for reference.
>
> 2022-04-25 Christophe Lyon <christophe.lyon@arm.com>
>
> gcc/
> PR target/105549
> * config/aarch64/aarch64.cc (aarch64_gimplify_va_arg_expr): Fix
> alignment of single-register parameters.
>
> gcc/testssuite/
> PR target/105549
> * gcc.target/aarch64/aapcs64/va_arg-17.c: New.
> ---
> gcc/config/aarch64/aarch64.cc | 4 +-
> .../gcc.target/aarch64/aapcs64/va_arg-17.c | 105 ++++++++++++++++++
> 2 files changed, 108 insertions(+), 1 deletion(-)
> create mode 100644 gcc/testsuite/gcc.target/aarch64/aapcs64/va_arg-17.c
>
> diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc
> index f650abbc4ce..bd855758778 100644
> --- a/gcc/config/aarch64/aarch64.cc
> +++ b/gcc/config/aarch64/aarch64.cc
> @@ -19667,7 +19667,9 @@ aarch64_gimplify_va_arg_expr (tree valist, tree type, gimple_seq *pre_p,
> rsize = ROUND_UP (size, UNITS_PER_WORD);
> nregs = rsize / UNITS_PER_WORD;
>
> - if (align > 8)
> + /* Align on double-word only if we need 2 registers, like in
> + aarch64_layout_arg. */
> + if (align > 8 && nregs == 2)
> {
> if (abi_break && warn_psabi)
> inform (input_location, "parameter passing for argument of type "
> diff --git a/gcc/testsuite/gcc.target/aarch64/aapcs64/va_arg-17.c b/gcc/testsuite/gcc.target/aarch64/aapcs64/va_arg-17.c
> new file mode 100644
> index 00000000000..24895c3ab48
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/aarch64/aapcs64/va_arg-17.c
> @@ -0,0 +1,105 @@
> +/* Test AAPCS64 layout and __builtin_va_arg.
> +
> + This test covers a corner case where a composite type parameter fits in one
> + register: we do not need a double-word alignment when accessing it in the
> + va_arg stack area. */
> +
> +/* { dg-do run { target aarch64*-*-* } } */
> +
> +#ifndef IN_FRAMEWORK
> +#define AAPCS64_TEST_STDARG
> +#define TESTFILE "va_arg-17.c"
> +#include "type-def.h"
> +
> +enum E6 { e6_0, e6_1, e6_2, e6_3, e6_65533 = 65533, e6_65534, e6_65535 };
> +typedef enum E6 Tal16E6 __attribute__((aligned (16)));
> +typedef unsigned int Tuint;
> +
> +int fails;
> +
> +union S2844 {
> + Tuint a:((((10) - 1) & 31) + 1);
> + Tal16E6 __attribute__((aligned (2), packed)) b:31;
> + struct{}c[0];
> +} ;
> +union S2844 s2844;
> +union S2844 a2844[5];
> +
> +#define HAS_DATA_INIT_FUNC
> +void init_data ()
> +{
> + memset (&s2844, '\0', sizeof (s2844));
> + memset (a2844, '\0', sizeof (a2844));
> + s2844.a = 799U;
> + a2844[2].a = 586U;
> +}
> +
> +#include "abitest.h"
> +#else
> + ARG (int , 1 , W0 , LAST_NAMED_ARG_ID)
> + DOTS
> + ANON_PROMOTED (float , 1.0f, double, 1.0, D0, 1)
> + ANON (union S2844 , s2844 , X1 , 2)
> + ANON (long long , 2LL , X2 , 3)
> + ANON (union S2844 , a2844[2] , X3 , 4)
> + LAST_ANON (union S2844 , a2844[2] , X4 , 5)
> +#endif
> +
> +#if 0
> + /* This test is derived from a case generated by struct-layout-1.exp: */
> +
> +enum E6 { e6_0, e6_1, e6_2, e6_3, e6_65533 = 65533, e6_65534, e6_65535 };
> +typedef enum E6 Tal16E6 __attribute__((aligned (16)));
> +typedef unsigned int Tuint;
> +
> +int fails;
> +
> +union S2844 {
> + Tuint a:((((10) - 1) & 31) + 1);
> + Tal16E6 __attribute__((aligned (2), packed)) b:31;
> + struct{}c[0];
> +} ;
> +union S2844 s2844;
> +union S2844 a2844[5];
> +
> +typedef __builtin_va_list __gnuc_va_list;
> +typedef __gnuc_va_list va_list;
> +
> +void check2844va (int z, ...) {
> + union S2844 arg, *p;
> + va_list ap;
> +
> + __builtin_va_start(ap,z);
> + if (__builtin_va_arg(ap,double) != 1.0)
> + printf ("fail %d.%d\n", 2844, 0), ++fails;
> +
> + p = &s2844;
> + arg = __builtin_va_arg(ap,union S2844); /* This would fail. */
> + if (p->a != arg.a)
> + printf ("fail %d.%d\n", 2844, 1), ++fails;
> +
> + if (__builtin_va_arg(ap,long long) != 3LL)
> + printf ("fail %d.%d\n", 2844, 2), ++fails;
> +
> + p = &a2844[2];
> + arg = __builtin_va_arg(ap,union S2844); /* This would fail. */
> + if (p->a != arg.a)
> + printf ("fail %d.%d\n", 2844, 3), ++fails;
> +
> + arg = __builtin_va_arg(ap,union S2844); /* This would fail. */
> + if (p->a != arg.a)
> + printf ("fail %d.%d\n", 2844, 4), ++fails;
> +
> + __builtin_va_end(ap);
> +}
> +
> +int main (void) {
> + int i, j;
> + memset (&s2844, '\0', sizeof (s2844));
> + memset (a2844, '\0', sizeof (a2844));
> + s2844.a = 799U;
> + a2844[2].a = 586U;
> + check2844va (1, 1.0, s2844, 2LL, a2844[2], a2844[2]);
> + exit (fails != 0);
> +}
> +#endif /* 0 */
@@ -19667,7 +19667,9 @@ aarch64_gimplify_va_arg_expr (tree valist, tree type, gimple_seq *pre_p,
rsize = ROUND_UP (size, UNITS_PER_WORD);
nregs = rsize / UNITS_PER_WORD;
- if (align > 8)
+ /* Align on double-word only if we need 2 registers, like in
+ aarch64_layout_arg. */
+ if (align > 8 && nregs == 2)
{
if (abi_break && warn_psabi)
inform (input_location, "parameter passing for argument of type "
new file mode 100644
@@ -0,0 +1,105 @@
+/* Test AAPCS64 layout and __builtin_va_arg.
+
+ This test covers a corner case where a composite type parameter fits in one
+ register: we do not need a double-word alignment when accessing it in the
+ va_arg stack area. */
+
+/* { dg-do run { target aarch64*-*-* } } */
+
+#ifndef IN_FRAMEWORK
+#define AAPCS64_TEST_STDARG
+#define TESTFILE "va_arg-17.c"
+#include "type-def.h"
+
+enum E6 { e6_0, e6_1, e6_2, e6_3, e6_65533 = 65533, e6_65534, e6_65535 };
+typedef enum E6 Tal16E6 __attribute__((aligned (16)));
+typedef unsigned int Tuint;
+
+int fails;
+
+union S2844 {
+ Tuint a:((((10) - 1) & 31) + 1);
+ Tal16E6 __attribute__((aligned (2), packed)) b:31;
+ struct{}c[0];
+} ;
+union S2844 s2844;
+union S2844 a2844[5];
+
+#define HAS_DATA_INIT_FUNC
+void init_data ()
+{
+ memset (&s2844, '\0', sizeof (s2844));
+ memset (a2844, '\0', sizeof (a2844));
+ s2844.a = 799U;
+ a2844[2].a = 586U;
+}
+
+#include "abitest.h"
+#else
+ ARG (int , 1 , W0 , LAST_NAMED_ARG_ID)
+ DOTS
+ ANON_PROMOTED (float , 1.0f, double, 1.0, D0, 1)
+ ANON (union S2844 , s2844 , X1 , 2)
+ ANON (long long , 2LL , X2 , 3)
+ ANON (union S2844 , a2844[2] , X3 , 4)
+ LAST_ANON (union S2844 , a2844[2] , X4 , 5)
+#endif
+
+#if 0
+ /* This test is derived from a case generated by struct-layout-1.exp: */
+
+enum E6 { e6_0, e6_1, e6_2, e6_3, e6_65533 = 65533, e6_65534, e6_65535 };
+typedef enum E6 Tal16E6 __attribute__((aligned (16)));
+typedef unsigned int Tuint;
+
+int fails;
+
+union S2844 {
+ Tuint a:((((10) - 1) & 31) + 1);
+ Tal16E6 __attribute__((aligned (2), packed)) b:31;
+ struct{}c[0];
+} ;
+union S2844 s2844;
+union S2844 a2844[5];
+
+typedef __builtin_va_list __gnuc_va_list;
+typedef __gnuc_va_list va_list;
+
+void check2844va (int z, ...) {
+ union S2844 arg, *p;
+ va_list ap;
+
+ __builtin_va_start(ap,z);
+ if (__builtin_va_arg(ap,double) != 1.0)
+ printf ("fail %d.%d\n", 2844, 0), ++fails;
+
+ p = &s2844;
+ arg = __builtin_va_arg(ap,union S2844); /* This would fail. */
+ if (p->a != arg.a)
+ printf ("fail %d.%d\n", 2844, 1), ++fails;
+
+ if (__builtin_va_arg(ap,long long) != 3LL)
+ printf ("fail %d.%d\n", 2844, 2), ++fails;
+
+ p = &a2844[2];
+ arg = __builtin_va_arg(ap,union S2844); /* This would fail. */
+ if (p->a != arg.a)
+ printf ("fail %d.%d\n", 2844, 3), ++fails;
+
+ arg = __builtin_va_arg(ap,union S2844); /* This would fail. */
+ if (p->a != arg.a)
+ printf ("fail %d.%d\n", 2844, 4), ++fails;
+
+ __builtin_va_end(ap);
+}
+
+int main (void) {
+ int i, j;
+ memset (&s2844, '\0', sizeof (s2844));
+ memset (a2844, '\0', sizeof (a2844));
+ s2844.a = 799U;
+ a2844[2].a = 586U;
+ check2844va (1, 1.0, s2844, 2LL, a2844[2], a2844[2]);
+ exit (fails != 0);
+}
+#endif /* 0 */