On Fri, Jan 27, 2023 at 4:12 AM Richard Sandiford via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> Tamar Christina <tamar.christ...@arm.com> writes:
> > Hi All,
> >
> > aarch64-option-extensions.def explicitly defines the semantics for an empty
> > midr
> > field as being:
> >
> > In that case this field
> > should contain a space (" ") separated list of the strings in
> > 'Features'
> > that are required. Their order is not important. An empty string
> > means
> > do not detect this feature during auto detection.
> >
> > That is to say, an empty string means that we don't know the midr value for
> > this
> > feature and so it just shouldn't be taken into account for native features
> > detection. However this meaning seems to have gotten lost at some point.
> >
> > This results in e.g. -mcpu=native on a Neoverse N2 disabling features it
> > does
> > have. Essentially we disabled any mandatory feature for which there is no
> > midr
> > entry.
> >
> > The rationale for having -mcpu=native being able to disable features at
> > all, is
> > because the kernel is able to disable a mandatory feature for correctness
> > issues. Unfortunately we can't distinguish between "old kernel"
> > and "kernel disabled".
> >
> > This patch adds a new field that indicates whether the midr field has any
> > value
> > at all. If there's no value we skip the extension when determining the
> > "off"
> > flags.
> >
> > Bootstrapped Regtested on aarch64-none-linux-gnu and no issues.
> >
> > Ok for master? I think it needs backporting but need to verify older
> > compilers.
> > If one is required, OK for backporting?
> >
> > Thanks,
> > Tamar
> >
> > gcc/ChangeLog:
> >
> > * common/config/aarch64/aarch64-common.cc
> > (struct aarch64_option_extension): Add native_detect and document
> > struct
> > a bit more.
> > (all_extensions): Set new field native_detect.
> > * config/aarch64/aarch64.cc (struct aarch64_option_extension): Delete
> > unused struct.
> >
> > gcc/testsuite/ChangeLog:
> >
> > * gcc.target/aarch64/cpunative/info_19: New test.
> > * gcc.target/aarch64/cpunative/info_20: New test.
> > * gcc.target/aarch64/cpunative/info_21: New test.
> > * gcc.target/aarch64/cpunative/info_22: New test.
> > * gcc.target/aarch64/cpunative/native_cpu_19.c: New test.
> > * gcc.target/aarch64/cpunative/native_cpu_20.c: New test.
> > * gcc.target/aarch64/cpunative/native_cpu_21.c: New test.
> > * gcc.target/aarch64/cpunative/native_cpu_22.c: New test.
>
> Mostly LGTM, but some nits below.
>
> > --- inline copy of patch --
> > diff --git a/gcc/common/config/aarch64/aarch64-common.cc
> > b/gcc/common/config/aarch64/aarch64-common.cc
> > index
> > a9695d60197e6585957b293d2d755a557e124d4f..4e9e9c0bf86a5ef2667f0bb7e646ba06152aa982
> > 100644
> > --- a/gcc/common/config/aarch64/aarch64-common.cc
> > +++ b/gcc/common/config/aarch64/aarch64-common.cc
> > @@ -139,10 +139,17 @@ aarch64_handle_option (struct gcc_options *opts,
> > /* An ISA extension in the co-processor and main instruction set space. */
> > struct aarch64_option_extension
> > {
> > - const char *name;
> > + /* The extension name to pass on to the assembler. */
> > + const char *const name;
>
> There's no need to make name itself const.
>
> > + /* The smallest set of feature bits to toggle to enable this option. */
> > aarch64_feature_flags flag_canonical;
> > - aarch64_feature_flags flags_on;
> > - aarch64_feature_flags flags_off;
> > + /* If this feature is turned on, these bits also need to be turned on.
> > */
> > + const unsigned long flags_on;
> > + /* If this feature is turned off, these bits also need to be turned off.
> > */
> > + const unsigned long flags_off;
>
> Please don't undo the aarch64_feature_flags abstraction. "long" isn't
> enough for x86_32 to aarch64 cross-compilers (yes, I know, but still),
> and we're not far off running out of room in the uint64_t. The point
> of the abstraction was to reduce the number of changes that we need
> once we have 65 or more features, architecture levels, etc.
And a cross from x86_64-mingw to aarch64, long would be still 32bit as
mingw (and Windows in general) is a LLP64IL32 target. x86_64-mingw is
a less obscured target too.
Thanks,
Andrew Pinski
>
> > + /* Indicates whether this feature is taken into account during native cpu
> > + detection. */
> > + bool native_detect;
> > };
> >
> > /* ISA extensions in AArch64. */
> > @@ -150,9 +157,9 @@ static constexpr aarch64_option_extension
> > all_extensions[] =
> > {
> > #define AARCH64_OPT_EXTENSION(NAME, IDENT, C, D, E, F) \
> > {NAME, AARCH64_FL_##IDENT, feature_deps::IDENT ().explicit_on, \
> > - feature_deps::get_flags_off (feature_deps::root_off_##IDENT)},
> > + feature_deps::get_flags_off (feature_deps::root_off_##IDENT), strlen
> > (F)},
>
> strlen isn't guaranteed to be evaluated at compile time. How about
> F[0] instead? Would be good to rename F to FEATURE_STRING, now that
> the expansion actually uses it.
>
> > #include "config/aarch64/aarch64-option-extensions.def"
> > - {NULL, 0, 0, 0}
> > + {NULL, 0, 0, 0, false}
> > };
> >
> > struct processor_name_to_arch
> > @@ -325,9 +332,13 @@ aarch64_get_extension_string_for_isa_flags
> > outstr += opt.name;
> > }
> >
> > - /* Remove the features in current_flags & ~isa_flags. */
> > + /* Remove the features in current_flags & ~isa_flags. If the feature
> > does
> > + not have an HWCAPs then it shouldn't be taken into account for feature
> > + detection because one way or another we can't tell if it's available
> > + or not. */
> > for (auto &opt : all_extensions)
> > - if (opt.flag_canonical & current_flags & ~isa_flags)
> > + if (opt.native_detect
> > + && (opt.flag_canonical & current_flags & ~isa_flags))
> > {
> > current_flags &= ~opt.flags_off;
> > outstr += "+no";
> > diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc
> > index
> > d36b57341b336a81dc2e1a975986b3e37402602a..860aeb3e5fbf655e87284be28cc72648c1cd71f9
> > 100644
> > --- a/gcc/config/aarch64/aarch64.cc
> > +++ b/gcc/config/aarch64/aarch64.cc
> > @@ -2808,14 +2808,6 @@ static const struct attribute_spec
> > aarch64_attribute_table[] =
> > { NULL, 0, 0, false, false, false, false, NULL, NULL }
> > };
> >
> > -/* An ISA extension in the co-processor and main instruction set space. */
> > -struct aarch64_option_extension
> > -{
> > - const char *const name;
> > - const unsigned long flags_on;
> > - const unsigned long flags_off;
> > -};
> > -
> > typedef enum aarch64_cond_code
> > {
> > AARCH64_EQ = 0, AARCH64_NE, AARCH64_CS, AARCH64_CC, AARCH64_MI,
> > AARCH64_PL,
> > diff --git a/gcc/testsuite/gcc.target/aarch64/cpunative/info_19
> > b/gcc/testsuite/gcc.target/aarch64/cpunative/info_19
> > new file mode 100644
> > index
> > 0000000000000000000000000000000000000000..616d3b26d03398a8ef3f4cdf4c56b144ecf0b1c2
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.target/aarch64/cpunative/info_19
> > @@ -0,0 +1,9 @@
> > +processor : 0
> > +BogoMIPS : 100.00
> > +Features : fp asimd evtstrm aes pmull sha1 sha2 crc32 atomics fphp
> > asimdhp cpuid asimdrdm jscvt fcma lrcpc dcpop sha3 sm3 sm4 asimddp sha512
> > sve asimdfhm dit uscat ilrcpc flagm ssbs sb dcpodp sve2 sveaes svepmull
> > svebitperm svesha3 svesm4 flagm2 frint svei8mm svebf16 i8mm bf16 dgh bti
> > +CPU implementer : 0x41
> > +CPU architecture: 8
> > +CPU variant : 0x0
> > +CPU part : 0xd49
> > +CPU revision : 2
> > +
> > diff --git a/gcc/testsuite/gcc.target/aarch64/cpunative/info_20
> > b/gcc/testsuite/gcc.target/aarch64/cpunative/info_20
> > new file mode 100644
> > index
> > 0000000000000000000000000000000000000000..45d45d15f16b927ebdffa80bca76270a7e2ed135
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.target/aarch64/cpunative/info_20
> > @@ -0,0 +1,9 @@
> > +processor : 0
> > +BogoMIPS : 100.00
> > +Features : fp asimd evtstrm aes pmull sha1 sha2 crc32 atomics fphp
> > asimdhp cpuid asimdrdm jscvt fcma lrcpc dcpop sha3 sm3 sm4 asimddp sha512
> > sve asimdfhm dit uscat ilrcpc flagm ssbs sb dcpodp sve2 sveaes svepmull
> > svebitperm svesha3 svesm4 flagm2 frint svei8mm svebf16 i8mm bf16 dgh bti
> > paca pacg
> > +CPU implementer : 0x41
> > +CPU architecture: 8
> > +CPU variant : 0x0
> > +CPU part : 0xd49
> > +CPU revision : 2
> > +
> > diff --git a/gcc/testsuite/gcc.target/aarch64/cpunative/info_21
> > b/gcc/testsuite/gcc.target/aarch64/cpunative/info_21
> > new file mode 100644
> > index
> > 0000000000000000000000000000000000000000..3c418a4bee4e2bf009e658def70bd184c4084d16
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.target/aarch64/cpunative/info_21
> > @@ -0,0 +1,9 @@
> > +processor : 0
> > +BogoMIPS : 100.00
> > +Features : fp asimd evtstrm aes pmull sha1 sha2 crc32 atomics fphp
> > asimdhp cpuid asimdrdm jscvt fcma lrcpc dcpop sha3 sm3 sm4 asimddp sha512
> > sve asimdfhm dit uscat ilrcpc flagm ssbs sb dcpodp sve2 sveaes svepmull
> > svebitperm svesha3 svesm4 flagm2 frint svei8mm svebf16 i8mm bf16 dgh bti
> > +CPU implementer : 0x41
> > +CPU architecture: 8
> > +CPU variant : 0x0
> > +CPU part : 0xd08
> > +CPU revision : 2
> > +
> > diff --git a/gcc/testsuite/gcc.target/aarch64/cpunative/info_22
> > b/gcc/testsuite/gcc.target/aarch64/cpunative/info_22
> > new file mode 100644
> > index
> > 0000000000000000000000000000000000000000..3147eec1f443b3a7da77a89148c23511846ecd85
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.target/aarch64/cpunative/info_22
> > @@ -0,0 +1,9 @@
> > +processor : 0
> > +BogoMIPS : 100.00
> > +Features : fp asimd evtstrm aes pmull sha1 sha2 crc32 atomics fphp
> > asimdhp cpuid asimdrdm jscvt fcma lrcpc dcpop sha3 sm3 sm4 asimddp sha512
> > sve asimdfhm dit uscat ilrcpc flagm ssbs sb dcpodp sve2 sveaes svepmull
> > svebitperm svesha3 svesm4 flagm2 frint svei8mm svebf16 i8mm bf16 dgh bti
> > paca pacg
> > +CPU implementer : 0x41
> > +CPU architecture: 8
> > +CPU variant : 0x0
> > +CPU part : 0xd08
> > +CPU revision : 2
> > +
> > diff --git a/gcc/testsuite/gcc.target/aarch64/cpunative/native_cpu_19.c
> > b/gcc/testsuite/gcc.target/aarch64/cpunative/native_cpu_19.c
> > new file mode 100644
> > index
> > 0000000000000000000000000000000000000000..980d3f79dfb03b0d8eb68f691bf2dedf80aed87d
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.target/aarch64/cpunative/native_cpu_19.c
> > @@ -0,0 +1,14 @@
> > +/* { dg-do compile { target { { aarch64*-*-linux*} && native } } } */
> > +/* { dg-set-compiler-env-var GCC_CPUINFO
> > "$srcdir/gcc.target/aarch64/cpunative/info_19" } */
> > +/* { dg-additional-options "-mcpu=native" } */
> > +
> > +int main()
> > +{
> > + return 0;
> > +}
> > +
> > +/* { dg-final { scan-assembler {\.arch
> > armv9-a\+crc\+profile\+memtag\+sve2-sm4\+sve2-aes\+sve2-sha3\+sve2-bitperm\+i8mm\+bf16\+nopauth\n}
> > } } */
> > +
> > +/* Test one that if the kernel doesn't report the availability of a
> > mandatory
> > + feature that it has turned it off for whatever reason. As such
> > compilers
> > + should follow along. */
> > diff --git a/gcc/testsuite/gcc.target/aarch64/cpunative/native_cpu_20.c
> > b/gcc/testsuite/gcc.target/aarch64/cpunative/native_cpu_20.c
> > new file mode 100644
> > index
> > 0000000000000000000000000000000000000000..117df2b0b6cd5751d9f5175b4343aad9825a6c43
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.target/aarch64/cpunative/native_cpu_20.c
> > @@ -0,0 +1,15 @@
> > +/* { dg-do compile { target { { aarch64*-*-linux*} && native } } } */
> > +/* { dg-set-compiler-env-var GCC_CPUINFO
> > "$srcdir/gcc.target/aarch64/cpunative/info_20" } */
> > +/* { dg-additional-options "-mcpu=native" } */
> > +
> > +int main()
> > +{
> > + return 0;
> > +}
> > +
> > +/* { dg-final { scan-assembler {\.arch
> > armv9-a\+crc\+profile\+memtag\+sve2-sm4\+sve2-aes\+sve2-sha3\+sve2-bitperm\+i8mm\+bf16\n}
> > } } */
> > +
> > +/* Check whether features that don't have a midr name during detection are
> > + correctly ignored. These features shouldn't affect the native
> > detection.
> > + This particular test checks that predres is not turned off during
> > + detection. */
> > diff --git a/gcc/testsuite/gcc.target/aarch64/cpunative/native_cpu_21.c
> > b/gcc/testsuite/gcc.target/aarch64/cpunative/native_cpu_21.c
> > new file mode 100644
> > index
> > 0000000000000000000000000000000000000000..7c7b163640ec0791f3a686597b265776b8edfb5d
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.target/aarch64/cpunative/native_cpu_21.c
> > @@ -0,0 +1,13 @@
> > +/* { dg-do compile { target { { aarch64*-*-linux*} && native } } } */
> > +/* { dg-set-compiler-env-var GCC_CPUINFO
> > "$srcdir/gcc.target/aarch64/cpunative/info_21" } */
> > +/* { dg-additional-options "-mcpu=native" } */
> > +
> > +int main()
> > +{
> > + return 0;
> > +}
> > +
> > +/* { dg-final { scan-assembler {\.arch
> > armv8-a\+crc\+lse\+rcpc\+rdma\+dotprod\+fp16fml\+sb\+ssbs\+sve2-sm4\+sve2-aes\+sve2-sha3\+sve2-bitperm\+i8mm\+bf16\+flagm\n}
> > } } */
> > +
> > +/* Check that an Armv8-A core which doesn't doesn't fall apart on
> > extensions
> > + without midr values. */
>
> Typo, maybe s/which doesn't doesn't/doesn't/.
>
> > diff --git a/gcc/testsuite/gcc.target/aarch64/cpunative/native_cpu_22.c
> > b/gcc/testsuite/gcc.target/aarch64/cpunative/native_cpu_22.c
> > new file mode 100644
> > index
> > 0000000000000000000000000000000000000000..3457623f308107118bdc7f9652b566219f167f46
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.target/aarch64/cpunative/native_cpu_22.c
> > @@ -0,0 +1,13 @@
> > +/* { dg-do compile { target { { aarch64*-*-linux*} && native } } } */
> > +/* { dg-set-compiler-env-var GCC_CPUINFO
> > "$srcdir/gcc.target/aarch64/cpunative/info_22" } */
> > +/* { dg-additional-options "-mcpu=native" } */
> > +
> > +int main()
> > +{
> > + return 0;
> > +}
> > +
> > +/* { dg-final { scan-assembler {\.arch
> > armv8-a\+crc\+lse\+rcpc\+rdma\+dotprod\+fp16fml\+sb\+ssbs\+sve2-sm4\+sve2-aes\+sve2-sha3\+sve2-bitperm\+i8mm\+bf16\+flagm\+pauth\n}
> > } } */
> > +
> > +/* Check that an Armv8-A core which doesn't doesn't fall apart on
> > extensions
> > + without midr values and that it enables optional features. */
>
> Same here.
>
> Thanks,
> Richard
