Re: [PATCH] function: Fix another TYPE_NO_NAMED_ARGS_STDARG_P spot

[Richard Biener <rguenther@suse.de>]

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  <jakub@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 <rguenther@suse.de>
SUSE Software Solutions Germany GmbH,
Frankenstrasse 146, 90461 Nuernberg, Germany;
GF: Ivo Totev, Andrew McDonald, Werner Knoblich; (HRB 36809, AG Nuernberg)