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. 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]> -- H.J.
From 868309ed9c6756a0560062da55e17732bf12085a Mon Sep 17 00:00:00 2001 From: "H.J. Lu" <[email protected]> Date: Sat, 28 Jun 2025 06:27:25 +0800 Subject: [PATCH v3] Adjust check for addressable misaligned stack argument 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. 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]> --- gcc/config/i386/i386.cc | 20 +++++++++++- gcc/function.cc | 11 +++++-- gcc/testsuite/gcc.dg/pr124671.c | 34 +++++++++++++++++++++ gcc/testsuite/gcc.target/i386/pr120839-1a.c | 15 +++++++++ gcc/testsuite/gcc.target/i386/pr120839-1b.c | 5 +++ gcc/testsuite/gcc.target/i386/pr120839-2.c | 19 ++++++++++++ 6 files changed, 100 insertions(+), 4 deletions(-) create mode 100644 gcc/testsuite/gcc.dg/pr124671.c create mode 100644 gcc/testsuite/gcc.target/i386/pr120839-1a.c create mode 100644 gcc/testsuite/gcc.target/i386/pr120839-1b.c create mode 100644 gcc/testsuite/gcc.target/i386/pr120839-2.c diff --git a/gcc/config/i386/i386.cc b/gcc/config/i386/i386.cc index 15e0dd547a9..3f7fe0bcf0c 100644 --- a/gcc/config/i386/i386.cc +++ b/gcc/config/i386/i386.cc @@ -8607,6 +8607,20 @@ struct stack_access_data unsigned int *stack_alignment; }; +/* Return true if OP references an argument passed on stack. */ + +static bool +ix86_argument_passed_on_stack_p (const_rtx op) +{ + tree mem_expr = MEM_EXPR (op); + if (mem_expr) + { + tree var = get_base_address (mem_expr); + return TREE_CODE (var) == PARM_DECL; + } + return false; +} + /* Update the maximum stack slot alignment from memory alignment in PAT. */ static void @@ -8622,7 +8636,11 @@ ix86_update_stack_alignment (rtx, const_rtx pat, void *data) auto op = *iter; if (MEM_P (op)) { - if (reg_mentioned_p (p->reg, XEXP (op, 0))) + /* NB: Ignore arguments passed on stack since caller is + responsible to align the outgoing stack for arguments + passed on stack. */ + if (reg_mentioned_p (p->reg, XEXP (op, 0)) + && !ix86_argument_passed_on_stack_p (op)) { unsigned int alignment = MEM_ALIGN (op); diff --git a/gcc/function.cc b/gcc/function.cc index bba05f3380d..b004508dd9a 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,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))) stack_parm = NULL; /* If parm was passed in memory, and we need to convert it on entry, @@ -3714,7 +3719,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/pr124671.c b/gcc/testsuite/gcc.dg/pr124671.c new file mode 100644 index 00000000000..339757f849e --- /dev/null +++ b/gcc/testsuite/gcc.dg/pr124671.c @@ -0,0 +1,34 @@ +/* { dg-do run { target automatic_stack_alignment } } */ +/* { dg-options "-O2" } */ + +#define ALIGNMENT 256 + +#include <stddef.h> + +typedef struct { + long double a; + long double b; +} c __attribute__((aligned(ALIGNMENT))); + +__attribute__ ((noipa)) +void foo (c *p) +{ + if ((((ptrdiff_t) p) & (ALIGNMENT - 1)) != 0) + __builtin_abort (); +} + +__attribute__ ((noipa)) +void +bar (c f) +{ + foo (&f); +} + +c f; + +int +main() +{ + bar (f); + return 0; +} diff --git a/gcc/testsuite/gcc.target/i386/pr120839-1a.c b/gcc/testsuite/gcc.target/i386/pr120839-1a.c new file mode 100644 index 00000000000..d99237f8497 --- /dev/null +++ b/gcc/testsuite/gcc.target/i386/pr120839-1a.c @@ -0,0 +1,15 @@ +/* { dg-do compile } */ +/* { dg-options "-O2" } */ +/* { dg-final { scan-assembler-not "and\[lq\]?\[\\t \]*\\$-32,\[\\t \]*%\[re\]?sp" } } */ + +typedef struct +{ + long double a; + long double b; +} c __attribute__((aligned(32))); +extern double d; +void +bar (c f) +{ + d = f.a; +} diff --git a/gcc/testsuite/gcc.target/i386/pr120839-1b.c b/gcc/testsuite/gcc.target/i386/pr120839-1b.c new file mode 100644 index 00000000000..5362eb8fc5a --- /dev/null +++ b/gcc/testsuite/gcc.target/i386/pr120839-1b.c @@ -0,0 +1,5 @@ +/* { dg-do compile } */ +/* { dg-options "-O2 -mavx2" } */ +/* { dg-final { scan-assembler-not "and\[lq\]?\[\\t \]*\\$-32,\[\\t \]*%\[re\]?sp" } } */ + +#include "pr120839-1a.c" diff --git a/gcc/testsuite/gcc.target/i386/pr120839-2.c b/gcc/testsuite/gcc.target/i386/pr120839-2.c new file mode 100644 index 00000000000..e5b711c966f --- /dev/null +++ b/gcc/testsuite/gcc.target/i386/pr120839-2.c @@ -0,0 +1,19 @@ +/* { dg-do compile } */ +/* { dg-options "-O2" } */ +/* { dg-final { scan-assembler-not "and\[lq\]?\[\\t \]*\\$-32,\[\\t \]*%\[re\]?sp" } } */ + +typedef struct +{ + long double a; + long double b; +} c __attribute__((aligned(32))); +extern c x; +extern double d; +extern void bar (c); +void +foo (void) +{ + x.a = d; + x.b = d; + bar (x); +} -- 2.53.0
