diff mbox series

[01/11] aarch64: Improve mcpu/march conflict check

Message ID a7a3e107-c6f7-093a-5237-3a16aeb92c7e@e124511.cambridge.arm.com
State New
Headers
Series aarch64: Refactor target parsing |

Commit Message

Andrew Carlotti Jan. 10, 2025, 5:21 p.m. UTC 
  Features from a cpu or base architecture that were explicitly disabled
by a +nofeat option were being incorrectly added back in before checking
for conflicts between -mcpu and -march options.  This patch instead
compares the returned feature masks directly.

gcc/ChangeLog:

	* config/aarch64/aarch64.cc (aarch64_override_options): Compare
	returned feature masks directly.

gcc/testsuite/ChangeLog:

	* gcc.target/aarch64/target_attr_crypto_ice_1.c: Prune warning.
	* gcc.target/aarch64/target_attr_crypto_ice_2.c: Ditto.

Comments

Richard Sandiford Jan. 14, 2025, 3:25 p.m. UTC | #1 
Andrew Carlotti <andrew.carlotti@arm.com> writes:
> Features from a cpu or base architecture that were explicitly disabled
> by a +nofeat option were being incorrectly added back in before checking
> for conflicts between -mcpu and -march options.  This patch instead
> compares the returned feature masks directly.
>
> gcc/ChangeLog:
>
> 	* config/aarch64/aarch64.cc (aarch64_override_options): Compare
> 	returned feature masks directly.
>
> gcc/testsuite/ChangeLog:
>
> 	* gcc.target/aarch64/target_attr_crypto_ice_1.c: Prune warning.
> 	* gcc.target/aarch64/target_attr_crypto_ice_2.c: Ditto.

OK, thanks.

I looked at the patch that added this code (g:da332ce109451c89) to see
whether the handling of +no was deliberate behaviour, but I agree that
it doesn't seem to have been.  The patch replaced direct checks of
cpu->flags and arch->flags with the current condition that takes
cpu_isa and arch_isa into account.  This patch takes the next step
and avoids cpu->flags and arch->flags altogether.

I also agree that the new behaviour seems more obvious.

Richard

>
>
> diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc
> index ad31e9d255c05dda00c7c2b4755ccec33ae2c83d..330a04c147a97bcd99d6819290d7f82ff5066a44 100644
> --- a/gcc/config/aarch64/aarch64.cc
> +++ b/gcc/config/aarch64/aarch64.cc
> @@ -19282,13 +19282,10 @@ aarch64_override_options (void)
>  	 cpu features would end up disabling an achitecture feature.  In
>  	 otherwords the cpu features need to be a strict superset of the arch
>  	 features and if so prefer the -march ISA flags.  */
> -      auto full_arch_flags = arch->flags | arch_isa;
> -      auto full_cpu_flags = cpu->flags | cpu_isa;
> -      if (~full_cpu_flags & full_arch_flags)
> +      if (~cpu_isa & arch_isa)
>  	{
>  	  std::string ext_diff
> -	    = aarch64_get_extension_string_for_isa_flags (full_arch_flags,
> -							  full_cpu_flags);
> +	    = aarch64_get_extension_string_for_isa_flags (arch_isa, cpu_isa);
>  	  warning (0, "switch %<-mcpu=%s%> conflicts with %<-march=%s%> switch "
>  		      "and resulted in options %qs being added",
>  		       aarch64_cpu_string,
> diff --git a/gcc/testsuite/gcc.target/aarch64/target_attr_crypto_ice_1.c b/gcc/testsuite/gcc.target/aarch64/target_attr_crypto_ice_1.c
> index 3b354c0611092b1fb66e4a9c2098a9806c749825..f13e5e2560cd43aab570ab5d240e4cf1975d2f12 100644
> --- a/gcc/testsuite/gcc.target/aarch64/target_attr_crypto_ice_1.c
> +++ b/gcc/testsuite/gcc.target/aarch64/target_attr_crypto_ice_1.c
> @@ -1,5 +1,6 @@
>  /* { dg-do compile } */
>  /* { dg-options "-O2 -mcpu=thunderx+nofp -march=armv8-a" } */
> +/* { dg-prune-output "warning: switch .* conflicts" } */
>  
>  #include "arm_neon.h"
>  
> diff --git a/gcc/testsuite/gcc.target/aarch64/target_attr_crypto_ice_2.c b/gcc/testsuite/gcc.target/aarch64/target_attr_crypto_ice_2.c
> index d0a62b83351b671d157ec0de083d681394056d79..ab2549228a7fa06aa26592e02d0d2055f6b990ed 100644
> --- a/gcc/testsuite/gcc.target/aarch64/target_attr_crypto_ice_2.c
> +++ b/gcc/testsuite/gcc.target/aarch64/target_attr_crypto_ice_2.c
> @@ -1,5 +1,6 @@
>  /* { dg-do compile } */
>  /* { dg-options "-O2 -mcpu=thunderx+nofp -march=armv8-a" } */
> +/* { dg-prune-output "warning: switch .* conflicts" } */
>  
>  /* Make sure that we don't ICE when dealing with vector parameters
>     in a simd-tagged function within a non-simd translation unit.  */
diff mbox series

Patch

diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc
index ad31e9d255c05dda00c7c2b4755ccec33ae2c83d..330a04c147a97bcd99d6819290d7f82ff5066a44 100644
--- a/gcc/config/aarch64/aarch64.cc
+++ b/gcc/config/aarch64/aarch64.cc
@@ -19282,13 +19282,10 @@  aarch64_override_options (void)
 	 cpu features would end up disabling an achitecture feature.  In
 	 otherwords the cpu features need to be a strict superset of the arch
 	 features and if so prefer the -march ISA flags.  */
-      auto full_arch_flags = arch->flags | arch_isa;
-      auto full_cpu_flags = cpu->flags | cpu_isa;
-      if (~full_cpu_flags & full_arch_flags)
+      if (~cpu_isa & arch_isa)
 	{
 	  std::string ext_diff
-	    = aarch64_get_extension_string_for_isa_flags (full_arch_flags,
-							  full_cpu_flags);
+	    = aarch64_get_extension_string_for_isa_flags (arch_isa, cpu_isa);
 	  warning (0, "switch %<-mcpu=%s%> conflicts with %<-march=%s%> switch "
 		      "and resulted in options %qs being added",
 		       aarch64_cpu_string,
diff --git a/gcc/testsuite/gcc.target/aarch64/target_attr_crypto_ice_1.c b/gcc/testsuite/gcc.target/aarch64/target_attr_crypto_ice_1.c
index 3b354c0611092b1fb66e4a9c2098a9806c749825..f13e5e2560cd43aab570ab5d240e4cf1975d2f12 100644
--- a/gcc/testsuite/gcc.target/aarch64/target_attr_crypto_ice_1.c
+++ b/gcc/testsuite/gcc.target/aarch64/target_attr_crypto_ice_1.c
@@ -1,5 +1,6 @@ 
 /* { dg-do compile } */
 /* { dg-options "-O2 -mcpu=thunderx+nofp -march=armv8-a" } */
+/* { dg-prune-output "warning: switch .* conflicts" } */
 
 #include "arm_neon.h"
 
diff --git a/gcc/testsuite/gcc.target/aarch64/target_attr_crypto_ice_2.c b/gcc/testsuite/gcc.target/aarch64/target_attr_crypto_ice_2.c
index d0a62b83351b671d157ec0de083d681394056d79..ab2549228a7fa06aa26592e02d0d2055f6b990ed 100644
--- a/gcc/testsuite/gcc.target/aarch64/target_attr_crypto_ice_2.c
+++ b/gcc/testsuite/gcc.target/aarch64/target_attr_crypto_ice_2.c
@@ -1,5 +1,6 @@ 
 /* { dg-do compile } */
 /* { dg-options "-O2 -mcpu=thunderx+nofp -march=armv8-a" } */
+/* { dg-prune-output "warning: switch .* conflicts" } */
 
 /* Make sure that we don't ICE when dealing with vector parameters
    in a simd-tagged function within a non-simd translation unit.  */