On Thu, Nov 21, 2024 at 6:43 AM H.J. Lu <hjl.to...@gmail.com> wrote:
> On Wed, Nov 20, 2024 at 9:55 PM Richard Sandiford <
> richard.sandif...@arm.com> wrote:
>
>> "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
>>
>
The outgoing stack slot size may be different from the BLKmode argument
size due to parameter alignment. Check partial != 0 for BLKmode argument
passed on stack.
gcc/
PR middle-end/117098
* calls.cc (store_one_arg): Check artial != 0 for BLKmode argument
passed on stack.
gcc/testsuite/
PR middle-end/117098
* gcc.dg/sibcall-12.c: New test.
OK for master?
Thanks.
--
H.J.
From 055310301cd17247b156b2b14fc19aaaa0eec4ae 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 v2] sibcall: Check partial != 0 for BLKmode argument
The outgoing stack slot size may be different from the BLKmode argument
size due to parameter alignment. Check partial != 0 for BLKmode argument
passed on stack.
gcc/
PR middle-end/117098
* calls.cc (store_one_arg): Check artial != 0 for BLKmode argument
passed on stack.
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 | 2 +-
gcc/testsuite/gcc.dg/sibcall-12.c | 13 +++++++++++++
2 files changed, 14 insertions(+), 1 deletion(-)
create mode 100644 gcc/testsuite/gcc.dg/sibcall-12.c
diff --git a/gcc/calls.cc b/gcc/calls.cc
index f67067acad4..01c4ef51545 100644
--- a/gcc/calls.cc
+++ b/gcc/calls.cc
@@ -5246,7 +5246,7 @@ 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))
+ if (partial != 0)
sibcall_failure = true;
}
else if (maybe_in_range_p (arg->locate.offset.constant,
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);
+}
--
2.47.0
