On Thu, 26 Mar 2026, Richard Sandiford wrote:
> Richard Biener <[email protected]> writes:
> > The following fixes a confusion seen 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. x86 supports
> > stack-realignment so we can honor the declared alignment and this
> > clears up the confusion. The following replaces the bound
> > by MAX_SUPPORTED_STACK_ALIGNMENT if SUPPORTS_STACK_ALIGNMENT,
> > only affecting x86 and nvptx at this point.
> >
> > Bootstrapped and tested on x86_64-unknown-linux-gnu.
> >
> > OK?
>
> LGTM
>
> > Btw, for the testcase I see aarch64 producing
> >
> > e:
> > .LFB0:
> > .cfi_startproc
> > stp x29, x30, [sp, -64]!
> > .cfi_def_cfa_offset 64
> > .cfi_offset 29, -64
> > .cfi_offset 30, -56
> > mov x29, sp
> > add x0, sp, 47
> > and x0, x0, -32
> > stp q0, q1, [x0]
> > bl __trunctfdf2
> > adrp x0, .LANCHOR0
> > str d0, [x0, #:lo12:.LANCHOR0]
> > ldp x29, x30, [sp], 64
> > .cfi_restore 30
> > .cfi_restore 29
> > .cfi_def_cfa_offset 0
> > ret
> >
> > so it does seem to dynamically align the stack and we expand
> >
> > (insn 8 7 9 2 (set (mem/c:TF (reg/f:DI 108) [2 f+0 S16 A256])
> > (reg:TF 103)) "t.c":5:13 -1
> > (nil))
> > ...
> > (insn 15 14 16 2 (set (reg:TF 32 v0)
> > (mem/c:TF (reg/f:DI 108) [1 f.a+0 S16 A256])) "t.c":5:20 -1
> > (nil))
> >
> > but the port doesn't define MAX_STACK_ALIGNMENTm has STACK_BOUNDARY
> > 128 and no PREFERRED_STACK_BOUNDARY. Doesn't that tell us there's
> > some generic code handling large stack allocations?
>
> Yeah, like you said to in the earlier thread, we can individually
> allocate things with higher alignment using alloca with padding.
> SUPPORTS_STACK_ALIGNMENT is whether we can align the stack pointer
> itself to a higher value, and thus use "static" allocation and constant
> offsets even for alignments greater than STACK_BOUNDARY.
>
> So perhaps this can indeed be tweaked for !SUPPORTS_STACK_ALIGNMENT
> targets. But any change there should probably be motivated by a
> testcase for such a target. You mentioned in the v1 covering note about
> wanting something conservative for this late stage. The new patch seems
> like a good balance to me FWIW.
Yes, disrupting general code as little as possible was my goal. I
have not ruled out the x86 code that ICEs originally is at fault
(and should not ICE), but clearly the stack object was not aligned
according to how it is accessed and the patch fixes that. That
we now ICE in such situation is what regressed (previously we
"only" possibly generate wrong code). Alan correctly mentions
that w/o architectural accesses that require alignment the
alignment is likely only relevant when the object has its address
taken. So possibly using BIGGEST_ALIGNMENT if !TREE_ADDRESSABLE
avoids excessive copying and stack-realignment in some cases.
But doing that re-triggers the ICE in the backend for the testcase
which I can then avoid using -mavx (upping BIGGEST_ALIGNMENT).
I'll send v3 with an additional backend fix.
Richard.
> Thanks,
> Richard
>
> >
> > PR middle-end/120839
> > * function.cc (assign_parm_adjust_stack_rtl): Adjust
> > alignment check forcing a local copy for SUPPORTS_STACK_ALIGNMENT
> > targets if the argument is not aligned to its type and
> > the current alignment is less than MAX_SUPPORTED_STACK_ALIGNMENT.
> >
> > * gcc.dg/torture/pr120839.c: New testcase.
> > ---
> > gcc/function.cc | 4 +++-
> > gcc/testsuite/gcc.dg/torture/pr120839.c | 7 +++++++
> > 2 files changed, 10 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..5b3018f364e 100644
> > --- a/gcc/function.cc
> > +++ b/gcc/function.cc
> > @@ -2840,7 +2840,9 @@ 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) < (SUPPORTS_STACK_ALIGNMENT
> > + ? MAX_SUPPORTED_STACK_ALIGNMENT
> > + : 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)
