Re: [PATCH] [PR91400] build only one __cpu_model variable

[Richard Biener <richard.guenther@gmail.com>]

On Mon, May 3, 2021 at 11:31 AM Ivan Sorokin via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> Prior to this commit GCC -O2 generated quite bad code for this
> function:
>
> bool f()
> {
>     return __builtin_cpu_supports("popcnt")
>         && __builtin_cpu_supports("ssse3");
> }
>
> f:
>         movl    __cpu_model+12(%rip), %eax
>         xorl    %r8d, %r8d
>         testb   $4, %al
>         je      .L1
>         shrl    $6, %eax
>         movl    %eax, %r8d
>         andl    $1, %r8d
> .L1:
>         movl    %r8d, %eax
>         ret
>
> The problem was caused by the fact that internally every invocation
> of __builtin_cpu_supports built a new variable __cpu_model and a new
> type __processor_model. Because of this GIMPLE level optimizers
> weren't able to CSE the loads of __cpu_model and optimize
> bit-operations properly.
>
> This commit fixes the problem by caching created __cpu_model
> variable and __processor_model type. Now the GCC -O2 generates:
>
> f:
>         movl    __cpu_model+12(%rip), %eax
>         andl    $68, %eax
>         cmpl    $68, %eax
>         sete    %al
>         ret

The patch looks good, the function could need a comment
and the global variables better names, not starting with __

Up to the x86 maintainers - HJ, can you pick up this work?

Thanks,
Richard.

> gcc/ChangeLog:
>
>         PR target/91400
>         * config/i386/i386-builtins.c (fold_builtin_cpu): Extract
>         building of __cpu_model and __processor_model into new
>         function.
>         * config/i386/i386-builtins.c (init_cpu_model_var): New.
>         Cache creation of __cpu_model and __processor_model.
>
> gcc/testsuite/Changelog:
>
>         PR target/91400
>         * gcc.target/i386/pr91400.c: New.
> ---
>  gcc/config/i386/i386-builtins.c         | 27 ++++++++++++++++++-------
>  gcc/testsuite/gcc.target/i386/pr91400.c | 11 ++++++++++
>  2 files changed, 31 insertions(+), 7 deletions(-)
>  create mode 100644 gcc/testsuite/gcc.target/i386/pr91400.c
>
> diff --git a/gcc/config/i386/i386-builtins.c b/gcc/config/i386/i386-builtins.c
> index b66911082ab..b7d9dd18b03 100644
> --- a/gcc/config/i386/i386-builtins.c
> +++ b/gcc/config/i386/i386-builtins.c
> @@ -2103,6 +2103,25 @@ make_var_decl (tree type, const char *name)
>    return new_decl;
>  }
>
> +static GTY(()) tree __cpu_model_var;
> +static GTY(()) tree __processor_model_type;
> +
> +static void
> +init_cpu_model_var()
> +{
> +  if (__cpu_model_var != NULL_TREE)
> +    {
> +      gcc_assert (__processor_model_type != NULL_TREE);
> +      return;
> +    }
> +
> +  __processor_model_type = build_processor_model_struct ();
> +  __cpu_model_var = make_var_decl (__processor_model_type,
> +                                  "__cpu_model");
> +
> +  varpool_node::add (__cpu_model_var);
> +}
> +
>  /* FNDECL is a __builtin_cpu_is or a __builtin_cpu_supports call that is folded
>     into an integer defined in libgcc/config/i386/cpuinfo.c */
>
> @@ -2114,13 +2133,7 @@ fold_builtin_cpu (tree fndecl, tree *args)
>      = (enum ix86_builtins) DECL_MD_FUNCTION_CODE (fndecl);
>    tree param_string_cst = NULL;
>
> -  tree __processor_model_type = build_processor_model_struct ();
> -  tree __cpu_model_var = make_var_decl (__processor_model_type,
> -                                       "__cpu_model");
> -
> -
> -  varpool_node::add (__cpu_model_var);
> -
> +  init_cpu_model_var ();
>    gcc_assert ((args != NULL) && (*args != NULL));
>
>    param_string_cst = *args;
> diff --git a/gcc/testsuite/gcc.target/i386/pr91400.c b/gcc/testsuite/gcc.target/i386/pr91400.c
> new file mode 100644
> index 00000000000..e8b7d9285f9
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/i386/pr91400.c
> @@ -0,0 +1,11 @@
> +/* PR target/91400 */
> +/* { dg-do compile { target { ! ia32 } } } */
> +/* { dg-options "-O2" } */
> +/* { dg-final { scan-assembler-times "andl" 1 } } */
> +/* { dg-final { scan-assembler-times "68" 2 } } */
> +/* { dg-final { scan-assembler-not "je" } } */
> +
> +_Bool f()
> +{
> +    return __builtin_cpu_supports("popcnt") && __builtin_cpu_supports("ssse3");
> +}
> --
> 2.25.1
>