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.
