* aarch64: Fix +nocrypto handling
@ 2023-12-05 19:33 Andrew Carlotti
2023-12-09 19:09 ` Richard Sandiford
0 siblings, 1 reply; 4+ messages in thread
From: Andrew Carlotti @ 2023-12-05 19:33 UTC (permalink / raw)
To: gcc-patches; +Cc: Richard Sandiford, Richard Earnshaw
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;
+ }
+
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. */
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: aarch64: Fix +nocrypto handling
2023-12-05 19:33 aarch64: Fix +nocrypto handling Andrew Carlotti
@ 2023-12-09 19:09 ` Richard Sandiford
2023-12-13 14:52 ` [PATCH v2] " Andrew Carlotti
0 siblings, 1 reply; 4+ messages in thread
From: Richard Sandiford @ 2023-12-09 19:09 UTC (permalink / raw)
To: Andrew Carlotti; +Cc: gcc-patches, Richard Earnshaw
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. */
^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH v2] aarch64: Fix +nocrypto handling
2023-12-09 19:09 ` Richard Sandiford
@ 2023-12-13 14:52 ` Andrew Carlotti
2023-12-13 17:34 ` Richard Sandiford
0 siblings, 1 reply; 4+ messages in thread
From: Andrew Carlotti @ 2023-12-13 14:52 UTC (permalink / raw)
To: gcc-patches; +Cc: Richard Earnshaw, richard.sandiford
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.
This bug should have been picked up by an existing test, but a missing
newline meant that the pattern incorrectly allowed "+crypto+nocrypto".
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_4.c: Add terminating newline.
* gcc.target/aarch64/options_set_27.c: New test.
diff --git a/gcc/common/config/aarch64/aarch64-common.cc b/gcc/common/config/aarch64/aarch64-common.cc
index 8fb901029ec2980a048177586b84201b3b398f9e..c2a6d357c0bc17996a25ea5c3a40f69d745c7931 100644
--- a/gcc/common/config/aarch64/aarch64-common.cc
+++ b/gcc/common/config/aarch64/aarch64-common.cc
@@ -311,6 +311,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];
@@ -320,7 +321,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)
{
@@ -339,14 +340,32 @@ aarch64_get_extension_string_for_isa_flags
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.native_detect_p
- && (opt.flag_canonical & current_flags & ~isa_flags))
- {
- current_flags &= ~opt.flags_off;
- outstr += "+no";
- outstr += opt.name;
- }
+ {
+ auto flags = opt.flag_canonical;
+ /* As a special case, don't emit "+noaes" or "+nosha2" when we could emit
+ "+nocrypto" instead, in order to support assemblers that predate the
+ separate per-feature crypto flags. Only allow "+nocrypto" when "sm4"
+ is not already enabled (to avoid dependending on whether "+nocrypto"
+ also disables "sm4"). */
+ if (flags & flags_crypto
+ && (flags_crypto & current_flags & ~isa_flags) == flags_crypto
+ && !(current_flags & AARCH64_FL_SM4))
+ continue;
+
+ if (flags == AARCH64_FL_CRYPTO)
+ /* If either crypto flag needs removing here, then both do. */
+ flags = flags_crypto;
+
+ if (opt.native_detect_p
+ && (flags & current_flags & ~isa_flags))
+ {
+ current_flags &= ~opt.flags_off;
+ outstr += "+no";
+ outstr += opt.name;
+ }
+ }
return outstr;
}
diff --git a/gcc/config/aarch64/aarch64-c.cc b/gcc/config/aarch64/aarch64-c.cc
index 115a2a8b7568c43a712d819e03147ff84ff182c0..cdc4e453a2054b1a1d2c70bf0b528e497ae0b9ad 100644
--- a/gcc/config/aarch64/aarch64-c.cc
+++ b/gcc/config/aarch64/aarch64-c.cc
@@ -188,7 +188,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 2cd0bc552ebadac06a2838ae2767852c036d0db4..501bb7478a0755fa76c488ec03dcfab6c272851c 100644
--- a/gcc/config/aarch64/aarch64.h
+++ b/gcc/config/aarch64/aarch64.h
@@ -204,7 +204,11 @@ constexpr auto AARCH64_FL_DEFAULT_ISA_MODE = AARCH64_FL_SM_OFF;
#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_SM_OFF (aarch64_isa_flags & AARCH64_FL_SM_OFF)
#define AARCH64_ISA_SM_ON (aarch64_isa_flags & AARCH64_FL_SM_ON)
@@ -213,7 +217,6 @@ constexpr auto AARCH64_FL_DEFAULT_ISA_MODE = AARCH64_FL_SM_OFF;
#define AARCH64_ISA_V8A (aarch64_isa_flags & AARCH64_FL_V8A)
#define AARCH64_ISA_V8_1A (aarch64_isa_flags & AARCH64_FL_V8_1A)
#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)
@@ -294,9 +297,6 @@ constexpr auto AARCH64_FL_DEFAULT_ISA_MODE = AARCH64_FL_SM_OFF;
#define AARCH64_FL_SCXTNUM AARCH64_FL_V8_5A
#define AARCH64_FL_ID_PFR2 AARCH64_FL_V8_5A
-/* 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..e35744640130778cba442c365e70028e53508586
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/options_set_27.c
@@ -0,0 +1,9 @@
+/* { 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 } } */
diff --git a/gcc/testsuite/gcc.target/aarch64/options_set_4.c b/gcc/testsuite/gcc.target/aarch64/options_set_4.c
index 5370e02e1531dd26e5a5fb37de0bab6aed513b71..7b00d09a47f0521f148b1dd576b4100db38aec00 100644
--- a/gcc/testsuite/gcc.target/aarch64/options_set_4.c
+++ b/gcc/testsuite/gcc.target/aarch64/options_set_4.c
@@ -6,7 +6,7 @@ int main ()
return 0;
}
-/* { dg-final { scan-assembler-times {\.arch armv8\.2\-a\+crc\+crypto} 1 } } */
+/* { dg-final { scan-assembler-times {\.arch armv8\.2\-a\+crc\+crypto\n} 1 } } */
/* Check if individual bits that make up a grouping is specified that only the
grouping is kept. */
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] aarch64: Fix +nocrypto handling
2023-12-13 14:52 ` [PATCH v2] " Andrew Carlotti
@ 2023-12-13 17:34 ` Richard Sandiford
0 siblings, 0 replies; 4+ messages in thread
From: Richard Sandiford @ 2023-12-13 17:34 UTC (permalink / raw)
To: Andrew Carlotti; +Cc: gcc-patches, Richard Earnshaw
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.
>
> This bug should have been picked up by an existing test, but a missing
> newline meant that the pattern incorrectly allowed "+crypto+nocrypto".
>
> 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_4.c: Add terminating newline.
> * gcc.target/aarch64/options_set_27.c: New test.
OK, thanks.
Richard
> diff --git a/gcc/common/config/aarch64/aarch64-common.cc b/gcc/common/config/aarch64/aarch64-common.cc
> index 8fb901029ec2980a048177586b84201b3b398f9e..c2a6d357c0bc17996a25ea5c3a40f69d745c7931 100644
> --- a/gcc/common/config/aarch64/aarch64-common.cc
> +++ b/gcc/common/config/aarch64/aarch64-common.cc
> @@ -311,6 +311,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];
> @@ -320,7 +321,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)
> {
> @@ -339,14 +340,32 @@ aarch64_get_extension_string_for_isa_flags
> 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.native_detect_p
> - && (opt.flag_canonical & current_flags & ~isa_flags))
> - {
> - current_flags &= ~opt.flags_off;
> - outstr += "+no";
> - outstr += opt.name;
> - }
> + {
> + auto flags = opt.flag_canonical;
> + /* As a special case, don't emit "+noaes" or "+nosha2" when we could emit
> + "+nocrypto" instead, in order to support assemblers that predate the
> + separate per-feature crypto flags. Only allow "+nocrypto" when "sm4"
> + is not already enabled (to avoid dependending on whether "+nocrypto"
> + also disables "sm4"). */
> + if (flags & flags_crypto
> + && (flags_crypto & current_flags & ~isa_flags) == flags_crypto
> + && !(current_flags & AARCH64_FL_SM4))
> + continue;
> +
> + if (flags == AARCH64_FL_CRYPTO)
> + /* If either crypto flag needs removing here, then both do. */
> + flags = flags_crypto;
> +
> + if (opt.native_detect_p
> + && (flags & current_flags & ~isa_flags))
> + {
> + current_flags &= ~opt.flags_off;
> + outstr += "+no";
> + outstr += opt.name;
> + }
> + }
>
> return outstr;
> }
> diff --git a/gcc/config/aarch64/aarch64-c.cc b/gcc/config/aarch64/aarch64-c.cc
> index 115a2a8b7568c43a712d819e03147ff84ff182c0..cdc4e453a2054b1a1d2c70bf0b528e497ae0b9ad 100644
> --- a/gcc/config/aarch64/aarch64-c.cc
> +++ b/gcc/config/aarch64/aarch64-c.cc
> @@ -188,7 +188,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 2cd0bc552ebadac06a2838ae2767852c036d0db4..501bb7478a0755fa76c488ec03dcfab6c272851c 100644
> --- a/gcc/config/aarch64/aarch64.h
> +++ b/gcc/config/aarch64/aarch64.h
> @@ -204,7 +204,11 @@ constexpr auto AARCH64_FL_DEFAULT_ISA_MODE = AARCH64_FL_SM_OFF;
>
> #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_SM_OFF (aarch64_isa_flags & AARCH64_FL_SM_OFF)
> #define AARCH64_ISA_SM_ON (aarch64_isa_flags & AARCH64_FL_SM_ON)
> @@ -213,7 +217,6 @@ constexpr auto AARCH64_FL_DEFAULT_ISA_MODE = AARCH64_FL_SM_OFF;
> #define AARCH64_ISA_V8A (aarch64_isa_flags & AARCH64_FL_V8A)
> #define AARCH64_ISA_V8_1A (aarch64_isa_flags & AARCH64_FL_V8_1A)
> #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)
> @@ -294,9 +297,6 @@ constexpr auto AARCH64_FL_DEFAULT_ISA_MODE = AARCH64_FL_SM_OFF;
> #define AARCH64_FL_SCXTNUM AARCH64_FL_V8_5A
> #define AARCH64_FL_ID_PFR2 AARCH64_FL_V8_5A
>
> -/* 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..e35744640130778cba442c365e70028e53508586
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/aarch64/options_set_27.c
> @@ -0,0 +1,9 @@
> +/* { 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 } } */
> diff --git a/gcc/testsuite/gcc.target/aarch64/options_set_4.c b/gcc/testsuite/gcc.target/aarch64/options_set_4.c
> index 5370e02e1531dd26e5a5fb37de0bab6aed513b71..7b00d09a47f0521f148b1dd576b4100db38aec00 100644
> --- a/gcc/testsuite/gcc.target/aarch64/options_set_4.c
> +++ b/gcc/testsuite/gcc.target/aarch64/options_set_4.c
> @@ -6,7 +6,7 @@ int main ()
> return 0;
> }
>
> -/* { dg-final { scan-assembler-times {\.arch armv8\.2\-a\+crc\+crypto} 1 } } */
> +/* { dg-final { scan-assembler-times {\.arch armv8\.2\-a\+crc\+crypto\n} 1 } } */
>
> /* Check if individual bits that make up a grouping is specified that only the
> grouping is kept. */
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-12-13 17:34 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-12-05 19:33 aarch64: Fix +nocrypto handling Andrew Carlotti
2023-12-09 19:09 ` Richard Sandiford
2023-12-13 14:52 ` [PATCH v2] " Andrew Carlotti
2023-12-13 17:34 ` Richard Sandiford
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).