On Fri, 1 Mar 2024, Jakub Jelinek wrote:
> Hi!
>
> When looking at PR114175 (although that bug seems to be now a riscv backend
> bug), I've noticed that for the TYPE_NO_NAMED_ARGS_STDARG_P functions which
> return value through hidden reference, like
> #include <stdarg.h>
>
> struct S { char a[64]; };
> int n;
>
> struct S
> foo (...)
> {
> struct S s = {};
> va_list ap;
> va_start (ap);
> for (int i = 0; i < n; ++i)
> if ((i & 1))
> s.a[0] += va_arg (ap, double);
> else
> s.a[0] += va_arg (ap, int);
> va_end (ap);
> return s;
> }
> we were incorrectly calling assign_parms_setup_varargs twice, once
> at the start of the function and once in
> if (cfun->stdarg && !DECL_CHAIN (parm))
> assign_parms_setup_varargs (&all, &data, false);
> where parm is the last and only "named" parameter.
>
> The first call, guarded with TYPE_NO_NAMED_ARGS_STDARG_P, was added in
> r13-3549 and is needed for int bar (...) etc. functions using
> va_start/va_arg/va_end, otherwise the
> FOR_EACH_VEC_ELT (fnargs, i, parm)
> in which the other call is will not iterate at all. But we shouldn't
> be doing that if we have the hidden return pointer.
>
> With the following patch on the above testcase with -O0 -std=c23 the
> assembly difference is:
> pushq %rbp
> .cfi_def_cfa_offset 16
> .cfi_offset 6, -16
> movq %rsp, %rbp
> .cfi_def_cfa_register 6
> pushq %rbx
> subq $192, %rsp
> .cfi_offset 3, -24
> - movq %rdi, -192(%rbp)
> - movq %rsi, -184(%rbp)
> - movq %rdx, -176(%rbp)
> - movq %rcx, -168(%rbp)
> - movq %r8, -160(%rbp)
> - movq %r9, -152(%rbp)
> - testb %al, %al
> - je .L2
> - movaps %xmm0, -144(%rbp)
> - movaps %xmm1, -128(%rbp)
> - movaps %xmm2, -112(%rbp)
> - movaps %xmm3, -96(%rbp)
> - movaps %xmm4, -80(%rbp)
> - movaps %xmm5, -64(%rbp)
> - movaps %xmm6, -48(%rbp)
> - movaps %xmm7, -32(%rbp)
> -.L2:
> movq %rdi, -312(%rbp)
> movq %rdi, -192(%rbp)
> movq %rsi, -184(%rbp)
> movq %rdx, -176(%rbp)
> movq %rcx, -168(%rbp)
> movq %r8, -160(%rbp)
> movq %r9, -152(%rbp)
> testb %al, %al
> - je .L13
> + je .L12
> movaps %xmm0, -144(%rbp)
> movaps %xmm1, -128(%rbp)
> movaps %xmm2, -112(%rbp)
> movaps %xmm3, -96(%rbp)
> movaps %xmm4, -80(%rbp)
> movaps %xmm5, -64(%rbp)
> movaps %xmm6, -48(%rbp)
> movaps %xmm7, -32(%rbp)
> -.L13:
> +.L12:
> plus some renumbering of labels later on which clearly shows
> that because of this bug, we were saving all the registers twice
> rather then once. With -O2 -std=c23 some of it is DCEd, but we still get
> subq $160, %rsp
> .cfi_def_cfa_offset 168
> - testb %al, %al
> - je .L2
> - movaps %xmm0, 24(%rsp)
> - movaps %xmm1, 40(%rsp)
> - movaps %xmm2, 56(%rsp)
> - movaps %xmm3, 72(%rsp)
> - movaps %xmm4, 88(%rsp)
> - movaps %xmm5, 104(%rsp)
> - movaps %xmm6, 120(%rsp)
> - movaps %xmm7, 136(%rsp)
> -.L2:
> movq %rdi, -24(%rsp)
> movq %rsi, -16(%rsp)
> movq %rdx, -8(%rsp)
> movq %rcx, (%rsp)
> movq %r8, 8(%rsp)
> movq %r9, 16(%rsp)
> testb %al, %al
> - je .L13
> + je .L12
> movaps %xmm0, 24(%rsp)
> movaps %xmm1, 40(%rsp)
> movaps %xmm2, 56(%rsp)
> movaps %xmm3, 72(%rsp)
> movaps %xmm4, 88(%rsp)
> movaps %xmm5, 104(%rsp)
> movaps %xmm6, 120(%rsp)
> movaps %xmm7, 136(%rsp)
> -.L13:
> +.L12:
> difference, i.e. this time not all, but the floating point args
> were conditionally all saved twice.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
OK.
Richard.
> 2024-03-01 Jakub Jelinek <ja...@redhat.com>
>
> * function.cc (assign_parms): Only call assign_parms_setup_varargs
> early for TYPE_NO_NAMED_ARGS_STDARG_P functions if fnargs is empty.
>
> --- gcc/function.cc.jj 2024-01-12 13:47:20.834428745 +0100
> +++ gcc/function.cc 2024-02-29 21:14:35.275889093 +0100
> @@ -3650,7 +3650,8 @@ assign_parms (tree fndecl)
> assign_parms_initialize_all (&all);
> fnargs = assign_parms_augmented_arg_list (&all);
>
> - if (TYPE_NO_NAMED_ARGS_STDARG_P (TREE_TYPE (fndecl)))
> + if (TYPE_NO_NAMED_ARGS_STDARG_P (TREE_TYPE (fndecl))
> + && fnargs.is_empty ())
> {
> struct assign_parm_data_one data = {};
> assign_parms_setup_varargs (&all, &data, false);
>
> Jakub
>
>
--
Richard Biener <rguent...@suse.de>
SUSE Software Solutions Germany GmbH,
Frankenstrasse 146, 90461 Nuernberg, Germany;
GF: Ivo Totev, Andrew McDonald, Werner Knoblich; (HRB 36809, AG Nuernberg)
