> Am 27.03.2026 um 16:33 schrieb H.J. Lu <[email protected]>:
>
> On Fri, Mar 27, 2026 at 6:49 AM Uros Bizjak <[email protected]> wrote:
>>
>>> On Fri, Mar 27, 2026 at 2:16 PM Richard Biener <[email protected]> wrote:
>>>
>>>> On Fri, 27 Mar 2026, Uros Bizjak wrote:
>>>
>>>> On Fri, Mar 27, 2026 at 1:25 PM Uros Bizjak <[email protected]> wrote:
>>>>>
>>>>> On Fri, Mar 27, 2026 at 12:14 PM 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.
>>>>>
>>>>> I don't think capping ix86_update_stack_alignment to BIGGEST_ALIGNMENT
>>>>> is correct. For TARGET_IAMCU, biggest alignment is 32, but it still
>>>>> can use FXSAVE that wants 16-byte (128-bit) alignment.
>>>>> set_fast_math_sse in libgcc/config/i386/crtfastmath.c uses:
>>>>>
>>>>> /* Check if DAZ is available. */
>>>>> struct
>>>>> {
>>>>> unsigned short cwd;
>>>>> unsigned short swd;
>>>>> unsigned short twd;
>>>>> unsigned short fop;
>>>>> unsigned int fip;
>>>>> unsigned int fcs;
>>>>> unsigned int foo;
>>>>> unsigned int fos;
>>>>> unsigned int mxcsr;
>>>>> unsigned int mxcsr_mask;
>>>>> unsigned int st_space[32];
>>>>> unsigned int xmm_space[32];
>>>>> unsigned int padding[56];
>>>>> } __attribute__ ((aligned (16))) fxsave;
>>>>>
>>>>> which will probably fail with your x86 change due to unsatisfied
>>>>> alignment requirements.
>>>>
>>>> This testcase illustrates above:
>>>>
>>>> --cut here--
>>>> void foo (void)
>>>> {
>>>> struct
>>>> {
>>>> unsigned short cwd;
>>>> unsigned short swd;
>>>> unsigned short twd;
>>>> unsigned short fop;
>>>> unsigned int fip;
>>>> unsigned int fcs;
>>>> unsigned int foo;
>>>> unsigned int fos;
>>>> unsigned int mxcsr;
>>>> unsigned int mxcsr_mask;
>>>> unsigned int st_space[32];
>>>> unsigned int xmm_space[32];
>>>> unsigned int padding[56];
>>>> } __attribute__ ((aligned (16))) fxsave;
>>>>
>>>> __builtin_ia32_fxsave(&fxsave);
>>>> }
>>>> --cut here--
>>>>
>>>> gcc -O2 -miamcu -m32 -mfxsr:
>>>>
>>>> foo:
>>>> pushl %ebp
>>>> movl %esp, %ebp
>>>> andl $-16, %esp
>>>> subl $512, %esp
>>>> fxsave (%esp)
>>>> leave
>>>> ret
>>>>
>>>> The compiler realigns the stack without problems, even if
>>>> BIGGEST_ALIGNMENT for TARGET_IAMCU is 32 bits.
>>>
>>> With the proposed patch it still does:
>>>
>>> foo:
>>> .LFB0:
>>> .cfi_startproc
>>> pushl %ebp
>>> .cfi_def_cfa_offset 8
>>> .cfi_offset 5, -8
>>> movl %esp, %ebp
>>> .cfi_def_cfa_register 5
>>> andl $-16, %esp
>>> subl $512, %esp
>>> fxsave (%esp)
>>> leave
>>> .cfi_restore 5
>>> .cfi_def_cfa 4, 4
>>> ret
>>>
>>> But that's likely because it takes the address of fxsave.
>>
>> Great, this was my biggest concern with the patch. If some variable is
>> defined with __attribute__((aligned)), then the compiler should emit
>> stack realignment code to grant requested alignment.
>>
>>> I'll note that I'd view BIGGEST_ALIGNMENT to be wrong, as its
>>> documentation says:
>>>
>>> "@defmac BIGGEST_ALIGNMENT
>>> Biggest alignment that any data type can require on this machine, in
>>> bits. Note that this is not the biggest alignment that is supported,
>>> just the biggest alignment that, when violated, may cause a fault.
>>> @end defmac"
>>
>> Under this explanation, the IA32MCU definition looks OK.
>>
>>> I'll also note that the scanning in ix86_update_stack_alignment is
>>> in addition(?) to what tracking of stack alignment needs is doing
>>> during RTL expansion.
>>>
>>> The alternative is to go back to v2 but that would cause extra
>>> stack copies to align variables where the extra alignment would
>>> not have an observable effect.
>>
>> Let's wait for HJ's opinion, but based on the definition of
>> BIGGEST_ALIGNEMNT, I think the x86 part should be OK.
>>
>> Uros.
>
> The issue is that DECL_ALIGN of PARM_DECL is limited by
> targetm.calls.function_arg_boundary no matter whatever value
> of DECL_ALIGN is. I am testing this patch.
>
> Since ix86_function_arg_boundary may ignore type alignment for arguments
> passed on stack, call ix86_function_arg_boundary to get the actual
> alignment of arguments passed on stack.
As shown in the bug that there is no properly aligned stack copy is a bug. At
least up to BIGGEST_ALIGNMENT this can make a meaningful difference.
So I believe this patch is wrong.
Why we now ICE in this case (we created wrong code for a long time) I do not
know. To resolve the P1 regression we do not need to fix the wrong code issue,
but only the ICE.
My patch fixes both. For your patch, why not simply ignore all PARM_DECL based
accesses? I’ll note the MEM_EXPR will also show on an eventual existing
properly aligned stack copy which you’d then mistreat?
Richard
> gcc/
>
> PR target/120839
> * config/i386/i386.cc (ix86_get_alignment_on_stack): New.
> (ix86_update_stack_alignment): Call ix86_get_alignment_on_stack
> to get alignment on stack.
>
> gcc/testsuite/
>
> PR target/120839
> * gcc.target/i386/pr120839-1a.c: New test.
> * gcc.target/i386/pr120839-1b.c: Likewise.
> * gcc.target/i386/pr120839-2.c: Likewise.
>
>
> --
> H.J.
> <0001-x86-Call-ix86_function_arg_boundary-to-get-alignment.patch>
