Andrew Carlotti <andrew.carlo...@arm.com> writes:
> Additionally, replace all checks for the AARCH64_FL_CRYPTO bit with
> checks for (AARCH64_FL_AES | AARCH64_FL_SHA2) instead. The value of the
> AARCH64_FL_CRYPTO bit within isa_flags is now ignored, but it is
> retained because removing it would make processing the data in
> option-extensions.def significantly more complex.
>
> Ok for master?
>
> gcc/ChangeLog:
>
> * common/config/aarch64/aarch64-common.cc
> (aarch64_get_extension_string_for_isa_flags): Fix generation of
> the "+nocrypto" extension.
> * config/aarch64/aarch64.h (AARCH64_ISA_CRYPTO): Remove.
> (TARGET_CRYPTO): Remove.
> * config/aarch64/aarch64-c.cc (aarch64_update_cpp_builtins):
> Don't use TARGET_CRYPTO.
>
> gcc/testsuite/ChangeLog:
>
> * gcc.target/aarch64/options_set_27.c: New test.
> * gcc.target/aarch64/options_set_28.c: New test.
>
> diff --git a/gcc/common/config/aarch64/aarch64-common.cc
> b/gcc/common/config/aarch64/aarch64-common.cc
> index
> 20bc4e1291bba9b73798398fea659f1154afa205..6d12454143cd64ebaafa7f5e6c23869ee0bfa543
> 100644
> --- a/gcc/common/config/aarch64/aarch64-common.cc
> +++ b/gcc/common/config/aarch64/aarch64-common.cc
> @@ -310,6 +310,7 @@ aarch64_get_extension_string_for_isa_flags
> But in order to make the output more readable, it seems better
> to add the strings in definition order. */
> aarch64_feature_flags added = 0;
> + auto flags_crypto = AARCH64_FL_AES | AARCH64_FL_SHA2;
> for (unsigned int i = ARRAY_SIZE (all_extensions); i-- > 0; )
> {
> auto &opt = all_extensions[i];
> @@ -319,7 +320,7 @@ aarch64_get_extension_string_for_isa_flags
> per-feature crypto flags. */
> auto flags = opt.flag_canonical;
> if (flags == AARCH64_FL_CRYPTO)
> - flags = AARCH64_FL_AES | AARCH64_FL_SHA2;
> + flags = flags_crypto;
>
> if ((flags & isa_flags & (explicit_flags | ~current_flags)) == flags)
> {
> @@ -337,9 +338,27 @@ aarch64_get_extension_string_for_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. */
> + or not.
> +
> + As a special case, emit "+nocrypto" instead of "+noaes+nosha2", in order
> + to support assemblers that predate the separate per-feature crypto
> flags.
> + Only use "+nocrypto" when "simd" is enabled (to avoid redundant feature
> + removal), and when "sm4" is not already enabled (to avoid dependending
> on
> + whether "+nocrypto" also disables "sm4") */
> + for (auto &opt : all_extensions)
> + if ((opt.flag_canonical == AARCH64_FL_CRYPTO)
> + && ((flags_crypto & current_flags & ~isa_flags) == flags_crypto)
> + && (current_flags & AARCH64_FL_SIMD)
> + && !(current_flags & AARCH64_FL_SM4))
> + {
> + current_flags &= ~opt.flags_off;
> + outstr += "+no";
> + outstr += opt.name;
> + }
> +
Is it an important part of the patch that we do this ahead of time,
rather than in the main loop? Doing it in the main loop feels more
natural, and should avoid the need for the SIMD test.
It we do use an in-loop test, I assume the test would need to be
something like:
(opt.flag_canonical & flag_crypto)
&& (flags_crypto & current_flags & ~isa_flags) == flags_crypto
&& !(current_flags & AARCH64_FL_SM4)
so that the new code is applied when the loop first sees a crypto flag.
The set of flags to disable would be:
current_flags &= ~feature_deps::get_flags_off (flag_crypto);
Otherwise it looks good, thanks. As a general formatting note,
GCC style is not to put individual comparisons in parentheses in
&& and || combos.
Richard
> for (auto &opt : all_extensions)
> if (opt.native_detect_p
> + && (opt.flag_canonical != AARCH64_FL_CRYPTO)
> && (opt.flag_canonical & current_flags & ~isa_flags))
> {
> current_flags &= ~opt.flags_off;
> diff --git a/gcc/config/aarch64/aarch64-c.cc b/gcc/config/aarch64/aarch64-c.cc
> index
> ab8844f6049dc95b97648b651bfcd3a4ccd3ca0b..4f9ee01d52f3ac42f95edbb030bdb2d09fc36d16
> 100644
> --- a/gcc/config/aarch64/aarch64-c.cc
> +++ b/gcc/config/aarch64/aarch64-c.cc
> @@ -140,7 +140,7 @@ aarch64_update_cpp_builtins (cpp_reader *pfile)
> aarch64_def_or_undef (TARGET_ILP32, "_ILP32", pfile);
> aarch64_def_or_undef (TARGET_ILP32, "__ILP32__", pfile);
>
> - aarch64_def_or_undef (TARGET_CRYPTO, "__ARM_FEATURE_CRYPTO", pfile);
> + aarch64_def_or_undef (TARGET_AES && TARGET_SHA2, "__ARM_FEATURE_CRYPTO",
> pfile);
> aarch64_def_or_undef (TARGET_SIMD_RDMA, "__ARM_FEATURE_QRDMX", pfile);
> aarch64_def_or_undef (TARGET_SVE, "__ARM_FEATURE_SVE", pfile);
> cpp_undef (pfile, "__ARM_FEATURE_SVE_BITS");
> diff --git a/gcc/config/aarch64/aarch64.h b/gcc/config/aarch64/aarch64.h
> index
> 1ac298926ce1606a87bcdcaf691f182ca416d600..d3613a0a42b7b6d2c4452739841b133014909a39
> 100644
> --- a/gcc/config/aarch64/aarch64.h
> +++ b/gcc/config/aarch64/aarch64.h
> @@ -177,10 +177,13 @@ enum class aarch64_feature : unsigned char {
>
> #endif
>
> -/* Macros to test ISA flags. */
> +/* Macros to test ISA flags.
> +
> + There is intentionally no macro for AARCH64_FL_CRYPTO, since this flag bit
> + is not always set when its constituent features are present.
> + Check (TARGET_AES && TARGET_SHA2) instead. */
>
> #define AARCH64_ISA_CRC (aarch64_isa_flags & AARCH64_FL_CRC)
> -#define AARCH64_ISA_CRYPTO (aarch64_isa_flags & AARCH64_FL_CRYPTO)
> #define AARCH64_ISA_FP (aarch64_isa_flags & AARCH64_FL_FP)
> #define AARCH64_ISA_SIMD (aarch64_isa_flags & AARCH64_FL_SIMD)
> #define AARCH64_ISA_LSE (aarch64_isa_flags & AARCH64_FL_LSE)
> @@ -223,9 +226,6 @@ enum class aarch64_feature : unsigned char {
> #define AARCH64_ISA_LS64 (aarch64_isa_flags & AARCH64_FL_LS64)
> #define AARCH64_ISA_CSSC (aarch64_isa_flags & AARCH64_FL_CSSC)
>
> -/* Crypto is an optional extension to AdvSIMD. */
> -#define TARGET_CRYPTO (AARCH64_ISA_CRYPTO)
> -
> /* SHA2 is an optional extension to AdvSIMD. */
> #define TARGET_SHA2 (AARCH64_ISA_SHA2)
>
> diff --git a/gcc/testsuite/gcc.target/aarch64/options_set_27.c
> b/gcc/testsuite/gcc.target/aarch64/options_set_27.c
> new file mode 100644
> index
> 0000000000000000000000000000000000000000..08f2b5962ad5f4204eca4d2020ace74dbfd7c7ea
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/aarch64/options_set_27.c
> @@ -0,0 +1,11 @@
> +/* { dg-do compile } */
> +/* { dg-additional-options "-march=armv8.2-a+aes+sha2" } */
> +
> +int main ()
> +{
> + return 0;
> +}
> +
> +/* { dg-final { scan-assembler-times {\.arch armv8\.2\-a\+crc\+crypto\n} 1 }
> } */
> +
> +/* Checking if enabling default features drops the superfluous bits. */
> diff --git a/gcc/testsuite/gcc.target/aarch64/options_set_28.c
> b/gcc/testsuite/gcc.target/aarch64/options_set_28.c
> new file mode 100644
> index
> 0000000000000000000000000000000000000000..ec7619c6c937f44bc5a3ddc29c93ecfa5dafa2f5
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/aarch64/options_set_28.c
> @@ -0,0 +1,11 @@
> +/* { dg-do compile } */
> +/* { dg-additional-options "-march=armv8.2-a+aes+sha3" } */
> +
> +int main ()
> +{
> + return 0;
> +}
> +
> +/* { dg-final { scan-assembler-times {\.arch armv8\.2\-a\+crc\+aes\+sha3\n}
> 1 } } */
> +
> +/* Checking if enabling default features drops the superfluous bits. */
