On Thu, 26 Mar 2026, Richard Sandiford wrote:
> Richard Biener <[email protected]> writes:
> > The following fixes a confusion seein in the x86 backend by
> > assign_parm_adjust_stack_rtl failing to trigger a local stack copy
> > for an incoming stack parameter that is not aligned according to
> > its type. The condition was introduced in r0-64961-gbfc45551d5ace4
> > but there is the MEM_ALIGN (stack_parm) < PREFERRED_STACK_BOUNDARY
> > condition not triggering for the case in question where both
> > MEM_ALIGN and PREFERRED_STACK_BOUNDARY are 128. I cannot really
> > make sense of the condition - taking MEM_ALIGN (stack_parm)
> > as the ABI mandated alignment of the argument stack slot means
> > that is a lower bound on the incoming stack boundary. But
> > PREFERRED_STACK_BOUNDARY is not the actual boundary and relating
> > it to a minimum known boundary to somehow infer known bigger
> > alignment of the incoming parameter looks odd.
>
> I have no memory of this patch, but my reading is that
> PREFERRED_STACK_BOUNDARY is being used as the maximum attainable
> alignment. In other words, there is no point discarding the old
> underaligned stack slot if the new one wouldn't guarantee a
> greater alignment. According to this assumption, GCC's stack
> allocation code wouldn't preserve an alignment greater than
> PREFERRED_STACK_BOUNDARY even if the incoming stack pointer
> happened to be more aligned.
>
> However, this predates the dynamic stack realignment code
> (r0-88648-g2e3f842fe69ce4), so perhaps we should skip the
> PREFERRED_STACK_BOUNDARY check on targets that use that?
>
> E.g. on aarch64, if a type claims to have 32-byte alignment,
> GCC won't try to provide that alignment for arguments passed
> on the stack, whereas x86 will. https://godbolt.org/z/eWjfcoPbb
> is a not very good example. (Not very good because the parameter is
> passed by indirect reference on aarch64. I think the point still
> stands though.)
On x86 it doesn't seem to happen always (as in the PRs testcase).
But I wonder if when the ABI does not guarantee sufficient alignment
of the argument stack slot, and the target does not provide a way
to dynamically increase stack alignment to make the copy destination
properly aligned - what should we do? Don't we want to sorry ()
in that case? Because we have happily assumed the PARM_DECLs
alignment as written.
I had assumed that dynamic stack re-alignment code works everywhere
(where alloca works).
So you're suggesting to replace PREFERRED_STACK_BOUNDARY with
MAX_SUPPORTED_STACK_ALIGNMENT? And <= here.
>
> Changing < to <= doesn't look right for other targets though.
> It's likely to lead to pointless copying.
Why? If the stack can be aligned to PREFERRED_STACK_BOUNDARY
but the ABI misaligns the incoming stack slot we can very well
create a suitably aligned stack slot? So in fact the < is what
is odd in the first place? Shouldn't it be testing
STACK_BOUNDARY and not PREFERRED_STACK_BOUNDARY?
Thanks,
Richard.
>
> Thanks,
> Richard
>
> >
> > As minimal surgery at this point I'm not removing the condition
> > but make it include PREFERRED_STACK_BOUNDARY to make the copy
> > for the testcase in question. Correctness-wise I believe the
> > condition should be elided.
> >
> > Alan - you added this check, can you possibly guess what it was
> > supposed to guard against? Richard H., you approved the change in the
> > end, what's the reason for the PREFERRED_STACK_BOUNDARY guard? Richard
> > S., you became involved in the discussion at the end as well.
> >
> > Bootstrapped and tested on x86_64-unknown-linux-gnu.
> >
> > OK for trunk?
> >
> > Thanks,
> > Richard.
> >
> > PR middle-end/120839
> > * function.cc (assign_parm_adjust_stack_rtl): Adjust
> > alignment check forcing a local copy.
> >
> > * gcc.dg/torture/pr120839.c: New testcase.
> > ---
> > gcc/function.cc | 2 +-
> > gcc/testsuite/gcc.dg/torture/pr120839.c | 7 +++++++
> > 2 files changed, 8 insertions(+), 1 deletion(-)
> > create mode 100644 gcc/testsuite/gcc.dg/torture/pr120839.c
> >
> > diff --git a/gcc/function.cc b/gcc/function.cc
> > index bba05f3380d..e93d12785a5 100644
> > --- a/gcc/function.cc
> > +++ b/gcc/function.cc
> > @@ -2840,7 +2840,7 @@ assign_parm_adjust_stack_rtl (struct
> > assign_parm_data_one *data)
> > MEM_ALIGN (stack_parm))))
> > || (data->nominal_type
> > && TYPE_ALIGN (data->nominal_type) > MEM_ALIGN (stack_parm)
> > - && MEM_ALIGN (stack_parm) < PREFERRED_STACK_BOUNDARY)))
> > + && MEM_ALIGN (stack_parm) <= PREFERRED_STACK_BOUNDARY)))
> > stack_parm = NULL;
> >
> > /* If parm was passed in memory, and we need to convert it on entry,
> > diff --git a/gcc/testsuite/gcc.dg/torture/pr120839.c
> > b/gcc/testsuite/gcc.dg/torture/pr120839.c
> > new file mode 100644
> > index 00000000000..158e800649f
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.dg/torture/pr120839.c
> > @@ -0,0 +1,7 @@
> > +/* { dg-do compile } */
> > +
> > +typedef struct {
> > + long double a, b;
> > +} c __attribute__((aligned(32)));
> > +double d;
> > +void e(c f) { d = f.a; }
>
--
Richard Biener <[email protected]>
SUSE Software Solutions Germany GmbH,
Frankenstrasse 146, 90461 Nuernberg, Germany;
GF: Jochen Jaser, Andrew McDonald, Werner Knoblich; (HRB 36809, AG Nuernberg)
