On Mon, Mar 30, 2026 at 7:03 AM H.J. Lu <[email protected]> wrote: > > On Mon, Mar 30, 2026 at 6:18 AM Richard Biener <[email protected]> wrote: > > > > On Mon, 30 Mar 2026, H.J. Lu wrote: > > > > > 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. > > > > But that's a separate enhancement, not suitable for stage4. It also > > depends on the ABI, you may remove required re-alignment for targets > > where the ABI isn't as forgiving. It also seems that any such > > change should involve BIGGEST_ALIGNMENT as in my patch. > > BIGGEST_ALIGNMENT is the biggest alignment for all types, > not for each type. > > > Please submit separately for stage1. > > Will do that.
I opened: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=124697 Here is the v4 patch. 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 7a2b99bdcd3379b900cabd829f4088a4a7dcca43 Mon Sep 17 00:00:00 2001 From: "H.J. Lu" <[email protected]> Date: Sat, 28 Jun 2025 06:27:25 +0800 Subject: [PATCH v4] 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 | 12 ++++++-- 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, 101 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 44934d4fbd4..588e7c9d81d 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..46c0d8b54c2 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,13 @@ 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 + /* If its address is taken, make a local copy whose + 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 +3720,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..34d4d4271e2 --- /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" { xfail ia32 } } } */ + +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..d9dafc19afb --- /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" { xfail ia32 } } } */ + +#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
