"H.J. Lu" <hjl.to...@gmail.com> writes:
> Adjust BLKmode argument size for parameter alignment for sibcall check.
>
> gcc/
>
> PR middle-end/117098
> * calls.cc (store_one_arg): Adjust BLKmode argument size for
> alignment padding for sibcall check.
>
> gcc/testsuite/
>
> PR middle-end/117098
> * gcc.dg/sibcall-12.c: New test.
>
> OK for master?
>
>
> H.J.
> From 8b0518906cb23a9b5e77b04d6132c49047daebd2 Mon Sep 17 00:00:00 2001
> From: "H.J. Lu" <hjl.to...@gmail.com>
> Date: Sun, 13 Oct 2024 04:53:14 +0800
> Subject: [PATCH] sibcall: Adjust BLKmode argument size for alignment padding
>
> Adjust BLKmode argument size for parameter alignment for sibcall check.
>
> gcc/
>
> PR middle-end/117098
> * calls.cc (store_one_arg): Adjust BLKmode argument size for
> alignment padding for sibcall check.
>
> gcc/testsuite/
>
> PR middle-end/117098
> * gcc.dg/sibcall-12.c: New test.
>
> Signed-off-by: H.J. Lu <hjl.to...@gmail.com>
> ---
> gcc/calls.cc | 4 +++-
> gcc/testsuite/gcc.dg/sibcall-12.c | 13 +++++++++++++
> 2 files changed, 16 insertions(+), 1 deletion(-)
> create mode 100644 gcc/testsuite/gcc.dg/sibcall-12.c
>
> diff --git a/gcc/calls.cc b/gcc/calls.cc
> index c5c26f65280..163c7e509d9 100644
> --- a/gcc/calls.cc
> +++ b/gcc/calls.cc
> @@ -5236,7 +5236,9 @@ store_one_arg (struct arg_data *arg, rtx argblock, int
> flags,
> /* expand_call should ensure this. */
> gcc_assert (!arg->locate.offset.var
> && arg->locate.size.var == 0);
> - poly_int64 size_val = rtx_to_poly_int64 (size_rtx);
> + /* Adjust for argument alignment padding. */
> + poly_int64 size_val = ROUND_UP (UINTVAL (size_rtx),
> + parm_align / BITS_PER_UNIT);
This doesn't look right to me. For one thing, going from
rtx_to_poly_int64 to UINTVAL drops support for non-constant parameters.
But even ignoring that, I think padding size_val (the size of arg->value
IIUC) will pessimise the later:
else if (maybe_in_range_p (arg->locate.offset.constant,
i, size_val))
sibcall_failure = true;
and so cause sibcall failures elsewhere. I'm also not sure this
accurately reproduces the padding that is added by locate_and_pad_parm
for all cases (arguments that grow up vs down, padding below vs above
the argument).
AIUI, the point of the:
if (known_eq (arg->locate.offset.constant, i))
{
/* Even though they appear to be at the same location,
if part of the outgoing argument is in registers,
they aren't really at the same location. Check for
this by making sure that the incoming size is the
same as the outgoing size. */
if (maybe_ne (arg->locate.size.constant, size_val))
sibcall_failure_1 = true;
}
that you cite in the PR is to make sure that the nth byte of arg->value
corresponds to arg->locate.offset.constant + n. It's not clear to me
why the original fix for PR32602 didn't just check partial != 0.
Perhaps it was just for consistency with the neighbouring overlap check,
or maybe it was for some deeper reason.
Thanks,
Richard
>
> if (known_eq (arg->locate.offset.constant, i))
> {
> diff --git a/gcc/testsuite/gcc.dg/sibcall-12.c
> b/gcc/testsuite/gcc.dg/sibcall-12.c
> new file mode 100644
> index 00000000000..5773c9c1c4a
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/sibcall-12.c
> @@ -0,0 +1,13 @@
> +// Test for sibcall optimization with struct aligned on stack.
> +// { dg-options "-O2" }
> +// { dg-final { scan-assembler "jmp" { target i?86-*-* x86_64-*-* } } }
> +
> +struct A { char a[17]; };
> +
> +int baz (int a, int b, int c, void *p, struct A s, struct A);
> +
> +int
> +foo (int a, int b, int c, void *p, struct A s, struct A s2)
> +{
> + return baz (a, b, c, p, s, s2);
> +}
