mips: Fix overaligned function arguments [PR109435]
Checks
Commit Message
This patch changes alignment for typedef types when passed as
arguments, making the alignment equal to the alignment of
original (aliased) types.
This change makes it impossible for a typedef type to have
alignment that is less than its size.
Signed-off-by: Jovan Dmitrovic <jovan.dmitrovic@syrmia.com>
gcc/ChangeLog:
PR target/109435
* config/mips/mips.cc (mips_function_arg_alignment): Returns
the alignment of function argument. In case of typedef type,
it returns the aligment of the aliased type.
(mips_function_arg_boundary): Relocated calculation of the
aligment of function arguments.
gcc/testsuite/ChangeLog:
PR target/109435
* gcc.target/mips/align-1.c: New test.
---
gcc/config/mips/mips.cc | 18 +++++++++++++-
gcc/testsuite/gcc.target/mips/align-1.c | 33 +++++++++++++++++++++++++
2 files changed, 50 insertions(+), 1 deletion(-)
create mode 100644 gcc/testsuite/gcc.target/mips/align-1.c
Comments
Jovan Dmitrovic <Jovan.Dmitrovic@syrmia.com> 于2023年5月29日周一 19:00写道：
>
> This patch changes alignment for typedef types when passed as
> arguments, making the alignment equal to the alignment of
> original (aliased) types.
>
> This change makes it impossible for a typedef type to have
> alignment that is less than its size.
>
> Signed-off-by: Jovan Dmitrovic <jovan.dmitrovic@syrmia.com>
>
> gcc/ChangeLog:
> PR target/109435
> * config/mips/mips.cc (mips_function_arg_alignment): Returns
> the alignment of function argument. In case of typedef type,
> it returns the aligment of the aliased type.
> (mips_function_arg_boundary): Relocated calculation of the
> aligment of function arguments.
>
> gcc/testsuite/ChangeLog:
> PR target/109435
> * gcc.target/mips/align-1.c: New test.
> ---
> gcc/config/mips/mips.cc | 18 +++++++++++++-
> gcc/testsuite/gcc.target/mips/align-1.c | 33 +++++++++++++++++++++++++
> 2 files changed, 50 insertions(+), 1 deletion(-)
> create mode 100644 gcc/testsuite/gcc.target/mips/align-1.c
>
> diff --git a/gcc/config/mips/mips.cc b/gcc/config/mips/mips.cc
> index ca822758b41..2019b7cd7d9 100644
> --- a/gcc/config/mips/mips.cc
> +++ b/gcc/config/mips/mips.cc
> @@ -6190,6 +6190,22 @@ mips_arg_partial_bytes (cumulative_args_t cum, const function_arg_info &arg)
> return info.stack_words > 0 ? info.reg_words * UNITS_PER_WORD : 0;
> }
>
> +/* Given MODE and TYPE of a function argument, return the alignment in
> + bits. In case of typedef, alignment of its original type is
> + used. */
> +
> +static unsigned int
> +mips_function_arg_alignment (machine_mode mode, const_tree type)
> +{
> + if (!type)
> + return GET_MODE_ALIGNMENT (mode);
> +
> + if (is_typedef_decl (TYPE_NAME (type)))
> + type = DECL_ORIGINAL_TYPE (TYPE_NAME (type));
> +
> + return TYPE_ALIGN (type);
> +}
> +
> /* Implement TARGET_FUNCTION_ARG_BOUNDARY. Every parameter gets at
> least PARM_BOUNDARY bits of alignment, but will be given anything up
> to STACK_BOUNDARY bits if the type requires it. */
> @@ -6198,8 +6214,8 @@ static unsigned int
> mips_function_arg_boundary (machine_mode mode, const_tree type)
> {
> unsigned int alignment;
> + alignment = mips_function_arg_alignment (mode, type);
>
> - alignment = type ? TYPE_ALIGN (type) : GET_MODE_ALIGNMENT (mode);
> if (alignment < PARM_BOUNDARY)
> alignment = PARM_BOUNDARY;
> if (alignment > STACK_BOUNDARY)
> diff --git a/gcc/testsuite/gcc.target/mips/align-1.c b/gcc/testsuite/gcc.target/mips/align-1.c
> new file mode 100644
> index 00000000000..816751b8099
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/mips/align-1.c
> @@ -0,0 +1,33 @@
> +/* Check that typedef alignment does not affect passing of function
> + parameters. */
> +/* { dg-do run { target { "mips*-*-linux*" } } } */
Is it possible to check the result with something like
scan-assembler
scan-assembler-not
instead of real running?
> +
> +#include <assert.h>
> +
> +typedef struct ui8
> +{
> + unsigned v[8];
> +} uint8 __attribute__ ((aligned(64)));
> +
> +unsigned
> +callee (int x, uint8 a)
> +{
> + return a.v[0];
> +}
> +
> +uint8
> +identity (uint8 in)
> +{
> + return in;
> +}
> +
> +int
> +main (void)
> +{
> + uint8 vec = {{1, 2, 3, 4, 5, 6, 7, 8}};
> + uint8 temp = identity (vec);
> + unsigned temp2 = callee (1, identity (vec));
> + assert (callee (1, temp) == 1);
> + assert (temp2 == 1);
> + return 0;
> +}
> --
> 2.34.1
>
I suppose that it is possible to check assembly.
Following is part of diff before and after my patch:
29,32c29,32
< sd $6,0($2)
< sd $7,8($2)
< sd $8,16($2)
< sd $9,24($2)
---
> sd $5,0($2)
> sd $6,8($2)
> sd $7,16($2)
> sd $8,24($2)
63,66c63,66
< sd $6,0($2)
< sd $7,8($2)
< sd $8,16($2)
< sd $9,24($2)
---
> sd $5,0($2)
> sd $6,8($2)
> sd $7,16($2)
> sd $8,24($2)
138,141c138,141
< ld $6,64($16)
< ld $7,72($16)
< ld $8,80($16)
< ld $9,88($16)
---
> ld $5,64($16)
> ld $6,72($16)
> ld $7,80($16)
> ld $8,88($16)
148,151c148,151
< ld $6,64($16)
< ld $7,72($16)
< ld $8,80($16)
< ld $9,88($16)
---
> ld $5,64($16)
> ld $6,72($16)
> ld $7,80($16)
> ld $8,88($16)
167,170c167,170
< ld $6,0($16)
< ld $7,8($16)
< ld $8,16($16)
< ld $9,24($16)
---
> ld $5,0($16)
> ld $6,8($16)
> ld $7,16($16)
> ld $8,24($16)
What my patch effectively does it rearranges the data in
registers when invoking a function. I don't know whether
writing this testcase as an assembly check would make sense,
because that would make the testcase much less readable.
Jovan Dmitrovic <Jovan.Dmitrovic@syrmia.com> 于2023年6月6日周二 18:29写道：
>
> I suppose that it is possible to check assembly.
>
Great.
> Following is part of diff before and after my patch:
>
> 29,32c29,32
> < sd $6,0($2)
> < sd $7,8($2)
> < sd $8,16($2)
> < sd $9,24($2)
> ---
> > sd $5,0($2)
> > sd $6,8($2)
> > sd $7,16($2)
> > sd $8,24($2)
> 63,66c63,66
> < sd $6,0($2)
> < sd $7,8($2)
> < sd $8,16($2)
> < sd $9,24($2)
> ---
> > sd $5,0($2)
> > sd $6,8($2)
> > sd $7,16($2)
> > sd $8,24($2)
> 138,141c138,141
> < ld $6,64($16)
> < ld $7,72($16)
> < ld $8,80($16)
> < ld $9,88($16)
> ---
> > ld $5,64($16)
> > ld $6,72($16)
> > ld $7,80($16)
> > ld $8,88($16)
> 148,151c148,151
> < ld $6,64($16)
> < ld $7,72($16)
> < ld $8,80($16)
> < ld $9,88($16)
> ---
> > ld $5,64($16)
> > ld $6,72($16)
> > ld $7,80($16)
> > ld $8,88($16)
> 167,170c167,170
> < ld $6,0($16)
> < ld $7,8($16)
> < ld $8,16($16)
> < ld $9,24($16)
> ---
> > ld $5,0($16)
> > ld $6,8($16)
> > ld $7,16($16)
> > ld $8,24($16)
>
> What my patch effectively does it rearranges the data in
> registers when invoking a function. I don't know whether
> writing this testcase as an assembly check would make sense,
> because that would make the testcase much less readable.
I prefer an assembly check, because the test can be used even
for cross building.
It is not required, I guess.
> ________________________________________
> From: YunQiang Su <wzssyqa@gmail.com>
> Sent: Wednesday, May 31, 2023 12:05 PM
> To: Jovan Dmitrovic
> Cc: gcc-patches@gcc.gnu.org; Djordje Todorovic
> Subject: Re: [PATCH] mips: Fix overaligned function arguments [PR109435]
>
> Jovan Dmitrovic <Jovan.Dmitrovic@syrmia.com> 于2023年5月29日周一 19:00写道：
> >
> > This patch changes alignment for typedef types when passed as
> > arguments, making the alignment equal to the alignment of
> > original (aliased) types.
> >
> > This change makes it impossible for a typedef type to have
> > alignment that is less than its size.
> >
> > Signed-off-by: Jovan Dmitrovic <jovan.dmitrovic@syrmia.com>
> >
> > gcc/ChangeLog:
> > PR target/109435
> > * config/mips/mips.cc (mips_function_arg_alignment): Returns
> > the alignment of function argument. In case of typedef type,
> > it returns the aligment of the aliased type.
> > (mips_function_arg_boundary): Relocated calculation of the
> > aligment of function arguments.
> >
> > gcc/testsuite/ChangeLog:
> > PR target/109435
> > * gcc.target/mips/align-1.c: New test.
> > ---
> > gcc/config/mips/mips.cc | 18 +++++++++++++-
> > gcc/testsuite/gcc.target/mips/align-1.c | 33 +++++++++++++++++++++++++
> > 2 files changed, 50 insertions(+), 1 deletion(-)
> > create mode 100644 gcc/testsuite/gcc.target/mips/align-1.c
> >
> > diff --git a/gcc/config/mips/mips.cc b/gcc/config/mips/mips.cc
> > index ca822758b41..2019b7cd7d9 100644
> > --- a/gcc/config/mips/mips.cc
> > +++ b/gcc/config/mips/mips.cc
> > @@ -6190,6 +6190,22 @@ mips_arg_partial_bytes (cumulative_args_t cum, const function_arg_info &arg)
> > return info.stack_words > 0 ? info.reg_words * UNITS_PER_WORD : 0;
> > }
> >
> > +/* Given MODE and TYPE of a function argument, return the alignment in
> > + bits. In case of typedef, alignment of its original type is
> > + used. */
> > +
> > +static unsigned int
> > +mips_function_arg_alignment (machine_mode mode, const_tree type)
> > +{
> > + if (!type)
> > + return GET_MODE_ALIGNMENT (mode);
> > +
> > + if (is_typedef_decl (TYPE_NAME (type)))
> > + type = DECL_ORIGINAL_TYPE (TYPE_NAME (type));
> > +
> > + return TYPE_ALIGN (type);
> > +}
> > +
> > /* Implement TARGET_FUNCTION_ARG_BOUNDARY. Every parameter gets at
> > least PARM_BOUNDARY bits of alignment, but will be given anything up
> > to STACK_BOUNDARY bits if the type requires it. */
> > @@ -6198,8 +6214,8 @@ static unsigned int
> > mips_function_arg_boundary (machine_mode mode, const_tree type)
> > {
> > unsigned int alignment;
> > + alignment = mips_function_arg_alignment (mode, type);
> >
> > - alignment = type ? TYPE_ALIGN (type) : GET_MODE_ALIGNMENT (mode);
> > if (alignment < PARM_BOUNDARY)
> > alignment = PARM_BOUNDARY;
> > if (alignment > STACK_BOUNDARY)
> > diff --git a/gcc/testsuite/gcc.target/mips/align-1.c b/gcc/testsuite/gcc.target/mips/align-1.c
> > new file mode 100644
> > index 00000000000..816751b8099
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.target/mips/align-1.c
> > @@ -0,0 +1,33 @@
> > +/* Check that typedef alignment does not affect passing of function
> > + parameters. */
> > +/* { dg-do run { target { "mips*-*-linux*" } } } */
>
> Is it possible to check the result with something like
> scan-assembler
> scan-assembler-not
> instead of real running?
>
> > +
> > +#include <assert.h>
> > +
> > +typedef struct ui8
> > +{
> > + unsigned v[8];
> > +} uint8 __attribute__ ((aligned(64)));
> > +
> > +unsigned
> > +callee (int x, uint8 a)
> > +{
> > + return a.v[0];
> > +}
> > +
> > +uint8
> > +identity (uint8 in)
> > +{
> > + return in;
> > +}
> > +
> > +int
> > +main (void)
> > +{
> > + uint8 vec = {{1, 2, 3, 4, 5, 6, 7, 8}};
> > + uint8 temp = identity (vec);
> > + unsigned temp2 = callee (1, identity (vec));
> > + assert (callee (1, temp) == 1);
> > + assert (temp2 == 1);
> > + return 0;
> > +}
> > --
> > 2.34.1
> >
I see what you mean now, so I've made adjustment in order for testcase to work
on assembly. Following is the updated patch.
Regards,
Jovan
From 2744357b5232c61bf1f780c4915d47b19d71f993 Mon Sep 17 00:00:00 2001
From: Jovan Dmitrovic <Jovan.Dmitrovic@Syrmia.com>
Date: Fri, 19 May 2023 12:36:55 +0200
Subject: [PATCH] mips: Fix overaligned function arguments [PR109435]
This patch changes alignment for typedef types when passed as
arguments, making the alignment equal to the alignment of
original (aliased) types.
This change makes it impossible for a typedef type to have
alignment that is less than its size.
Signed-off-by: Jovan Dmitrovic <jovan.dmitrovic@syrmia.com>
gcc/ChangeLog:
PR target/109435
* config/mips/mips.cc (mips_function_arg_alignment): Returns
the alignment of function argument. In case of typedef type,
it returns the aligment of the aliased type.
(mips_function_arg_boundary): Relocated calculation of the
aligment of function arguments.
gcc/testsuite/ChangeLog:
PR target/109435
* gcc.target/mips/align-1.c: New test.
---
gcc/config/mips/mips.cc | 19 ++++++++++++-
gcc/testsuite/gcc.target/mips/align-1.c | 38 +++++++++++++++++++++++++
2 files changed, 56 insertions(+), 1 deletion(-)
create mode 100644 gcc/testsuite/gcc.target/mips/align-1.c
diff --git a/gcc/config/mips/mips.cc b/gcc/config/mips/mips.cc
index c1d1691306e..20ba35f754c 100644
--- a/gcc/config/mips/mips.cc
+++ b/gcc/config/mips/mips.cc
@@ -6190,6 +6190,23 @@ mips_arg_partial_bytes (cumulative_args_t cum, const function_arg_info &arg)
return info.stack_words > 0 ? info.reg_words * UNITS_PER_WORD : 0;
}
+/* Given MODE and TYPE of a function argument, return the alignment in
+ bits.
+ In case of typedef, alignment of its original type is
+ used. */
+
+static unsigned int
+mips_function_arg_alignment (machine_mode mode, const_tree type)
+{
+ if (!type)
+ return GET_MODE_ALIGNMENT (mode);
+
+ if (is_typedef_decl (TYPE_NAME (type)))
+ type = DECL_ORIGINAL_TYPE (TYPE_NAME (type));
+
+ return TYPE_ALIGN (type);
+}
+
/* Implement TARGET_FUNCTION_ARG_BOUNDARY. Every parameter gets at
least PARM_BOUNDARY bits of alignment, but will be given anything up
to STACK_BOUNDARY bits if the type requires it. */
@@ -6198,8 +6215,8 @@ static unsigned int
mips_function_arg_boundary (machine_mode mode, const_tree type)
{
unsigned int alignment;
+ alignment = mips_function_arg_alignment (mode, type);
- alignment = type ? TYPE_ALIGN (type) : GET_MODE_ALIGNMENT (mode);
if (alignment < PARM_BOUNDARY)
alignment = PARM_BOUNDARY;
if (alignment > STACK_BOUNDARY)
diff --git a/gcc/testsuite/gcc.target/mips/align-1.c b/gcc/testsuite/gcc.target/mips/align-1.c
new file mode 100644
index 00000000000..5c639bee274
--- /dev/null
+++ b/gcc/testsuite/gcc.target/mips/align-1.c
@@ -0,0 +1,38 @@
+/* Check that typedef alignment does not affect passing of function
+ parameters. */
+/* { dg-do compile { target { "mips*-*-linux*" } } } */
+/* { dg-skip-if "" { *-*-* } { "-flto" } { "" } } */
+
+#include <assert.h>
+
+typedef struct ui8
+{
+ unsigned v[8];
+} uint8 __attribute__ ((aligned(64)));
+
+unsigned
+callee (int x, uint8 a)
+{
+ return a.v[0];
+}
+
+uint8
+identity (uint8 in)
+{
+ return in;
+}
+
+int
+main (void)
+{
+ uint8 vec = {{1, 2, 3, 4, 5, 6, 7, 8}};
+ uint8 temp = identity (vec);
+ unsigned temp2 = callee (1, identity (vec));
+ assert (callee (1, temp) == 1);
+ assert (temp2 == 1);
+ return 0;
+}
+
+/* { dg-final { scan-assembler "\tsd\t\\\$5,0\\(\\\$\[0-9\]\\)" } } */
+/* { dg-final { scan-assembler "\tsd\t\\\$6,8\\(\\\$\[0-9\]\\)" } } */
+/* { dg-final { scan-assembler "\tsd\t\\\$7,16\\(\\\$\[0-9\]\\)" } } */
Jovan Dmitrovic <Jovan.Dmitrovic@syrmia.com> 于2023年6月7日周三 18:29写道：
>
> I see what you mean now, so I've made adjustment in order for testcase to work
> on assembly. Following is the updated patch.
>
> Regards,
> Jovan
>
> From 2744357b5232c61bf1f780c4915d47b19d71f993 Mon Sep 17 00:00:00 2001
> From: Jovan Dmitrovic <Jovan.Dmitrovic@Syrmia.com>
> Date: Fri, 19 May 2023 12:36:55 +0200
> Subject: [PATCH] mips: Fix overaligned function arguments [PR109435]
>
> This patch changes alignment for typedef types when passed as
> arguments, making the alignment equal to the alignment of
> original (aliased) types.
>
> This change makes it impossible for a typedef type to have
> alignment that is less than its size.
>
> Signed-off-by: Jovan Dmitrovic <jovan.dmitrovic@syrmia.com>
>
> gcc/ChangeLog:
> PR target/109435
> * config/mips/mips.cc (mips_function_arg_alignment): Returns
> the alignment of function argument. In case of typedef type,
> it returns the aligment of the aliased type.
> (mips_function_arg_boundary): Relocated calculation of the
> aligment of function arguments.
>
Please refer
https://gcc.gnu.org/contribute.html
about how to work with the ChangeLog.
> gcc/testsuite/ChangeLog:
> PR target/109435
> * gcc.target/mips/align-1.c: New test.
> ---
> gcc/config/mips/mips.cc | 19 ++++++++++++-
> gcc/testsuite/gcc.target/mips/align-1.c | 38 +++++++++++++++++++++++++
> 2 files changed, 56 insertions(+), 1 deletion(-)
> create mode 100644 gcc/testsuite/gcc.target/mips/align-1.c
>
> diff --git a/gcc/config/mips/mips.cc b/gcc/config/mips/mips.cc
> index c1d1691306e..20ba35f754c 100644
> --- a/gcc/config/mips/mips.cc
> +++ b/gcc/config/mips/mips.cc
> @@ -6190,6 +6190,23 @@ mips_arg_partial_bytes (cumulative_args_t cum, const function_arg_info &arg)
> return info.stack_words > 0 ? info.reg_words * UNITS_PER_WORD : 0;
> }
>
> +/* Given MODE and TYPE of a function argument, return the alignment in
> + bits.
> + In case of typedef, alignment of its original type is
> + used. */
> +
> +static unsigned int
> +mips_function_arg_alignment (machine_mode mode, const_tree type)
> +{
> + if (!type)
> + return GET_MODE_ALIGNMENT (mode);
> +
> + if (is_typedef_decl (TYPE_NAME (type)))
> + type = DECL_ORIGINAL_TYPE (TYPE_NAME (type));
> +
> + return TYPE_ALIGN (type);
> +}
> +
> /* Implement TARGET_FUNCTION_ARG_BOUNDARY. Every parameter gets at
> least PARM_BOUNDARY bits of alignment, but will be given anything up
> to STACK_BOUNDARY bits if the type requires it. */
> @@ -6198,8 +6215,8 @@ static unsigned int
> mips_function_arg_boundary (machine_mode mode, const_tree type)
> {
> unsigned int alignment;
> + alignment = mips_function_arg_alignment (mode, type);
>
> - alignment = type ? TYPE_ALIGN (type) : GET_MODE_ALIGNMENT (mode);
> if (alignment < PARM_BOUNDARY)
> alignment = PARM_BOUNDARY;
> if (alignment > STACK_BOUNDARY)
> diff --git a/gcc/testsuite/gcc.target/mips/align-1.c b/gcc/testsuite/gcc.target/mips/align-1.c
> new file mode 100644
> index 00000000000..5c639bee274
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/mips/align-1.c
> @@ -0,0 +1,38 @@
> +/* Check that typedef alignment does not affect passing of function
> + parameters. */
> +/* { dg-do compile { target { "mips*-*-linux*" } } } */
mips* may be OK, since this test looks reasonable for bare metal platforms.
> +/* { dg-skip-if "" { *-*-* } { "-flto" } { "" } } */
Does `-flto` required here?
> +
> +#include <assert.h>
> +
> +typedef struct ui8
> +{
> + unsigned v[8];
> +} uint8 __attribute__ ((aligned(64)));
> +
> +unsigned
> +callee (int x, uint8 a)
> +{
> + return a.v[0];
> +}
> +
> +uint8
> +identity (uint8 in)
> +{
> + return in;
> +}
> +
> +int
> +main (void)
> +{
> + uint8 vec = {{1, 2, 3, 4, 5, 6, 7, 8}};
> + uint8 temp = identity (vec);
> + unsigned temp2 = callee (1, identity (vec));
> + assert (callee (1, temp) == 1);
> + assert (temp2 == 1);
> + return 0;
> +}
> +
> +/* { dg-final { scan-assembler "\tsd\t\\\$5,0\\(\\\$\[0-9\]\\)" } } */
> +/* { dg-final { scan-assembler "\tsd\t\\\$6,8\\(\\\$\[0-9\]\\)" } } */
> +/* { dg-final { scan-assembler "\tsd\t\\\$7,16\\(\\\$\[0-9\]\\)" } } */
I guess, this test may fail for mips32 targets?
Maybe we can add 2 tests: one for O32, and one for N32/N64.
Add `-mabi=32`/`-mabi=n32` option into `dg-do compile` line.
> --
> 2.34.1
>
>
>
>
> --
> YunQiang Su
Hi,
I am sending a revised patch, now with different tests for N64/N32 and O32 ABIs.
For the O32 ABI, I've skipped the -O0 and -Os pipelines, considering there is a
difference between exact offsets for store instructions (the registers used remain
the same).
Skipping -flto isn't really necessary, so I've removed that part.
I've fixed the Changelog, hopefully I've corrected the mistakes I made.
Regards,
Jovan
Jovan Dmitrovic <Jovan.Dmitrovic@syrmia.com> 于2023年6月27日周二 16:54写道：
>
> Hi,
> I am sending a revised patch, now with different tests for N64/N32 and O32 ABIs.
> For the O32 ABI, I've skipped the -O0 and -Os pipelines, considering there is a
> difference between exact offsets for store instructions (the registers used remain
> the same).
>
> Skipping -flto isn't really necessary, so I've removed that part.
>
> I've fixed the Changelog, hopefully I've corrected the mistakes I made.
>
Looks good.
I will submit this patch with some format improvement.
> Regards,
> Jovan
YunQiang Su <wzssyqa@gmail.com> 于2023年6月29日周四 14:04写道：
>
> Jovan Dmitrovic <Jovan.Dmitrovic@syrmia.com> 于2023年6月27日周二 16:54写道：
> >
> > Hi,
> > I am sending a revised patch, now with different tests for N64/N32 and O32 ABIs.
> > For the O32 ABI, I've skipped the -O0 and -Os pipelines, considering there is a
> > difference between exact offsets for store instructions (the registers used remain
> > the same).
> >
> > Skipping -flto isn't really necessary, so I've removed that part.
> >
> > I've fixed the Changelog, hopefully I've corrected the mistakes I made.
> >
>
> Looks good.
> I will submit this patch with some format improvement.
>
Ohh, my fault: the `-flto` option should always be skipped, when run test.
And you skipped -O0 test on O32, while this bug effects O0 only, it
should not be expected.
The below is my modification to your patch.
Is it OK for you?
--- xx.patch 2023-06-29 14:32:59.805474033 +0800
+++ build/0001-mips-Fix-overaligned-function-arguments-PR109435.patch
2023-06-29 18:01:19.245478275 +0800
@@ -1,4 +1,4 @@
-From 05e4ff4d2fbb91ea8040fb10d8d6a130ad24bba7 Mon Sep 17 00:00:00 2001
+From 7b5af22bb7c8fadce27e94c37c96101a06acd286 Mon Sep 17 00:00:00 2001
From: Jovan Dmitrovic <Jovan.Dmitrovic@Syrmia.com>
Date: Mon, 26 Jun 2023 17:00:20 +0200
Subject: [PATCH] mips: Fix overaligned function arguments [PR109435]
@@ -16,12 +16,13 @@
2023-06-27 Jovan Dmitrović <jovan.dmitrovic@syrmia.com>
gcc/ChangeLog:
- PR target/109435
+
+ PR target/109435
* config/mips/mips.cc (mips_function_arg_alignment): Returns
- the alignment of function argument. In case of typedef type,
- it returns the aligment of the aliased type.
+ the alignment of function argument. In case of typedef type,
+ it returns the aligment of the aliased type.
(mips_function_arg_boundary): Relocated calculation of the
- aligment of function arguments.
+ aligment of function arguments.
gcc/testsuite/ChangeLog:
@@ -29,9 +30,9 @@
* gcc.target/mips/align-1-o32.c: New test.
---
gcc/config/mips/mips.cc | 19 ++++++++++++++++++-
- gcc/testsuite/gcc.target/mips/align-1-n64.c | 19 +++++++++++++++++++
+ gcc/testsuite/gcc.target/mips/align-1-n64.c | 20 ++++++++++++++++++++
gcc/testsuite/gcc.target/mips/align-1-o32.c | 20 ++++++++++++++++++++
- 3 files changed, 57 insertions(+), 1 deletion(-)
+ 3 files changed, 58 insertions(+), 1 deletion(-)
create mode 100644 gcc/testsuite/gcc.target/mips/align-1-n64.c
create mode 100644 gcc/testsuite/gcc.target/mips/align-1-o32.c
@@ -75,14 +76,15 @@
if (alignment > STACK_BOUNDARY)
diff --git a/gcc/testsuite/gcc.target/mips/align-1-n64.c
b/gcc/testsuite/gcc.target/mips/align-1-n64.c
new file mode 100644
-index 00000000000..46e718d548d
+index 00000000000..3ede539c3a4
--- /dev/null
+++ b/gcc/testsuite/gcc.target/mips/align-1-n64.c
-@@ -0,0 +1,19 @@
+@@ -0,0 +1,20 @@
+/* Check that typedef alignment does not affect passing of function
+ parameters for N64/N32 ABIs. */
+/* { dg-do compile { target { "mips*-*-*" } } } */
+/* { dg-options "-mabi=64" } */
++/* { dg-skip-if "" { *-*-* } { "-flto" } { "" } } */
+
+typedef struct ui8
+{
@@ -100,7 +102,7 @@
+/* { dg-final { scan-assembler "\tsd\t\\\$7,16\\(\\\$\[0-9\]\\)" } } */
diff --git a/gcc/testsuite/gcc.target/mips/align-1-o32.c
b/gcc/testsuite/gcc.target/mips/align-1-o32.c
new file mode 100644
-index 00000000000..a548632b7f6
+index 00000000000..e043d6a3eca
--- /dev/null
+++ b/gcc/testsuite/gcc.target/mips/align-1-o32.c
@@ -0,0 +1,20 @@
@@ -108,7 +110,7 @@
+ parameters for O32 ABI. */
+/* { dg-do compile { target { "mips*-*-*" } } } */
+/* { dg-options "-mabi=32" } */
-+/* { dg-skip-if "" { *-*-* } { "-O0" "-Os" } { "" } } */
++/* { dg-skip-if "" { *-*-* } { "-flto" } { "" } } */
+
+typedef struct ui8
+{
@@ -121,10 +123,9 @@
+ return a.v[0];
+}
+
-+/* { dg-final { scan-assembler "\tsw\t\\\$5,100\\(\\\$sp\\)" } } */
-+/* { dg-final { scan-assembler "\tsw\t\\\$6,104\\(\\\$sp\\)" } } */
-+/* { dg-final { scan-assembler "\tsw\t\\\$7,108\\(\\\$sp\\)" } } */
++/* { dg-final { scan-assembler "\tsw\t\\\$5,1\\d\\d\\(\\\$(sp|fp)\\)" } } */
++/* { dg-final { scan-assembler "\tsw\t\\\$6,1\\d\\d\\(\\\$(sp|fp)\\)" } } */
++/* { dg-final { scan-assembler "\tsw\t\\\$7,1\\d\\d\\(\\\$(sp|fp)\\)" } } */
--
-2.34.1
-
+2.30.2
> > Regards,
> > Jovan
>
>
>
> --
> YunQiang Su
> Ohh, my fault: the `-flto` option should always be skipped, when run test.
Right, if tests run with `-flto` option, they will fail. However, I do believe
they are run only if LTO support is enabled, that's why my tests all passed
without explicitly skipping that option.
Your modification looks good to me.
Regards,
Jovan
@@ -6190,6 +6190,22 @@ mips_arg_partial_bytes (cumulative_args_t cum, const function_arg_info &arg)
return info.stack_words > 0 ? info.reg_words * UNITS_PER_WORD : 0;
}
+/* Given MODE and TYPE of a function argument, return the alignment in
+ bits. In case of typedef, alignment of its original type is
+ used. */
+
+static unsigned int
+mips_function_arg_alignment (machine_mode mode, const_tree type)
+{
+ if (!type)
+ return GET_MODE_ALIGNMENT (mode);
+
+ if (is_typedef_decl (TYPE_NAME (type)))
+ type = DECL_ORIGINAL_TYPE (TYPE_NAME (type));
+
+ return TYPE_ALIGN (type);
+}
+
/* Implement TARGET_FUNCTION_ARG_BOUNDARY. Every parameter gets at
least PARM_BOUNDARY bits of alignment, but will be given anything up
to STACK_BOUNDARY bits if the type requires it. */
@@ -6198,8 +6214,8 @@ static unsigned int
mips_function_arg_boundary (machine_mode mode, const_tree type)
{
unsigned int alignment;
+ alignment = mips_function_arg_alignment (mode, type);
- alignment = type ? TYPE_ALIGN (type) : GET_MODE_ALIGNMENT (mode);
if (alignment < PARM_BOUNDARY)
alignment = PARM_BOUNDARY;
if (alignment > STACK_BOUNDARY)
new file mode 100644
@@ -0,0 +1,33 @@
+/* Check that typedef alignment does not affect passing of function
+ parameters. */
+/* { dg-do run { target { "mips*-*-linux*" } } } */
+
+#include <assert.h>
+
+typedef struct ui8
+{
+ unsigned v[8];
+} uint8 __attribute__ ((aligned(64)));
+
+unsigned
+callee (int x, uint8 a)
+{
+ return a.v[0];
+}
+
+uint8
+identity (uint8 in)
+{
+ return in;
+}
+
+int
+main (void)
+{
+ uint8 vec = {{1, 2, 3, 4, 5, 6, 7, 8}};
+ uint8 temp = identity (vec);
+ unsigned temp2 = callee (1, identity (vec));
+ assert (callee (1, temp) == 1);
+ assert (temp2 == 1);
+ return 0;
+}