"H.J. Lu" <hjl.to...@gmail.com> writes:
> On Wed, Nov 20, 2024 at 2:12 AM Richard Sandiford
> <richard.sandif...@arm.com> wrote:
>>
>> "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;
>> }
>
> Does this
>
> diff --git a/gcc/calls.cc b/gcc/calls.cc
> index 246abe34243..98429cc757f 100644
> --- a/gcc/calls.cc
> +++ b/gcc/calls.cc
> @@ -5327,7 +5327,13 @@ store_one_arg (struct arg_data *arg, rtx
> argblock, int flags,
> 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))
> + poly_int64 aligned_size;
> + if (CONST_INT_P (size_rtx))
> + aligned_size = ROUND_UP (UINTVAL (size_rtx),
> + parm_align / BITS_PER_UNIT);
> + else
> + aligned_size = size_val;
> + if (maybe_ne (arg->locate.size.constant, aligned_size))
> sibcall_failure = true;
> }
> else if (maybe_in_range_p (arg->locate.offset.constant,
>
> look correct?
Heh. Playing the reviewer here, I was kind-of hoping you'd explain
why it was correct to me :)
But conceptually, the call is copying from arg->value to arg->locate.
And this code is trying to detect whether the copy is a nop, whether
it overlaps, or whether the source and destination are distinct.
It feels odd to grow the size of arg->value (the source of the copy),
given that the extra bytes shouldn't be copied.
Like I mentioned in the previous reply, it feels to me like testing
partial != 0 would deal with the situation described in the comment,
and the testcase in PR32602. Personally I think we should try that first.
Maybe it's wrong (quite probable), but if so, it'd be good to know and
understand why.
If that fails, perhaps a fallback would be to test:
maybe_lt (arg->locate.size.constant, size_val)
instead. I think the code is worried about the case where the stack
slot is smaller than the number of bytes in the source value (because
some of the source value is passed in registers instead). But the fact
that arg->locate.size includes padding seems to undermine the current
test, which is why partial != 0 seems like it would be more direct
and robust (if it works).
Thanks,
Richard
