On Mon, Mar 30, 2026 at 1:58 AM Richard Biener <[email protected]> wrote:
>
> On Sun, 29 Mar 2026, H.J. Lu wrote:
>
> > On Sat, Mar 28, 2026 at 5:18 AM H.J. Lu <[email protected]> wrote:
> > >
> > > On Sat, Mar 28, 2026 at 1:16 AM Richard Biener <[email protected]> wrote:
> > > >
> > > >
> > > >
> > > > > Am 28.03.2026 um 03:36 schrieb H.J. Lu <[email protected]>:
> > > > >
> > > > > On Fri, Mar 27, 2026 at 4:14 AM Richard Biener <[email protected]>
> > > > > wrote:
> > > > >>
> > > > >> 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
> > > > >> and the parameter has it's address taken and with BIGGEST_ALIGNMENT
> > > > >> if SUPPORTS_STACK_ALIGNMENT otherwise, only affecting x86 and nvptx
> > > > >> at this point.
> > > > >>
> > > > >> In addition to this it changes the i386 backends computation of
> > > > >> the maximum stack alignment used to bound itself to BIGGEST_ALIGNMENT
> > > > >> because the middle-end does not (and in general cannot) enforce
> > > > >> actual
> > > > >> alignment of all stack objects according to their type.
> > > > >>
> > > > >> Bootstrapped and tested on x86_64-unknown-linux-gnu.
> > > > >>
> > > > >> Compared to v2 this uses BIGGEST_ALIGNMENT if the parameter does not
> > > > >> have its address taken and caps ix86_update_stack_alignment on
> > > > >> BIGGEST_ALIGNMENT as well.
> > > > >>
> > > > >> OK for the x86 parts? I assume the refined function.cc hunk is still
> > > > >> LGTM from Richard S. side.
> > > > >>
> > > > >> Note I still fail to see why we need to scan all insns in the backend
> > > > >> again after RTL expansion should have ensured to keep track of the
> > > > >> maximum needed stack alignment. But I'm trying to avoid touching
> > > > >> code I do not understand as much as possible.
> > > > >>
> > > > >> Thanks,
> > > > >> Richard.
> > > > >>
> > > > >> PR middle-end/120839
> > > > >> * function.cc (assign_parm_adjust_stack_rtl): Get the
> > > > >> parameter
> > > > >> as arugment. 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 if the parameter has its address
> > > > >> taken or BIGGEST_ALIGNMENT otherwise.
> > > > >> (assign_parms): Adjust.
> > > > >> * config/i386/i386.cc (ix86_update_stack_alignment): Bound
> > > > >> recorded stack alignment requirement by BIGGEST_ALIGNMENT.
> > > > >>
> > > > >> * gcc.dg/torture/pr120839.c: New testcase.
> > > > >> * gcc.target/i386/pr120839-avx.c: Likewise.
> > > > >> ---
> > > > >> gcc/config/i386/i386.cc | 2 +-
> > > > >> gcc/function.cc | 14 +++++++++++---
> > > > >> gcc/testsuite/gcc.dg/torture/pr120839.c | 7 +++++++
> > > > >> gcc/testsuite/gcc.target/i386/pr120839-avx.c | 8 ++++++++
> > > > >> 4 files changed, 27 insertions(+), 4 deletions(-)
> > > > >> create mode 100644 gcc/testsuite/gcc.dg/torture/pr120839.c
> > > > >> create mode 100644 gcc/testsuite/gcc.target/i386/pr120839-avx.c
> > > > >>
> > > > >> diff --git a/gcc/config/i386/i386.cc b/gcc/config/i386/i386.cc
> > > > >> index 15e0dd547a9..f2a49bbaf46 100644
> > > > >> --- a/gcc/config/i386/i386.cc
> > > > >> +++ b/gcc/config/i386/i386.cc
> > > > >> @@ -8627,7 +8627,7 @@ ix86_update_stack_alignment (rtx, const_rtx
> > > > >> pat, void *data)
> > > > >> unsigned int alignment = MEM_ALIGN (op);
> > > > >>
> > > > >> if (alignment > *p->stack_alignment)
> > > > >> - *p->stack_alignment = alignment;
> > > > >> + *p->stack_alignment = MIN (alignment,
> > > > >> BIGGEST_ALIGNMENT);
> > > > >
> > > > > This is wrong. X86 backend supports MAX_OFILE_ALIGNMENT
> > > > > stack alignment.
> > > > >
> > > > >> break;
> > > > >> }
> > > > >> else
> > > > >> diff --git a/gcc/function.cc b/gcc/function.cc
> > > > >> index bba05f3380d..41987b4f7a4 100644
> > > > >> --- a/gcc/function.cc
> > > > >> +++ b/gcc/function.cc
> > > > >> @@ -2825,7 +2825,7 @@ assign_parm_remove_parallels (struct
> > > > >> assign_parm_data_one *data)
> > > > >> always valid and properly aligned. */
> > > > >>
> > > > >> static void
> > > > >> -assign_parm_adjust_stack_rtl (struct assign_parm_data_one *data)
> > > > >> +assign_parm_adjust_stack_rtl (tree parm, struct
> > > > >> assign_parm_data_one *data)
> > > > >> {
> > > > >> rtx stack_parm = data->stack_parm;
> > > > >>
> > > > >> @@ -2840,7 +2840,15 @@ 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)))
> > > > >> + /* When we can re-align the stack ensure appropriate
> > > > >> alignment
> > > > >> + of the function local object up to
> > > > >> BIGGEST_ALIGNMENT if
> > > > >> + it is only accessed directly or up to the maximum
> > > > >> supported
> > > > >> + alignment if the address is exposed. */
> > > > >> + && MEM_ALIGN (stack_parm) < (SUPPORTS_STACK_ALIGNMENT
> > > > >> + ? (TREE_ADDRESSABLE (parm)
> > > > >> + ?
> > > > >> MAX_SUPPORTED_STACK_ALIGNMENT
> > > > >> + : BIGGEST_ALIGNMENT)
> > > > >> + :
> > > > >> PREFERRED_STACK_BOUNDARY))))
> > > > >
> > > > > I think it should be
> > > > >
> > > > > @@ -2840,7 +2840,11 @@ 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)))
> > > > > + /* The parm stack slot works if its address isn't taken.
> > > > > When
> > > > > + making a local copy, MAX_SUPPORTED_STACK_ALIGNMENT is
> > > > > + its maximum alignment. */
> > > > > + && TREE_ADDRESSABLE (parm)
> > > > > + && MEM_ALIGN (stack_parm) < MAX_SUPPORTED_STACK_ALIGNMENT)))
> > > > > stack_parm = NULL;
> > > >
> > > > Even when not address taken the ABI guaranteed alignment of the stack
> > > > slot might be less than the declared alignment. If the alignment
> > > > itself is not exposed via the address we have still to avoid faulting,
> > > > thus BIGGEST_ALIGNMENT
> > >
> > > There should be no fault. Otherwise, caller will fault first when
> > > such an argument
> > > is pushed onto stack.
> > >
> >
> > Here is the combined patch with tests.
> >
> > Adjust alignment check forcing a local copy if the argument on stack is
> > not aligned to its type and the current alignment, which is determined
> > by the backend, is less than MAX_SUPPORTED_STACK_ALIGNMENT when the
> > parameter has its address taken.
> >
> > For x86, ignore argument passed on stack when updating stack alignment
> > since caller is responsible to align the outgoing stack for arguments
> > passed on stack.
>
> @@ -2840,7 +2840,12 @@ 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)))
> + /* Use the parm stack slot, whose alignment is determined
> + by targetm.calls.function_arg_boundary, if its address
> + isn't taken. When making a local copy, its maximum
> + alignment is MAX_SUPPORTED_STACK_ALIGNMENT. */
> + && TREE_ADDRESSABLE (parm)
> + && MEM_ALIGN (stack_parm) < MAX_SUPPORTED_STACK_ALIGNMENT)))
>
> This changes the !TREE_ADDRESSABLE (parm) behavior which I think we
> do not want to do at this point. I think we want
>
> && (MEM_ALIGN (stack_parm) < PREFERRED_STACK_BOUNDARY
> || (TREE_ADDRESSABLE (parm)
> && MEM_ALIGN (stack_parm) <
> MAX_SUPPORTED_STACK_ALIGNMENT))
This will cause unnecessary stack realignment for
typedef struct {
double a;
double b;
double c;
double d;
} c __attribute__((aligned(32)));
extern double d;
void
e (float a1, float a2, float a3, float a4, float a5, float a6, c f)
{
d = f.a;
}
It is
pushq %rbp
movq %rsp, %rbp
movsd 16(%rbp), %xmm0
popq %rbp
movsd %xmm0, d(%rip)
ret
vs
pushq %rbp
movq %rsp, %rbp
andq $-32, %rsp
movdqu 16(%rbp), %xmm0
movaps %xmm0, -32(%rsp)
movsd -32(%rsp), %xmm0
movsd %xmm0, d(%rip)
leave
ret
MEM_ALIGN (stack_parm) is properly aligned according to ISA
and ABI as set by target. There is no need to realign stack
if address isn't taken.
> this doesn't affect targets that cannot re-align the stack due
> to the way MAX_SUPPORTED_STACK_ALIGNMENT is derived. For
> unaffected targets the new check is simply redundant
> (MAX_SUPPORTED_STACK_ALIGNMENT is then equal to PREFERRED_STACK_BOUNDARY).
>
> The middle-end part is OK with that change.
>
> I'll leave the x86 part to x86 maintainers.
>
> Thanks,
> Richard.
>
>
> > gcc/
> >
> > PR target/120839
> > PR middle-end/124671
> > * function.cc (assign_parm_adjust_stack_rtl): Get the parameter
> > as argument. Adjust alignment check forcing a local copy.
> > (assign_parms): Adjust.
> > * config/i386/i386.cc (ix86_argument_passed_on_stack_p): New.
> > (ix86_update_stack_alignment): Ignore argument passed on stack.
> >
> > gcc/testsuite/
> >
> > PR target/120839
> > PR middle-end/124671
> > * gcc.dg/pr124671.c: New test.
> > * gcc.target/i386/pr120839-1a.c: Likewise.
> > * gcc.target/i386/pr120839-1b.c: Likewise.
> > * gcc.target/i386/pr120839-2.c: Likewise.
> >
> > Signed-off-by: H.J. Lu <[email protected]>
> > Co-Authored-By: Richard Biener <[email protected]>
> >
> >
>
> --
> 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)
--
H.J.
