aarch64: Fix +nocrypto handling
Checks
Commit Message
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.
Comments
Andrew Carlotti <andrew.carlotti@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. */
@@ -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;
+ }
+
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;
@@ -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");
@@ -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)
new file mode 100644
@@ -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. */
new file mode 100644
@@ -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. */