Re: [PATCH] x86: Compile CPUID functions with -mgeneral-regs-only

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

On Thu, Jun 24, 2021 at 2:42 PM H.J. Lu <hjl.tools@gmail.com> wrote:
>
> On Thu, Jun 24, 2021 at 5:35 AM Richard Biener
> <richard.guenther@gmail.com> wrote:
> >
> > On Thu, Jun 24, 2021 at 2:13 PM H.J. Lu via Gcc-patches
> > <gcc-patches@gcc.gnu.org> wrote:
> > >
> > > CPUID functions are used to detect CPU features.  If vector ISAs
> > > are enabled, compiler is free to use them in these functions.  Add
> > > __attribute__ ((target("general-regs-only"))) to CPUID functions
> > > to avoid vector instructions.
> >
> > But there are GPR instructions not in x86_64, so shouldn't
> > we use target("march=x86_64") or so?  Note doing either will
> > of course prevent inlining of those "inlines".
>
> Does -march=x86_64, which enables CMOV and other GPR
> ISAs,  work for -m32?

I don't think so.  I'm also not sure whether -march=xyz in a
target attribute overrides -mavx512f on the command-line ;)

> > So I'm not sure how much of a fix this is ... the error will almost
> > always be visible in the caller as well.
>
> I think _attribute__ ((target("general-regs-only"))) is a step
> forward.

That I agree to, but then the cpuid code is likely written the
way it is to allow inlining.  But code using CPUID should best compile
functions under the check with additional target attribute
(or in a separate TU) rather than compiling everything with
extra -mXYZ and trying to "disable" things in the dispatching
code (and the code leading to it!).

Richard.

> > > gcc/
> > >
> > >         PR target/101185
> > >         * config/i386/cpuid.h (__get_cpuid_max): Add
> > >         __attribute__ ((target("general-regs-only"))).
> > >         (__get_cpuid): Likewise.
> > >         (__get_cpuid_count): Likewise.
> > >         (__cpuidex): Likewise.
> > >
> > > gcc/testsuite/
> > >
> > >         PR target/101185
> > >         * gcc.target/i386/avx512-check.h (check_osxsave): Add
> > >         __attribute__ ((target("general-regs-only"))).
> > >         (main): Likewise.
> > > ---
> > >  gcc/config/i386/cpuid.h                      | 4 ++++
> > >  gcc/testsuite/gcc.target/i386/avx512-check.h | 2 ++
> > >  2 files changed, 6 insertions(+)
> > >
> > > diff --git a/gcc/config/i386/cpuid.h b/gcc/config/i386/cpuid.h
> > > index aebc17c6827..74881ee91e5 100644
> > > --- a/gcc/config/i386/cpuid.h
> > > +++ b/gcc/config/i386/cpuid.h
> > > @@ -243,6 +243,7 @@
> > >     pointer is non-null, then first four bytes of the signature
> > >     (as found in ebx register) are returned in location pointed by sig.  */
> > >
> > > +__attribute__ ((target("general-regs-only")))
> > >  static __inline unsigned int
> > >  __get_cpuid_max (unsigned int __ext, unsigned int *__sig)
> > >  {
> > > @@ -298,6 +299,7 @@ __get_cpuid_max (unsigned int __ext, unsigned int *__sig)
> > >     supported and returns 1 for valid cpuid information or 0 for
> > >     unsupported cpuid leaf.  All pointers are required to be non-null.  */
> > >
> > > +__attribute__ ((target("general-regs-only")))
> > >  static __inline int
> > >  __get_cpuid (unsigned int __leaf,
> > >              unsigned int *__eax, unsigned int *__ebx,
> > > @@ -315,6 +317,7 @@ __get_cpuid (unsigned int __leaf,
> > >
> > >  /* Same as above, but sub-leaf can be specified.  */
> > >
> > > +__attribute__ ((target("general-regs-only")))
> > >  static __inline int
> > >  __get_cpuid_count (unsigned int __leaf, unsigned int __subleaf,
> > >                    unsigned int *__eax, unsigned int *__ebx,
> > > @@ -330,6 +333,7 @@ __get_cpuid_count (unsigned int __leaf, unsigned int __subleaf,
> > >    return 1;
> > >  }
> > >
> > > +__attribute__ ((target("general-regs-only")))
> > >  static __inline void
> > >  __cpuidex (int __cpuid_info[4], int __leaf, int __subleaf)
> > >  {
> > > diff --git a/gcc/testsuite/gcc.target/i386/avx512-check.h b/gcc/testsuite/gcc.target/i386/avx512-check.h
> > > index 0a377dba1d5..406faf8fe03 100644
> > > --- a/gcc/testsuite/gcc.target/i386/avx512-check.h
> > > +++ b/gcc/testsuite/gcc.target/i386/avx512-check.h
> > > @@ -25,6 +25,7 @@ do_test (void)
> > >  }
> > >  #endif
> > >
> > > +__attribute__ ((target("general-regs-only")))
> > >  static int
> > >  check_osxsave (void)
> > >  {
> > > @@ -34,6 +35,7 @@ check_osxsave (void)
> > >    return (ecx & bit_OSXSAVE) != 0;
> > >  }
> > >
> > > +__attribute__ ((target("general-regs-only")))
> > >  int
> > >  main ()
> > >  {
> > > --
> > > 2.31.1
> > >
>
>
>
> --
> H.J.