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. > > > BTW, I opened: > > > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=124671 > > > > with a run-time testcase. > > > > /* If parm was passed in memory, and we need to convert it on entry, > > > >> stack_parm = NULL; > >> > >> /* If parm was passed in memory, and we need to convert it on entry, > >> @@ -3714,7 +3722,7 @@ assign_parms (tree fndecl) > >> else > >> set_decl_incoming_rtl (parm, data.entry_parm, false); > >> > >> - assign_parm_adjust_stack_rtl (&data); > >> + assign_parm_adjust_stack_rtl (parm, &data); > >> > >> if (assign_parm_setup_block_p (&data)) > >> assign_parm_setup_block (&all, parm, &data); > >> 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; } > >> diff --git a/gcc/testsuite/gcc.target/i386/pr120839-avx.c > >> b/gcc/testsuite/gcc.target/i386/pr120839-avx.c > >> new file mode 100644 > >> index 00000000000..1f5406e065c > >> --- /dev/null > >> +++ b/gcc/testsuite/gcc.target/i386/pr120839-avx.c > >> @@ -0,0 +1,8 @@ > >> +/* { dg-do compile } */ > >> +/* { dg-options "-O -mavx" } */ > >> + > >> +typedef struct { > >> + long double a, b; > >> +} c __attribute__((aligned(32))); > >> +double d; > >> +void e(c f) { d = f.a; } > >> -- > >> 2.51.0 > > > > > > > > -- > > H.J. -- H.J.
