[PATCH] x86: Move cpuinfo.h from libgcc to common/config/i386

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] x86: Move cpuinfo.h from libgcc to common/config/i386

gcc - fortran mailing list
On Mon, May 18, 2020 at 5:57 AM H.J. Lu <[hidden email]> wrote:

>
> On Mon, May 18, 2020 at 5:43 AM Uros Bizjak <[hidden email]> wrote:
> >
> > On Mon, May 18, 2020 at 2:34 PM H.J. Lu <[hidden email]> wrote:
> > >
> > > On Mon, May 18, 2020 at 5:18 AM Uros Bizjak <[hidden email]> wrote:
> > > >
> > > > On Mon, May 18, 2020 at 1:58 PM H.J. Lu <[hidden email]> wrote:
> > > > >
> > > > > Add cpu model numbers for Intel Airmont, Tremont, Comet Lake, Ice Lake
> > > > > and Tiger Lake processor families.
> > > > >
> > > > > OK for master?
> > > >
> > > > OK.
> > >
> > > I am checking in my patch.
> > >
> > > > Please also update cpuinfo.c from libgcc and corresponding
> > >
> > > I will take a look to see if we share the same CPU detection code between
> > > libgcc and config/i386/driver-i386.c.
> >
> > I don't think it will bring any benefit, this is mainly one huge
> > switch statement that maps to different stuff in libgcc and
> > driver-i386.
>
> libgcc and config/i386/driver-i386.c differ even before my patch.
> I think we can do better.
>
Move cpuinfo.h from libgcc to common/config/i386 so that get_intel_cpu
can be shared by libgcc, GCC driver, gcc.target/i386/builtin_target.c
and libgfortran to detect the specific type of Intel CPU.  Update
libgfortran to use has_cpu_feature to detect x86 CPU features.

Tested on Linux/x86 and Linux/x86-64.  OK for master?

Thanks.

--
H.J.

0001-x86-Move-cpuinfo.h-from-libgcc-to-common-config-i386.patch (87K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] x86: Move cpuinfo.h from libgcc to common/config/i386

gcc - fortran mailing list
On Tue, May 19, 2020 at 4:17 AM H.J. Lu <[hidden email]> wrote:

>
> On Mon, May 18, 2020 at 5:57 AM H.J. Lu <[hidden email]> wrote:
> >
> > On Mon, May 18, 2020 at 5:43 AM Uros Bizjak <[hidden email]> wrote:
> > >
> > > On Mon, May 18, 2020 at 2:34 PM H.J. Lu <[hidden email]> wrote:
> > > >
> > > > On Mon, May 18, 2020 at 5:18 AM Uros Bizjak <[hidden email]> wrote:
> > > > >
> > > > > On Mon, May 18, 2020 at 1:58 PM H.J. Lu <[hidden email]> wrote:
> > > > > >
> > > > > > Add cpu model numbers for Intel Airmont, Tremont, Comet Lake, Ice Lake
> > > > > > and Tiger Lake processor families.
> > > > > >
> > > > > > OK for master?
> > > > >
> > > > > OK.
> > > >
> > > > I am checking in my patch.
> > > >
> > > > > Please also update cpuinfo.c from libgcc and corresponding
> > > >
> > > > I will take a look to see if we share the same CPU detection code between
> > > > libgcc and config/i386/driver-i386.c.
> > >
> > > I don't think it will bring any benefit, this is mainly one huge
> > > switch statement that maps to different stuff in libgcc and
> > > driver-i386.
> >
> > libgcc and config/i386/driver-i386.c differ even before my patch.
> > I think we can do better.
> >
>
> Move cpuinfo.h from libgcc to common/config/i386 so that get_intel_cpu
> can be shared by libgcc, GCC driver, gcc.target/i386/builtin_target.c
> and libgfortran to detect the specific type of Intel CPU.  Update
> libgfortran to use has_cpu_feature to detect x86 CPU features.
>
> Tested on Linux/x86 and Linux/x86-64.  OK for master?

Handling only Intel targets and not others is a sure way for patch to
be ignored.

Uros.
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] x86: Move cpuinfo.h from libgcc to common/config/i386

gcc - fortran mailing list
In reply to this post by gcc - fortran mailing list
Am 19.05.20 um 04:16 schrieb H.J. Lu via Fortran:
> Tested on Linux/x86 and Linux/x86-64.  OK for master?

Libfortran parts are OK.

Regards

        Thomas
Reply | Threaded
Open this post in threaded view
|

Re: V2 [PATCH] x86: Move cpuinfo.h from libgcc to common/config/i386

gcc - fortran mailing list
In reply to this post by gcc - fortran mailing list
On Tue, May 19, 2020 at 2:07 PM Uros Bizjak <[hidden email]> wrote:

>
> On Tue, May 19, 2020 at 9:58 PM H.J. Lu <[hidden email]> wrote:
> >
> > On Mon, May 18, 2020 at 10:56 PM Uros Bizjak <[hidden email]> wrote:
> > >
> > > On Tue, May 19, 2020 at 4:17 AM H.J. Lu <[hidden email]> wrote:
> > > >
> > > > On Mon, May 18, 2020 at 5:57 AM H.J. Lu <[hidden email]> wrote:
> > > > >
> > > > > On Mon, May 18, 2020 at 5:43 AM Uros Bizjak <[hidden email]> wrote:
> > > > > >
> > > > > > On Mon, May 18, 2020 at 2:34 PM H.J. Lu <[hidden email]> wrote:
> > > > > > >
> > > > > > > On Mon, May 18, 2020 at 5:18 AM Uros Bizjak <[hidden email]> wrote:
> > > > > > > >
> > > > > > > > On Mon, May 18, 2020 at 1:58 PM H.J. Lu <[hidden email]> wrote:
> > > > > > > > >
> > > > > > > > > Add cpu model numbers for Intel Airmont, Tremont, Comet Lake, Ice Lake
> > > > > > > > > and Tiger Lake processor families.
> > > > > > > > >
> > > > > > > > > OK for master?
> > > > > > > >
> > > > > > > > OK.
> > > > > > >
> > > > > > > I am checking in my patch.
> > > > > > >
> > > > > > > > Please also update cpuinfo.c from libgcc and corresponding
> > > > > > >
> > > > > > > I will take a look to see if we share the same CPU detection code between
> > > > > > > libgcc and config/i386/driver-i386.c.
> > > > > >
> > > > > > I don't think it will bring any benefit, this is mainly one huge
> > > > > > switch statement that maps to different stuff in libgcc and
> > > > > > driver-i386.
> > > > >
> > > > > libgcc and config/i386/driver-i386.c differ even before my patch.
> > > > > I think we can do better.
> > > > >
> > > >
> > > > Move cpuinfo.h from libgcc to common/config/i386 so that get_intel_cpu
> > > > can be shared by libgcc, GCC driver, gcc.target/i386/builtin_target.c
> > > > and libgfortran to detect the specific type of Intel CPU.  Update
> > > > libgfortran to use has_cpu_feature to detect x86 CPU features.
> > > >
> > > > Tested on Linux/x86 and Linux/x86-64.  OK for master?
> > >
> > > Handling only Intel targets and not others is a sure way for patch to
> > > be ignored.
> > >
> >
> > Here is the updated patch to cover AMD CPU.  It also fixes:
> >
> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95212
> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95220
> >
> > OK for master?
>
> Huh... I didn't think the solution will be this messy... I have to
> rethink the approach a bit.

That is what will happen when we have the same info in more than one place
There should only one place for CPU info.

> Can you in the meantime please prepare a simple patch to fix the above
> mentioned PRs and eventually backport it to other release branches? It
> should be simple enough to be committed under obvious rule.
>

Done:

https://gcc.gnu.org/pipermail/gcc-patches/2020-May/546088.html

--
H.J.
Reply | Threaded
Open this post in threaded view
|

Re: V2 [PATCH] x86: Move cpuinfo.h from libgcc to common/config/i386

gcc - fortran mailing list
In reply to this post by gcc - fortran mailing list
On Tue, May 19, 2020 at 2:07 PM Uros Bizjak <[hidden email]> wrote:

>
> On Tue, May 19, 2020 at 9:58 PM H.J. Lu <[hidden email]> wrote:
> >
> > On Mon, May 18, 2020 at 10:56 PM Uros Bizjak <[hidden email]> wrote:
> > >
> > > On Tue, May 19, 2020 at 4:17 AM H.J. Lu <[hidden email]> wrote:
> > > >
> > > > On Mon, May 18, 2020 at 5:57 AM H.J. Lu <[hidden email]> wrote:
> > > > >
> > > > > On Mon, May 18, 2020 at 5:43 AM Uros Bizjak <[hidden email]> wrote:
> > > > > >
> > > > > > On Mon, May 18, 2020 at 2:34 PM H.J. Lu <[hidden email]> wrote:
> > > > > > >
> > > > > > > On Mon, May 18, 2020 at 5:18 AM Uros Bizjak <[hidden email]> wrote:
> > > > > > > >
> > > > > > > > On Mon, May 18, 2020 at 1:58 PM H.J. Lu <[hidden email]> wrote:
> > > > > > > > >
> > > > > > > > > Add cpu model numbers for Intel Airmont, Tremont, Comet Lake, Ice Lake
> > > > > > > > > and Tiger Lake processor families.
> > > > > > > > >
> > > > > > > > > OK for master?
> > > > > > > >
> > > > > > > > OK.
> > > > > > >
> > > > > > > I am checking in my patch.
> > > > > > >
> > > > > > > > Please also update cpuinfo.c from libgcc and corresponding
> > > > > > >
> > > > > > > I will take a look to see if we share the same CPU detection code between
> > > > > > > libgcc and config/i386/driver-i386.c.
> > > > > >
> > > > > > I don't think it will bring any benefit, this is mainly one huge
> > > > > > switch statement that maps to different stuff in libgcc and
> > > > > > driver-i386.
> > > > >
> > > > > libgcc and config/i386/driver-i386.c differ even before my patch.
> > > > > I think we can do better.
> > > > >
> > > >
> > > > Move cpuinfo.h from libgcc to common/config/i386 so that get_intel_cpu
> > > > can be shared by libgcc, GCC driver, gcc.target/i386/builtin_target.c
> > > > and libgfortran to detect the specific type of Intel CPU.  Update
> > > > libgfortran to use has_cpu_feature to detect x86 CPU features.
> > > >
> > > > Tested on Linux/x86 and Linux/x86-64.  OK for master?
> > >
> > > Handling only Intel targets and not others is a sure way for patch to
> > > be ignored.
> > >
> >
> > Here is the updated patch to cover AMD CPU.  It also fixes:
> >
> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95212
> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95220
> >
> > OK for master?
>
> Huh... I didn't think the solution will be this messy... I have to
> rethink the approach a bit.
>

These can avoid duplication in i386-builtins.c:

/* These are the values for vendor types, cpu types and subtypes in
   cpuinfo.h.  Cpu types and subtypes should be subtracted by the
   corresponding start value.  */

#define M_CPU_TYPE_START (BUILTIN_VENDOR_MAX)
#define M_CPU_SUBTYPE_START \
  (M_CPU_TYPE_START + BUILTIN_CPU_TYPE_MAX)
#define M_VENDOR(a) (a)
#define M_CPU_TYPE(a) (M_CPU_TYPE_START + a)
#define M_CPU_SUBTYPE(a) (M_CPU_SUBTYPE_START + a)

static const _arch_names_table arch_names_table[] =
{
  {"amd", M_VENDOR (VENDOR_AMD)},
  {"intel", M_VENDOR (VENDOR_INTEL)},
  {"atom", M_CPU_TYPE (INTEL_BONNELL)},
  {"slm", M_CPU_TYPE (INTEL_SILVERMONT)},
  {"core2", M_CPU_TYPE (INTEL_CORE2)},
  {"corei7", M_CPU_TYPE (INTEL_COREI7)},
  {"nehalem", M_CPU_SUBTYPE (INTEL_COREI7_NEHALEM)},
  {"westmere", M_CPU_SUBTYPE (INTEL_COREI7_WESTMERE)},
  {"sandybridge", M_CPU_SUBTYPE (INTEL_COREI7_SANDYBRIDGE)},
  {"ivybridge", M_CPU_SUBTYPE (INTEL_COREI7_IVYBRIDGE)},
  {"haswell", M_CPU_SUBTYPE (INTEL_COREI7_HASWELL)},
  {"broadwell", M_CPU_SUBTYPE (INTEL_COREI7_BROADWELL)},
  {"skylake", M_CPU_SUBTYPE (INTEL_COREI7_SKYLAKE)},
  {"skylake-avx512", M_CPU_SUBTYPE (INTEL_COREI7_SKYLAKE_AVX512)},
  {"cannonlake", M_CPU_SUBTYPE (INTEL_COREI7_CANNONLAKE)},
  {"icelake-client", M_CPU_SUBTYPE (INTEL_COREI7_ICELAKE_CLIENT)},
  {"icelake-server", M_CPU_SUBTYPE (INTEL_COREI7_ICELAKE_SERVER)},
  {"cascadelake", M_CPU_SUBTYPE (INTEL_COREI7_CASCADELAKE)},
  {"tigerlake", M_CPU_SUBTYPE (INTEL_COREI7_TIGERLAKE)},
  {"cooperlake", M_CPU_SUBTYPE (INTEL_COREI7_COOPERLAKE)},
  {"bonnell", M_CPU_TYPE (INTEL_BONNELL)},
  {"silvermont", M_CPU_TYPE (INTEL_SILVERMONT)},
  {"goldmont", M_CPU_TYPE (INTEL_GOLDMONT)},
  {"goldmont-plus", M_CPU_TYPE (INTEL_GOLDMONT_PLUS)},
  {"tremont", M_CPU_TYPE (INTEL_TREMONT)},
  {"knl", M_CPU_TYPE (INTEL_KNL)},
  {"knm", M_CPU_TYPE (INTEL_KNM)},
  {"amdfam10h", M_CPU_TYPE (AMDFAM10H)},
  {"barcelona", M_CPU_SUBTYPE (AMDFAM10H_BARCELONA)},
  {"shanghai", M_CPU_SUBTYPE (AMDFAM10H_SHANGHAI)},
  {"istanbul", M_CPU_SUBTYPE (AMDFAM10H_ISTANBUL)},
  {"btver1", M_CPU_TYPE (AMD_BTVER1)},
  {"amdfam15h", M_CPU_TYPE (AMDFAM15H)},
  {"bdver1", M_CPU_SUBTYPE (AMDFAM15H_BDVER1)},
  {"bdver2", M_CPU_SUBTYPE (AMDFAM15H_BDVER2)},
  {"bdver3", M_CPU_SUBTYPE (AMDFAM15H_BDVER3)},
  {"bdver4", M_CPU_SUBTYPE (AMDFAM15H_BDVER4)},
  {"btver2", M_CPU_TYPE (AMD_BTVER2)},
  {"amdfam17h", M_CPU_TYPE (AMDFAM17H)},
  {"znver1", M_CPU_SUBTYPE (AMDFAM17H_ZNVER1)},
  {"znver2", M_CPU_SUBTYPE (AMDFAM17H_ZNVER2)},
};


--
H.J.
Reply | Threaded
Open this post in threaded view
|

Re: V2 [PATCH] x86: Move cpuinfo.h from libgcc to common/config/i386

gcc - fortran mailing list
In reply to this post by gcc - fortran mailing list
On Tue, May 19, 2020 at 11:40 PM H.J. Lu <[hidden email]> wrote:

> > > > > > > > I will take a look to see if we share the same CPU detection code between
> > > > > > > > libgcc and config/i386/driver-i386.c.
> > > > > > >
> > > > > > > I don't think it will bring any benefit, this is mainly one huge
> > > > > > > switch statement that maps to different stuff in libgcc and
> > > > > > > driver-i386.
> > > > > >
> > > > > > libgcc and config/i386/driver-i386.c differ even before my patch.
> > > > > > I think we can do better.
> > > > > >
> > > > >
> > > > > Move cpuinfo.h from libgcc to common/config/i386 so that get_intel_cpu
> > > > > can be shared by libgcc, GCC driver, gcc.target/i386/builtin_target.c
> > > > > and libgfortran to detect the specific type of Intel CPU.  Update
> > > > > libgfortran to use has_cpu_feature to detect x86 CPU features.
> > > > >
> > > > > Tested on Linux/x86 and Linux/x86-64.  OK for master?
> > > >
> > > > Handling only Intel targets and not others is a sure way for patch to
> > > > be ignored.
> > > >
> > >
> > > Here is the updated patch to cover AMD CPU.  It also fixes:
> > >
> > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95212
> > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95220
> > >
> > > OK for master?
> >
> > Huh... I didn't think the solution will be this messy... I have to
> > rethink the approach a bit.
>
> That is what will happen when we have the same info in more than one place
> There should only one place for CPU info.

Looking at the patch, it is clear that cpuinfo.c and driver-i386.c
should stay apart. They are two different things, and they are
orthogonal to each other; one can be updated without affecting the
other one. driver-i386.c handles way more targets than cpuinfo.c and
your patch only handles a subset of them. The unification does not
bring any benefit, it even complicates things more.

I think that unifying libgcc and i386-builtins.c is the way to go.
libgfortran should benefit from this unification, too.

Uros.
Reply | Threaded
Open this post in threaded view
|

Re: V2 [PATCH] x86: Move cpuinfo.h from libgcc to common/config/i386

gcc - fortran mailing list
On Tue, May 19, 2020 at 11:10 PM Uros Bizjak <[hidden email]> wrote:

>
> On Tue, May 19, 2020 at 11:40 PM H.J. Lu <[hidden email]> wrote:
>
> > > > > > > > > I will take a look to see if we share the same CPU detection code between
> > > > > > > > > libgcc and config/i386/driver-i386.c.
> > > > > > > >
> > > > > > > > I don't think it will bring any benefit, this is mainly one huge
> > > > > > > > switch statement that maps to different stuff in libgcc and
> > > > > > > > driver-i386.
> > > > > > >
> > > > > > > libgcc and config/i386/driver-i386.c differ even before my patch.
> > > > > > > I think we can do better.
> > > > > > >
> > > > > >
> > > > > > Move cpuinfo.h from libgcc to common/config/i386 so that get_intel_cpu
> > > > > > can be shared by libgcc, GCC driver, gcc.target/i386/builtin_target.c
> > > > > > and libgfortran to detect the specific type of Intel CPU.  Update
> > > > > > libgfortran to use has_cpu_feature to detect x86 CPU features.
> > > > > >
> > > > > > Tested on Linux/x86 and Linux/x86-64.  OK for master?
> > > > >
> > > > > Handling only Intel targets and not others is a sure way for patch to
> > > > > be ignored.
> > > > >
> > > >
> > > > Here is the updated patch to cover AMD CPU.  It also fixes:
> > > >
> > > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95212
> > > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95220
> > > >
> > > > OK for master?
> > >
> > > Huh... I didn't think the solution will be this messy... I have to
> > > rethink the approach a bit.
> >
> > That is what will happen when we have the same info in more than one place
> > There should only one place for CPU info.
>
> Looking at the patch, it is clear that cpuinfo.c and driver-i386.c
> should stay apart. They are two different things, and they are
> orthogonal to each other; one can be updated without affecting the
> other one. driver-i386.c handles way more targets than cpuinfo.c and
> your patch only handles a subset of them. The unification does not
> bring any benefit, it even complicates things more.

There should one place to check a CPU feature, not 2.  It can
be done with a proper cpuinfo.h.

> I think that unifying libgcc and i386-builtins.c is the way to go.
> libgfortran should benefit from this unification, too.
>

libgfortran shouldn't include cpuinfo.h.  It should use __builtin_cpu_is
and  __builtin_cpu_supports.

--
H.J.