On Thu, 26 Mar 2026, Richard Biener wrote:
> 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?
Or INCOMING_STACK_BOUNDARY. But there's this gem in tm.texi:
@defmac MAX_STACK_ALIGNMENT
Biggest stack alignment guaranteed by the backend. Use this macro
to specify the maximum alignment of a variable on stack.
If not defined, the default value is @code{STACK_BOUNDARY}.
@c FIXME: The default should be @code{PREFERRED_STACK_BOUNDARY}.
@c But the fix for PR 32893 indicates that we can only guarantee
@c maximum stack alignment on stack up to @code{STACK_BOUNDARY}, not
@c @code{PREFERRED_STACK_BOUNDARY}, if stack alignment isn't supported.
@end defmac
suggesting STACK_BOUNDARY. But then I think in this context, which
is about correctness, we shouldn't test against what is guaranteed
to work but what is at most possible to attain (apart from "by luck",
of course)?
On target w/o stack-realignment, what does __builtin_alloca_with_align
do for too large alignment? I think you can always round up the size
and adjust the resulting pointer, no?
That said, do you think MAX_SUPPORTED_STACK_ALIGNMENT would work
or do you prefer to make the check different for
SUPPORTS_STACK_ALIGNMENT vs. non-SUPPORTS_STACK_ALIGNMENT targets,
preserving the current check for the latter? It seems only
x86 (and nvptx) supports stack re-alignment.
Richard.
> 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)
