On Tue, Mar 31, 2026 at 12:21 AM Richard Biener <[email protected]> wrote: > > On Mon, Mar 30, 2026 at 6:34 PM H.J. Lu <[email protected]> wrote: > > > > On Mon, Mar 30, 2026 at 8:28 AM Richard Biener <[email protected]> wrote: > > > > > > > > > > > > > Am 30.03.2026 um 16:44 schrieb H.J. Lu <[email protected]>: > > > > > > > > On Mon, Mar 30, 2026 at 12:28 AM Richard Biener <[email protected]> > > > > wrote: > > > >> > > > >>> On Sat, 28 Mar 2026, H.J. Lu 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. > > > >> > > > >> I don't think this necessarily follows, the caller can end up using > > > >> memcpy > > > >> while portions accessed in the callee can end up using aligned moves. > > > >> Also when the ABI effectively requires less alignment the caller has to > > > >> avoid using aligned accesses even when the formal alignment of the > > > >> argument is bigger. > > > >> > > > >> All this is about ABI mandated/guaranteed alignment being lower than > > > >> the types alignment. > > > > > > > > Shouldn't it be decided by backend? > > > > > > I’m not sure what there is to decide? The > > > Alignment as specified by the ABI is insufficient to guarantee the > > > assignment required for the type. That’s a fact. > > > > > > IMO we should diagnose such cases to the user, suggesting pass by > > > reference rather than by value and then honor the types alignment, doing > > > ‚optimization Where it does not matter‘ possibly in a target specific way > > > - my approach would have been to rely on BIGGEST_ALIGNMENT as that should > > > cover all means of access to an object - possibly further constrained by > > > the objects natural size (hoping alignment padding isn’t accessed). > > > > > > > There are no functional issues when a misaligned argument > > is placed on stack. What benefits does it have to make an > > aligned copy when its address isn't taken? > > You can end up using instructions requiring the declared alignment. > A struct { float a[8]; } __attribute__((aligned (32))); can be accessed > via a vmovaps by doing *(v8sf *)s.a. You'd need to track actually > used alignment to elide the copy.
In order to generate vmovaps, instead of vmovdqu, we align the stack and generate vmovdqu first. It isn't an optimization. > IMO teaching users to avoid over-aligned by-value arguments via > diagnostics is the best thing to do here. They might have expected > register passing, much less stack passing + an extra copy due to > the alignment requirement. > > Richard. > > > > > -- > > H.J. -- H.J.
