Re: [PATCH] mips: Fix overaligned function arguments [PR109435]

[YunQiang Su <wzssyqa@gmail.com>]

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



-- 
YunQiang Su