diff mbox series

function: Fix another TYPE_NO_NAMED_ARGS_STDARG_P spot

Message ID ZeGTFI1hu9XHx+fh@tucnak
State Unresolved
Headers
Series function: Fix another TYPE_NO_NAMED_ARGS_STDARG_P spot |

Checks

Context Check Description
snail/gcc-patch-check warning Git am fail log

Commit Message

Jakub Jelinek March 1, 2024, 8:34 a.m. UTC 
  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?

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.


	Jakub

Comments

Richard Biener March 1, 2024, 9:47 a.m. UTC | #1 
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
> 
>
diff mbox series

Patch

--- 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);