Richard Biener <[email protected]> writes:
> 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,
> only affecting x86 and nvptx at this point.
>
> Bootstrapped and tested on x86_64-unknown-linux-gnu.
>
> OK?
LGTM
> Btw, for the testcase I see aarch64 producing
>
> e:
> .LFB0:
> .cfi_startproc
> stp x29, x30, [sp, -64]!
> .cfi_def_cfa_offset 64
> .cfi_offset 29, -64
> .cfi_offset 30, -56
> mov x29, sp
> add x0, sp, 47
> and x0, x0, -32
> stp q0, q1, [x0]
> bl __trunctfdf2
> adrp x0, .LANCHOR0
> str d0, [x0, #:lo12:.LANCHOR0]
> ldp x29, x30, [sp], 64
> .cfi_restore 30
> .cfi_restore 29
> .cfi_def_cfa_offset 0
> ret
>
> so it does seem to dynamically align the stack and we expand
>
> (insn 8 7 9 2 (set (mem/c:TF (reg/f:DI 108) [2 f+0 S16 A256])
> (reg:TF 103)) "t.c":5:13 -1
> (nil))
> ...
> (insn 15 14 16 2 (set (reg:TF 32 v0)
> (mem/c:TF (reg/f:DI 108) [1 f.a+0 S16 A256])) "t.c":5:20 -1
> (nil))
>
> but the port doesn't define MAX_STACK_ALIGNMENTm has STACK_BOUNDARY
> 128 and no PREFERRED_STACK_BOUNDARY. Doesn't that tell us there's
> some generic code handling large stack allocations?
Yeah, like you said to in the earlier thread, we can individually
allocate things with higher alignment using alloca with padding.
SUPPORTS_STACK_ALIGNMENT is whether we can align the stack pointer
itself to a higher value, and thus use "static" allocation and constant
offsets even for alignments greater than STACK_BOUNDARY.
So perhaps this can indeed be tweaked for !SUPPORTS_STACK_ALIGNMENT
targets. But any change there should probably be motivated by a
testcase for such a target. You mentioned in the v1 covering note about
wanting something conservative for this late stage. The new patch seems
like a good balance to me FWIW.
Thanks,
Richard
>
> PR middle-end/120839
> * function.cc (assign_parm_adjust_stack_rtl): 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.
>
> * gcc.dg/torture/pr120839.c: New testcase.
> ---
> gcc/function.cc | 4 +++-
> gcc/testsuite/gcc.dg/torture/pr120839.c | 7 +++++++
> 2 files changed, 10 insertions(+), 1 deletion(-)
> create mode 100644 gcc/testsuite/gcc.dg/torture/pr120839.c
>
> diff --git a/gcc/function.cc b/gcc/function.cc
> index bba05f3380d..5b3018f364e 100644
> --- a/gcc/function.cc
> +++ b/gcc/function.cc
> @@ -2840,7 +2840,9 @@ 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) < (SUPPORTS_STACK_ALIGNMENT
> + ? MAX_SUPPORTED_STACK_ALIGNMENT
> + : PREFERRED_STACK_BOUNDARY))))
> stack_parm = NULL;
>
> /* If parm was passed in memory, and we need to convert it on entry,
> diff --git a/gcc/testsuite/gcc.dg/torture/pr120839.c
> b/gcc/testsuite/gcc.dg/torture/pr120839.c
> new file mode 100644
> index 00000000000..158e800649f
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/torture/pr120839.c
> @@ -0,0 +1,7 @@
> +/* { dg-do compile } */
> +
> +typedef struct {
> + long double a, b;
> +} c __attribute__((aligned(32)));
> +double d;
> +void e(c f) { d = f.a; }
