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

[Jovan Dmitrovic <Jovan.Dmitrovic@Syrmia.com>]

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.
________________________________________
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
>