diff mbox series

[v2] aarch64: Fix +nocrypto handling

Message ID 4b906d4e-0624-184b-b296-283ac3479d5a@e124511.cambridge.arm.com
State New
Headers show
Series [v2] aarch64: Fix +nocrypto handling | expand

Commit Message

Andrew Carlotti Dec. 13, 2023, 2:52 p.m. UTC 
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.

Comments

Richard Sandiford Dec. 13, 2023, 5:34 p.m. UTC | #1 
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. */
diff mbox series

Patch

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. */