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
>
Like this?
diff --git a/gcc/calls.cc b/gcc/calls.cc
index 246abe34243..b85b22264de 100644
--- a/gcc/calls.cc
+++ b/gcc/calls.cc
@@ -5327,7 +5327,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,
Thanks.
> 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
>
--
H.J.
