Re: V2 [PATCH] x86: Share _isa_names_table and use cpuinfo.h
On Wed, Jun 24, 2020 at 4:38 PM H.J. Lu wrote:
>
> On Wed, Jun 24, 2020 at 03:46:25PM +0200, Uros Bizjak wrote:
> > On Wed, Jun 24, 2020 at 3:37 PM Uros Bizjak wrote:
> > >
> > > On Wed, Jun 24, 2020 at 3:06 PM H.J. Lu wrote:
> > > >
> > > > On Wed, Jun 24, 2020 at 02:43:43PM +0200, Uros Bizjak wrote:
> > > > > >
> > > > > > Here is the updated patch for x86 backend and libgcc. driver-i386.c
> > > > > > is unchanged.
> > > > >
> > > > > Thanks. We should change driver-i386.c very carefully and in an
> > > > > independent way from this patch. It is a complex and interwoven web of
> > > > > name, model and features check. I propose that we first convert
> > > > > various has_xxx checks to a new interface, in as trivial way as
> > > > > possible.
> > > > >
> > > >
> > > > Here is the patch to share isa_names_table and use cpuinfo.h to check
> > > > ISAs as well as detect newer Intel/AMD processors.
> > > >
> > > > OK for master?
> > >
> > > No. Don't use get_intel_cpu and get_amd_cpu in driver-i386.c.
> >
> > + cpu = get_amd_cpu (_model, _model2, cpu_features2);
> > + if (cpu == NULL)
> > + {
> > + if (name == signature_NSC_ebx)
> > + processor = PROCESSOR_GEODE;
> > + else if (has_feature (FEATURE_SSE2)
> >
> > Here is where your patch fails. AMD part has early bypass for
> > signature_NSC_ebx, so it is detected as NSC processor regardless of
> > what is detected by generic code.
> >
> > Uros.
>
> Here is the updated patch not to use get_amd_cpu in driver-i386.c.
> I believe the usage of get_intel_cpu in driver-i386.c is correct.
>
> OK for master?
OK, let's go with this version, but look out for fallout.
I hope there is some interest from AMD folks to unify and further
improve their part of the detection code.
Thanks,
Uros.
> Thanks.
>
> H.J.
> ---
> Both driver-i386.c and libgcc use CPUID to detect the processor name
> as well as available ISAs. To detect the same processor or ISAs, the
> same detection logic is duplicated in 2 places. Sometimes only one place
> was up to date or got it right. Sometimes both places got it wrong.
>
> 1. Add common/config/i386/i386-isas.h to define _isa_names_table.
> 2. Use isa_names_table to auto-generate ISA command-line options.
> 3. Use isa_names_table to auto-generate __builtin_cpu_supports tests.
> 4. Use common/config/i386/cpuinfo.h to check available ISAs and detect
> newer Intel processors in driver-i386.c and builtin_target.c.
> 5. Detection of AMD processors and older processors in driver-i386.c is
> unchanged.
>
> gcc/
>
> PR target/95843
> * common/config/i386/i386-isas.h: New file. Extracted from
> gcc/config/i386/i386-builtins.c.
> (_isa_names_table): Add option.
> (ISA_NAMES_TABLE_START): New.
> (ISA_NAMES_TABLE_END): Likewise.
> (ISA_NAMES_TABLE_ENTRY): Likewise.
> (isa_names_table): Defined with ISA_NAMES_TABLE_START,
> ISA_NAMES_TABLE_END and ISA_NAMES_TABLE_ENTRY. Add more ISAs
> from enum processor_features.
> * config/i386/driver-i386.c: Include
> "common/config/i386/cpuinfo.h" and
> "common/config/i386/i386-isas.h".
> (has_feature): New macro.
> (host_detect_local_cpu): Call cpu_indicator_init to get CPU
> features. Use has_feature to detect processor features. Call
> Call get_intel_cpu to get the newer Intel CPU name. Use
> isa_names_table to generate command-line options.
> * config/i386/i386-builtins.c: Include
> "common/config/i386/i386-isas.h".
> (_arch_names_table): Removed.
> (isa_names_table): Likewise.
>
> gcc/testsuite/
>
> PR target/95843
> * gcc.target/i386/builtin_target.c: Include ,
> ../../../common/config/i386/i386-cpuinfo.h and
> ../../../common/config/i386/cpuinfo.h.
> (check_amd_cpu_model): Removed.
> (check_intel_cpu_model): Likewise,
> (CHECK___builtin_cpu_is): New.
> (gcc_assert): New. Defined as assert.
> (gcc_unreachable): New. Defined as abort.
> (inline): New. Defined as empty.
> (ISA_NAMES_TABLE_START): Likewise.
> (ISA_NAMES_TABLE_END): Likewise.
> (ISA_NAMES_TABLE_ENTRY): New.
> (check_features): Include
> "../../../common/config/i386/i386-isas.h".
> (check_detailed): Call cpu_indicator_init. Always call
> check_features. Call get_amd_cpu instead of check_amd_cpu_model.
> Call get_intel_cpu instead of check_intel_cpu_model.
> ---
> gcc/common/config/i386/i386-isas.h| 163 +
> gcc/config/i386/driver-i386.c | 644 +++---
> gcc/config/i386/i386-builtins.c | 52 +-
> .../gcc.target/i386/builtin_target.c | 355 +-
> 4 files changed, 306 insertions(+), 908 deletions(-)
> create mode 100644 gcc/common/config/i386/i386-isas.h
>
> diff --git a/gcc/common/config/i386/i386-isas.h
V2 [PATCH] x86: Share _isa_names_table and use cpuinfo.h
On Wed, Jun 24, 2020 at 03:46:25PM +0200, Uros Bizjak wrote:
> On Wed, Jun 24, 2020 at 3:37 PM Uros Bizjak wrote:
> >
> > On Wed, Jun 24, 2020 at 3:06 PM H.J. Lu wrote:
> > >
> > > On Wed, Jun 24, 2020 at 02:43:43PM +0200, Uros Bizjak wrote:
> > > > >
> > > > > Here is the updated patch for x86 backend and libgcc. driver-i386.c
> > > > > is unchanged.
> > > >
> > > > Thanks. We should change driver-i386.c very carefully and in an
> > > > independent way from this patch. It is a complex and interwoven web of
> > > > name, model and features check. I propose that we first convert
> > > > various has_xxx checks to a new interface, in as trivial way as
> > > > possible.
> > > >
> > >
> > > Here is the patch to share isa_names_table and use cpuinfo.h to check
> > > ISAs as well as detect newer Intel/AMD processors.
> > >
> > > OK for master?
> >
> > No. Don't use get_intel_cpu and get_amd_cpu in driver-i386.c.
>
> + cpu = get_amd_cpu (_model, _model2, cpu_features2);
> + if (cpu == NULL)
> + {
> + if (name == signature_NSC_ebx)
> + processor = PROCESSOR_GEODE;
> + else if (has_feature (FEATURE_SSE2)
>
> Here is where your patch fails. AMD part has early bypass for
> signature_NSC_ebx, so it is detected as NSC processor regardless of
> what is detected by generic code.
>
> Uros.
Here is the updated patch not to use get_amd_cpu in driver-i386.c.
I believe the usage of get_intel_cpu in driver-i386.c is correct.
OK for master?
Thanks.
H.J.
---
Both driver-i386.c and libgcc use CPUID to detect the processor name
as well as available ISAs. To detect the same processor or ISAs, the
same detection logic is duplicated in 2 places. Sometimes only one place
was up to date or got it right. Sometimes both places got it wrong.
1. Add common/config/i386/i386-isas.h to define _isa_names_table.
2. Use isa_names_table to auto-generate ISA command-line options.
3. Use isa_names_table to auto-generate __builtin_cpu_supports tests.
4. Use common/config/i386/cpuinfo.h to check available ISAs and detect
newer Intel processors in driver-i386.c and builtin_target.c.
5. Detection of AMD processors and older processors in driver-i386.c is
unchanged.
gcc/
PR target/95843
* common/config/i386/i386-isas.h: New file. Extracted from
gcc/config/i386/i386-builtins.c.
(_isa_names_table): Add option.
(ISA_NAMES_TABLE_START): New.
(ISA_NAMES_TABLE_END): Likewise.
(ISA_NAMES_TABLE_ENTRY): Likewise.
(isa_names_table): Defined with ISA_NAMES_TABLE_START,
ISA_NAMES_TABLE_END and ISA_NAMES_TABLE_ENTRY. Add more ISAs
from enum processor_features.
* config/i386/driver-i386.c: Include
"common/config/i386/cpuinfo.h" and
"common/config/i386/i386-isas.h".
(has_feature): New macro.
(host_detect_local_cpu): Call cpu_indicator_init to get CPU
features. Use has_feature to detect processor features. Call
Call get_intel_cpu to get the newer Intel CPU name. Use
isa_names_table to generate command-line options.
* config/i386/i386-builtins.c: Include
"common/config/i386/i386-isas.h".
(_arch_names_table): Removed.
(isa_names_table): Likewise.
gcc/testsuite/
PR target/95843
* gcc.target/i386/builtin_target.c: Include ,
../../../common/config/i386/i386-cpuinfo.h and
../../../common/config/i386/cpuinfo.h.
(check_amd_cpu_model): Removed.
(check_intel_cpu_model): Likewise,
(CHECK___builtin_cpu_is): New.
(gcc_assert): New. Defined as assert.
(gcc_unreachable): New. Defined as abort.
(inline): New. Defined as empty.
(ISA_NAMES_TABLE_START): Likewise.
(ISA_NAMES_TABLE_END): Likewise.
(ISA_NAMES_TABLE_ENTRY): New.
(check_features): Include
"../../../common/config/i386/i386-isas.h".
(check_detailed): Call cpu_indicator_init. Always call
check_features. Call get_amd_cpu instead of check_amd_cpu_model.
Call get_intel_cpu instead of check_intel_cpu_model.
---
gcc/common/config/i386/i386-isas.h| 163 +
gcc/config/i386/driver-i386.c | 644 +++---
gcc/config/i386/i386-builtins.c | 52 +-
.../gcc.target/i386/builtin_target.c | 355 +-
4 files changed, 306 insertions(+), 908 deletions(-)
create mode 100644 gcc/common/config/i386/i386-isas.h
diff --git a/gcc/common/config/i386/i386-isas.h
b/gcc/common/config/i386/i386-isas.h
new file mode 100644
index 000..08c9dbecc76
--- /dev/null
+++ b/gcc/common/config/i386/i386-isas.h
@@ -0,0 +1,163 @@
+/* i386 ISA table.
+ Copyright (C) 2020 Free Software Foundation, Inc.
+
+This file is part of GCC.
+
+GCC is free software; you can redistribute it and/or modify
+it under the terms of the GNU General Public License as published by
+the Free Software Foundation; either version
