On Thu, 26 Mar 2026, Richard Biener wrote:
> 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?
Ah, of course. But shouldn't this test
TYPE_ALIGN (data->nominal_type) <= PREFERRED_STACK_BOUNDARY? Even
if MEM_ALIGN (stack_parm) is lees than PREFERRED_STACK_BOUNDARY,
what's the point in copying when we still cannot get
TYPE_ALIGN (data->nominal_type) alignment?
> 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.
I'm testing sth along that line.
Richard.
> 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)
