Re: [PATCH] PR target/66819: Allow indirect sibcall with register arguments

[Uros Bizjak <ubizjak at gmail dot com>]

On Thu, Jul 9, 2015 at 12:54 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
> Indirect sibcall with register arguments is OK when there is register
> available for argument passing.
>
> OK for trunk if there is no regression?
>
>
> H.J.
> ---
> gcc/
>
>         PR target/66819
>         * config/i386/i386.c (ix86_function_ok_for_sibcall): Allow
>         indirect sibcall with register arguments if register available
>         for argument passing.
>         (init_cumulative_args): Set cfun->machine->arg_reg_available_p
>         to cum->nregs != 0.
>         (function_arg_advance_32): Set cfun->machine->arg_reg_available_p
>         to 0 when setting cum->nregs = 0.

Do we also need similar functionality for 64bit ABIs? What happens if
we are out of argument regs there?

>         * config/i386/i386.h (machine_function): Add arg_reg_available_p.
>
> gcc/testsuite/
>
>         PR target/66819
>         * gcc.target/i386/pr66819-1.c: New test.
>         * gcc.target/i386/pr66819-2.c: Likewise.
>         * gcc.target/i386/pr66819-3.c: Likewise.
>         * gcc.target/i386/pr66819-4.c: Likewise.
>         * gcc.target/i386/pr66819-5.c: Likewise.
> ---
>  gcc/config/i386/i386.c                    | 15 +++++++++------
>  gcc/config/i386/i386.h                    |  3 +++
>  gcc/testsuite/gcc.target/i386/pr66819-1.c |  8 ++++++++
>  gcc/testsuite/gcc.target/i386/pr66819-2.c |  8 ++++++++
>  gcc/testsuite/gcc.target/i386/pr66819-3.c | 10 ++++++++++
>  gcc/testsuite/gcc.target/i386/pr66819-4.c | 12 ++++++++++++
>  gcc/testsuite/gcc.target/i386/pr66819-5.c | 10 ++++++++++
>  7 files changed, 60 insertions(+), 6 deletions(-)
>  create mode 100644 gcc/testsuite/gcc.target/i386/pr66819-1.c
>  create mode 100644 gcc/testsuite/gcc.target/i386/pr66819-2.c
>  create mode 100644 gcc/testsuite/gcc.target/i386/pr66819-3.c
>  create mode 100644 gcc/testsuite/gcc.target/i386/pr66819-4.c
>  create mode 100644 gcc/testsuite/gcc.target/i386/pr66819-5.c
>
> diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
> index 54ee6f3..85e59a8 100644
> --- a/gcc/config/i386/i386.c
> +++ b/gcc/config/i386/i386.c
> @@ -5628,12 +5628,12 @@ ix86_function_ok_for_sibcall (tree decl, tree exp)
>        if (!decl
>           || (TARGET_DLLIMPORT_DECL_ATTRIBUTES && DECL_DLLIMPORT_P (decl)))
>         {
> -         if (ix86_function_regparm (type, NULL) >= 3)
> -           {
> -             /* ??? Need to count the actual number of registers to be used,
> -                not the possible number of registers.  Fix later.  */
> -             return false;
> -           }
> +         /* FIXME: The symbol indirect call doesn't need a
> +            call-clobbered register.  But we don't know if
> +            this is a symbol indirect call or not  here.  */
> +         if (ix86_function_regparm (type, NULL) >= 3
> +             && !cfun->machine->arg_reg_available_p)

Isn't enough to look at arg_reg_available here?

> +           return false;
>         }
>      }
>
> @@ -6567,6 +6567,7 @@ init_cumulative_args (CUMULATIVE_ARGS *cum,  /* Argument info to initialize */
>                     ? X86_64_REGPARM_MAX
>                     : X86_64_MS_REGPARM_MAX);
>      }
> +  cfun->machine->arg_reg_available_p = cum->nregs != 0;

false instead of 0. This is a boolean.

>    if (TARGET_SSE)
>      {
>        cum->sse_nregs = SSE_REGPARM_MAX;
> @@ -6636,6 +6637,7 @@ init_cumulative_args (CUMULATIVE_ARGS *cum,  /* Argument info to initialize */
>           else
>             cum->nregs = ix86_function_regparm (fntype, fndecl);
>         }
> +      cfun->machine->arg_reg_available_p = cum->nregs != 0;

IMO, cum->nregs > 0 would be more descriptive.

>        /* Set up the number of SSE registers used for passing SFmode
>          and DFmode arguments.  Warn for mismatching ABI.  */
> @@ -7584,6 +7586,7 @@ pass_in_reg:
>         {
>           cum->nregs = 0;
>           cum->regno = 0;
> +         cfun->machine->arg_reg_available_p = 0;
>         }
>        break;
>
> diff --git a/gcc/config/i386/i386.h b/gcc/config/i386/i386.h
> index 74334ff..0b6e304 100644
> --- a/gcc/config/i386/i386.h
> +++ b/gcc/config/i386/i386.h
> @@ -2479,6 +2479,9 @@ struct GTY(()) machine_function {
>    /* If true, it is safe to not save/restore DRAP register.  */
>    BOOL_BITFIELD no_drap_save_restore : 1;
>
> +  /* If true, there is register available for argument passing.  */
> +  BOOL_BITFIELD arg_reg_available_p : 1;

This is not a predicate, but a boolean flag. Please remove _p from the name.

> +
>    /* During prologue/epilogue generation, the current frame state.
>       Otherwise, the frame state at the end of the prologue.  */
>    struct machine_frame_state fs;
> diff --git a/gcc/testsuite/gcc.target/i386/pr66819-1.c b/gcc/testsuite/gcc.target/i386/pr66819-1.c
> new file mode 100644
> index 0000000..7c8a1ab
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/i386/pr66819-1.c
> @@ -0,0 +1,8 @@
> +/* { dg-do compile { target ia32 } } */
> +/* { dg-options "-O2 -mregparm=3" } */
> +/* { dg-final { scan-assembler-not "call" } } */
> +
> +void foo(void (*bar)(void))
> +{
> +  bar();
> +}
> diff --git a/gcc/testsuite/gcc.target/i386/pr66819-2.c b/gcc/testsuite/gcc.target/i386/pr66819-2.c
> new file mode 100644
> index 0000000..9de4f97
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/i386/pr66819-2.c
> @@ -0,0 +1,8 @@
> +/* { dg-do compile { target ia32 } } */
> +/* { dg-options "-fPIC -O2 -mregparm=3" } */
> +/* { dg-final { scan-assembler-not "call" } } */
> +
> +void foo(void (*bar)(void))
> +{
> +  bar();
> +}
> diff --git a/gcc/testsuite/gcc.target/i386/pr66819-3.c b/gcc/testsuite/gcc.target/i386/pr66819-3.c
> new file mode 100644
> index 0000000..3bc5a34
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/i386/pr66819-3.c
> @@ -0,0 +1,10 @@
> +/* { dg-do compile { target ia32 } } */
> +/* { dg-options "-O2 -mregparm=3" } */
> +/* { dg-final { scan-assembler-not "call" } } */
> +
> +void (*bar)(int, int);
> +
> +void foo(int i, int j)
> +{
> +  bar(i, j);
> +}
> diff --git a/gcc/testsuite/gcc.target/i386/pr66819-4.c b/gcc/testsuite/gcc.target/i386/pr66819-4.c
> new file mode 100644
> index 0000000..18b2ccf
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/i386/pr66819-4.c
> @@ -0,0 +1,12 @@
> +/* { dg-do compile { target ia32 } } */
> +/* { dg-options "-O2 -mregparm=3" } */
> +/* { dg-final { scan-assembler-not "call" } } */
> +
> +#include <stdarg.h>
> +
> +void (*bar)(int, va_list);
> +
> +void foo(int i, va_list args)
> +{
> +  bar(i, args);
> +}
> diff --git a/gcc/testsuite/gcc.target/i386/pr66819-5.c b/gcc/testsuite/gcc.target/i386/pr66819-5.c
> new file mode 100644
> index 0000000..6b019d1
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/i386/pr66819-5.c
> @@ -0,0 +1,10 @@
> +/* { dg-do compile { target ia32 } } */
> +/* { dg-options "-O2 -mregparm=3" } */
> +/* { dg-final { scan-assembler "call" } } */
> +
> +void (*bar)(int, int, int);
> +
> +void foo(int i, int j, int k)
> +{
> +  bar(i, j, k);
> +}
> --
> 2.4.3
>