AArch64: Cleanup CPU option processing code

  The --with-cpu/--with-arch configure option processing not only checks valid arguments
but also sets TARGET_CPU_DEFAULT with a CPU and extension bitmask.  This isn't used
however since a --with-cpu is translated into a -mcpu option which is processed as if
written on the command-line (so TARGET_CPU_DEFAULT is never accessed).

So remove all the complex processing and bitmask, and just validate the option.
Fix a bug that always reports valid architecture extensions as invalid.  As a result
the CPU processing in aarch64.cc can be simplified.

Bootstrap OK, regress pass, OK for commit?

ChangeLog:
2022-04-19  Wilco Dijkstra  <wdijkstr@arm.com>

        * config.gcc (aarch64*-*-*): Simplify --with-cpu and --with-arch
        processing.  Add support for architectural extensions.
        * config/aarch64/aarch64.h (TARGET_CPU_DEFAULT): Remove
        AARCH64_CPU_DEFAULT_FLAGS.
        (TARGET_CPU_NBITS): Remove.
        (TARGET_CPU_MASK): Remove.
        * config/aarch64/aarch64.cc (AARCH64_CPU_DEFAULT_FLAGS): Remove define.
        (get_tune_cpu): Assert CPU is always valid.
        (get_arch): Assert architecture is always valid.
        (aarch64_override_options): Cleanup CPU selection code and simplify logic.

---
  

Wilco Dijkstra via Gcc-patches <gcc-patches@gcc.gnu.org> writes:
> The --with-cpu/--with-arch configure option processing not only checks valid arguments
> but also sets TARGET_CPU_DEFAULT with a CPU and extension bitmask.  This isn't used
> however since a --with-cpu is translated into a -mcpu option which is processed as if
> written on the command-line (so TARGET_CPU_DEFAULT is never accessed).
>
> So remove all the complex processing and bitmask, and just validate the option.
> Fix a bug that always reports valid architecture extensions as invalid.  As a result
> the CPU processing in aarch64.cc can be simplified.
>
> Bootstrap OK, regress pass, OK for commit?

Although invoking ./cc1 directly only half-works with --with-arch,
it half-works well-enough that I'd still like to keep it working.
But I agree we should apply your change first, then I can follow up
with a patch to make --with-* work with ./cc1 later.  (I have a version
locally and the net result is much simpler than the status quo, as well
as hopefully actually working properly.)

My main question about the patch itself is:

> +  explicit_arch = selected_arch->arch;
>    if (!selected_tune)
>      selected_tune = selected_cpu;
> +  explicit_tune_core = selected_tune->ident;
> +
> +  gcc_assert (explicit_tune_core != aarch64_none);
> +  gcc_assert (explicit_arch != aarch64_no_arch);

Do we still need both selected_arch and explicit_arch?  explicit_arch
seems a misnomer now, since it includes implicit as well as explicit
choices.  Same for selected_tune and explicit_tune_core.

aarch64_option_restore has:

  if (opts->x_explicit_tune_core == aarch64_none
      && opts->x_explicit_arch != aarch64_no_arch)
    selected_tune = &all_cores[selected_arch->ident];
  else
    selected_tune = aarch64_get_tune_cpu (ptr->x_explicit_tune_core);

Is the “if” condition ever true, or can we now restore the tune
info unconditionally?

Thanks,
Richard
  

Hi Richard,

> Although invoking ./cc1 directly only half-works with --with-arch,
> it half-works well-enough that I'd still like to keep it working.
> But I agree we should apply your change first, then I can follow up
> with a patch to make --with-* work with ./cc1 later.  (I have a version
> locally and the net result is much simpler than the status quo, as well
> as hopefully actually working properly.)

That sounds good indeed. Is that changing TARGET_CPU_DEFAULT into
a string can could just be parsed like a -mcpu option?

> Do we still need both selected_arch and explicit_arch?  explicit_arch
> seems a misnomer now, since it includes implicit as well as explicit
> choices.  Same for selected_tune and explicit_tune_core.

At the moment we do since these are settings that must be saved/restored
since they can be overridden. However it may be possible to do further
cleanups to remove some of this. I also wonder whether we can remove the
internal override feature (override_tune_string) since that further complicates
the tunings, and I'm not convinced that it is either useful or being used at all.

> aarch64_option_restore has:
>
>  if (opts->x_explicit_tune_core == aarch64_none
>      && opts->x_explicit_arch != aarch64_no_arch)
>    selected_tune = &all_cores[selected_arch->ident];
>  else
>    selected_tune = aarch64_get_tune_cpu (ptr->x_explicit_tune_core);
>
> Is the “if” condition ever true, or can we now restore the tune
> info unconditionally?

Yes that was added a year or so after I created this patch, so this
is now redundant. I've removed it in v2:


The --with-cpu/--with-arch configure option processing not only checks valid arguments
but also sets TARGET_CPU_DEFAULT with a CPU and extension bitmask.  This isn't used
however since a --with-cpu is translated into a -mcpu option which is processed as if
written on the command-line (so TARGET_CPU_DEFAULT is never accessed).

So remove all the complex processing and bitmask, and just validate the option.
Fix a bug that always reports valid architecture extensions as invalid.  As a result
the CPU processing in aarch64.c can be simplified.

Bootstrap OK, regress pass, OK for commit?

ChangeLog:
2022-04-19  Wilco Dijkstra  <wdijkstr@arm.com>

        * config.gcc (aarch64*-*-*): Simplify --with-cpu and --with-arch
        processing.  Add support for architectural extensions.
        * config/aarch64/aarch64.h (TARGET_CPU_DEFAULT): Remove
        AARCH64_CPU_DEFAULT_FLAGS.
        (TARGET_CPU_NBITS): Remove.
        (TARGET_CPU_MASK): Remove.
        * config/aarch64/aarch64.cc (AARCH64_CPU_DEFAULT_FLAGS): Remove define.
        (get_tune_cpu): Assert CPU is always valid.
        (get_arch): Assert architecture is always valid.
        (aarch64_override_options): Cleanup CPU selection code and simplify logic.
        (aarch64_option_restore): Remove unnecessary checks on tune.

---

diff --git a/gcc/config.gcc b/gcc/config.gcc
index c5064dd376660c192d5573997b4fc86b6b3e3838..b48d5451e8027c93fb1f614812589183d0a88c4b 100644
--- a/gcc/config.gcc
+++ b/gcc/config.gcc
@@ -4178,8 +4178,6 @@ case "${target}" in
 			  pattern=AARCH64_CORE
 			fi
 
-			ext_mask=AARCH64_CPU_DEFAULT_FLAGS
-
 			# Find the base CPU or ARCH id in aarch64-cores.def or
 			# aarch64-arches.def
 			if [ x"$base_val" = x ] \
@@ -4187,23 +4185,6 @@ case "${target}" in
 				    ${srcdir}/config/aarch64/$def \
 				    > /dev/null; then
 
-			  if [ $which = arch ]; then
-				base_id=`grep "^$pattern(\"$base_val\"," \
-				  ${srcdir}/config/aarch64/$def | \
-				  sed -e 's/^[^,]*,[ 	]*//' | \
-				  sed -e 's/,.*$//'`
-				# Extract the architecture flags from aarch64-arches.def
-				ext_mask=`grep "^$pattern(\"$base_val\"," \
-				   ${srcdir}/config/aarch64/$def | \
-				   sed -e 's/)$//' | \
-				   sed -e 's/^.*,//'`
-			  else
-				base_id=`grep "^$pattern(\"$base_val\"," \
-				  ${srcdir}/config/aarch64/$def | \
-				  sed -e 's/^[^,]*,[ 	]*//' | \
-				  sed -e 's/,.*$//'`
-			  fi
-
 			  # Disallow extensions in --with-tune=cortex-a53+crc.
 			  if [ $which = tune ] && [ x"$ext_val" != x ]; then
 			    echo "Architecture extensions not supported in --with-$which=$val" 1>&2
@@ -4234,25 +4215,7 @@ case "${target}" in
 					grep "^\"$base_ext\""`
 
 				if [ x"$base_ext" = x ] \
-				    || [[ -n $opt_line ]]; then
-
-				  # These regexp extract the elements based on
-				  # their group match index in the regexp.
-				  ext_canon=`echo -e "$opt_line" | \
-					sed -e "s/$sed_patt/\2/"`
-				  ext_on=`echo -e "$opt_line" | \
-					sed -e "s/$sed_patt/\3/"`
-				  ext_off=`echo -e "$opt_line" | \
-					sed -e "s/$sed_patt/\4/"`
-
-				  if [ $ext = $base_ext ]; then
-					# Adding extension
-					ext_mask="("$ext_mask") | ("$ext_on" | "$ext_canon")"
-				  else
-					# Removing extension
-					ext_mask="("$ext_mask") & ~("$ext_off" | "$ext_canon")"
-				  fi
-
+				    || [ x"$opt_line" != x ]; then
 				  true
 				else
 				  echo "Unknown extension used in --with-$which=$val" 1>&2
@@ -4261,10 +4224,6 @@ case "${target}" in
 				ext_val=`echo $ext_val | sed -e 's/[a-z0-9]\+//'`
 			  done
 
-			  ext_mask="(("$ext_mask") << TARGET_CPU_NBITS)"
-			  if [ x"$base_id" != x ]; then
-				target_cpu_cname="TARGET_CPU_$base_id | $ext_mask"
-			  fi
 			  true
 			else
 			  # Allow --with-$which=native.
diff --git a/gcc/config/aarch64/aarch64.h b/gcc/config/aarch64/aarch64.h
index 359b6e8561faa38f53a806b8c114c83ae37e7e17..f835da33b72f36bbf25a0e1328135411bd8ab4f6 100644
--- a/gcc/config/aarch64/aarch64.h
+++ b/gcc/config/aarch64/aarch64.h
@@ -813,16 +813,9 @@ enum target_cpus
   TARGET_CPU_generic
 };
 
-/* Define how many bits are used to represent the CPU in TARGET_CPU_DEFAULT.
-   This needs to be big enough to fit the value of TARGET_CPU_generic.
-   All bits after this are used to represent the AARCH64_CPU_DEFAULT_FLAGS.  */
-#define TARGET_CPU_NBITS 8
-#define TARGET_CPU_MASK ((1 << TARGET_CPU_NBITS) - 1)
-
 /* If there is no CPU defined at configure, use generic as default.  */
 #ifndef TARGET_CPU_DEFAULT
-#define TARGET_CPU_DEFAULT \
-  (TARGET_CPU_generic | (AARCH64_CPU_DEFAULT_FLAGS << TARGET_CPU_NBITS))
+# define TARGET_CPU_DEFAULT TARGET_CPU_generic
 #endif
 
 /* If inserting NOP before a mult-accumulate insn remember to adjust the
diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc
index f650abbc4ce49cf0947049931f86bad1130c3428..43d87d1b9c4ef1a85094e51f81745f98f1ef27fb 100644
--- a/gcc/config/aarch64/aarch64.cc
+++ b/gcc/config/aarch64/aarch64.cc
@@ -2760,8 +2760,6 @@ static const struct attribute_spec aarch64_attribute_table[] =
   { NULL,                 0, 0, false, false, false, false, NULL, NULL }
 };
 
-#define AARCH64_CPU_DEFAULT_FLAGS ((selected_cpu) ? selected_cpu->flags : 0)
-
 /* An ISA extension in the co-processor and main instruction set space.  */
 struct aarch64_option_extension
 {
@@ -18057,39 +18055,24 @@ aarch64_validate_mtune (const char *str, const struct processor **res)
   return false;
 }
 
-static_assert (TARGET_CPU_generic < TARGET_CPU_MASK,
-	       "TARGET_CPU_NBITS is big enough");
-
-/* Return the CPU corresponding to the enum CPU.
-   If it doesn't specify a cpu, return the default.  */
+/* Return the CPU corresponding to the enum CPU.  */
 
 static const struct processor *
 aarch64_get_tune_cpu (enum aarch64_processor cpu)
 {
-  if (cpu != aarch64_none)
-    return &all_cores[cpu];
+  gcc_assert (cpu != aarch64_none);
 
-  /* The & TARGET_CPU_MASK is to extract the bottom TARGET_CPU_NBITS bits that
-     encode the default cpu as selected by the --with-cpu GCC configure option
-     in config.gcc.
-     ???: The whole TARGET_CPU_DEFAULT and AARCH64_CPU_DEFAULT_FLAGS
-     flags mechanism should be reworked to make it more sane.  */
-  return &all_cores[TARGET_CPU_DEFAULT & TARGET_CPU_MASK];
+  return &all_cores[cpu];
 }
 
-/* Return the architecture corresponding to the enum ARCH.
-   If it doesn't specify a valid architecture, return the default.  */
+/* Return the architecture corresponding to the enum ARCH.  */
 
 static const struct processor *
 aarch64_get_arch (enum aarch64_arch arch)
 {
-  if (arch != aarch64_no_arch)
-    return &all_architectures[arch];
-
-  const struct processor *cpu
-    = &all_cores[TARGET_CPU_DEFAULT & TARGET_CPU_MASK];
+  gcc_assert (arch != aarch64_no_arch);
 
-  return &all_architectures[cpu->arch];
+  return &all_architectures[arch];
 }
 
 /* Return the VG value associated with -msve-vector-bits= value VALUE.  */
@@ -18127,10 +18110,6 @@ aarch64_override_options (void)
   uint64_t arch_isa = 0;
   aarch64_isa_flags = 0;
 
-  bool valid_cpu = true;
-  bool valid_tune = true;
-  bool valid_arch = true;
-
   selected_cpu = NULL;
   selected_arch = NULL;
   selected_tune = NULL;
@@ -18145,77 +18124,56 @@ aarch64_override_options (void)
      If either of -march or -mtune is given, they override their
      respective component of -mcpu.  */
   if (aarch64_cpu_string)
-    valid_cpu = aarch64_validate_mcpu (aarch64_cpu_string, &selected_cpu,
-					&cpu_isa);
+    aarch64_validate_mcpu (aarch64_cpu_string, &selected_cpu, &cpu_isa);
 
   if (aarch64_arch_string)
-    valid_arch = aarch64_validate_march (aarch64_arch_string, &selected_arch,
-					  &arch_isa);
+    aarch64_validate_march (aarch64_arch_string, &selected_arch, &arch_isa);
 
   if (aarch64_tune_string)
-    valid_tune = aarch64_validate_mtune (aarch64_tune_string, &selected_tune);
+    aarch64_validate_mtune (aarch64_tune_string, &selected_tune);
 
 #ifdef SUBTARGET_OVERRIDE_OPTIONS
   SUBTARGET_OVERRIDE_OPTIONS;
 #endif
 
-  /* If the user did not specify a processor, choose the default
-     one for them.  This will be the CPU set during configuration using
-     --with-cpu, otherwise it is "generic".  */
-  if (!selected_cpu)
-    {
-      if (selected_arch)
-	{
-	  selected_cpu = &all_cores[selected_arch->ident];
-	  aarch64_isa_flags = arch_isa;
-	  explicit_arch = selected_arch->arch;
-	}
-      else
-	{
-	  /* Get default configure-time CPU.  */
-	  selected_cpu = aarch64_get_tune_cpu (aarch64_none);
-	  aarch64_isa_flags = TARGET_CPU_DEFAULT >> TARGET_CPU_NBITS;
-	}
-
-      if (selected_tune)
-	explicit_tune_core = selected_tune->ident;
-    }
-  /* If both -mcpu and -march are specified check that they are architecturally
-     compatible, warn if they're not and prefer the -march ISA flags.  */
-  else if (selected_arch)
+  if (selected_cpu && selected_arch)
     {
+      /* If both -mcpu and -march are specified, warn if they are not
+	 architecturally compatible and prefer the -march ISA flags.  */
       if (selected_arch->arch != selected_cpu->arch)
 	{
 	  warning (0, "switch %<-mcpu=%s%> conflicts with %<-march=%s%> switch",
 		       aarch64_cpu_string,
 		       aarch64_arch_string);
 	}
+
       aarch64_isa_flags = arch_isa;
-      explicit_arch = selected_arch->arch;
-      explicit_tune_core = selected_tune ? selected_tune->ident
-					  : selected_cpu->ident;
     }
-  else
+  else if (selected_cpu)
     {
-      /* -mcpu but no -march.  */
-      aarch64_isa_flags = cpu_isa;
-      explicit_tune_core = selected_tune ? selected_tune->ident
-					  : selected_cpu->ident;
-      gcc_assert (selected_cpu);
       selected_arch = &all_architectures[selected_cpu->arch];
-      explicit_arch = selected_arch->arch;
+      aarch64_isa_flags = cpu_isa;
     }
-
-  /* Set the arch as well as we will need it when outputing
-     the .arch directive in assembly.  */
-  if (!selected_arch)
+  else if (selected_arch)
     {
-      gcc_assert (selected_cpu);
+      selected_cpu = &all_cores[selected_arch->ident];
+      aarch64_isa_flags = arch_isa;
+    }
+  else
+    {
+      /* No -mcpu or -march specified, so use the default CPU.  */
+      selected_cpu = &all_cores[TARGET_CPU_DEFAULT];
       selected_arch = &all_architectures[selected_cpu->arch];
+      aarch64_isa_flags = selected_cpu->flags;
     }
 
+  explicit_arch = selected_arch->arch;
   if (!selected_tune)
     selected_tune = selected_cpu;
+  explicit_tune_core = selected_tune->ident;
+
+  gcc_assert (explicit_tune_core != aarch64_none);
+  gcc_assert (explicit_arch != aarch64_no_arch);
 
   if (aarch64_enable_bti == 2)
     {
@@ -18251,15 +18209,6 @@ aarch64_override_options (void)
   if (aarch64_ra_sign_scope != AARCH64_FUNCTION_NONE && TARGET_ILP32)
     sorry ("return address signing is only supported for %<-mabi=lp64%>");
 
-  /* Make sure we properly set up the explicit options.  */
-  if ((aarch64_cpu_string && valid_cpu)
-       || (aarch64_tune_string && valid_tune))
-    gcc_assert (explicit_tune_core != aarch64_none);
-
-  if ((aarch64_cpu_string && valid_cpu)
-       || (aarch64_arch_string && valid_arch))
-    gcc_assert (explicit_arch != aarch64_no_arch);
-
   /* The pass to insert speculation tracking runs before
      shrink-wrapping and the latter does not know how to update the
      tracking status.  So disable it in this case.  */
  

Wilco Dijkstra via Gcc-patches <gcc-patches@gcc.gnu.org> writes:
> Hi Richard,
>
>> Although invoking ./cc1 directly only half-works with --with-arch,
>> it half-works well-enough that I'd still like to keep it working.
>> But I agree we should apply your change first, then I can follow up
>> with a patch to make --with-* work with ./cc1 later.  (I have a version
>> locally and the net result is much simpler than the status quo, as well
>> as hopefully actually working properly.)
>
> That sounds good indeed. Is that changing TARGET_CPU_DEFAULT into
> a string can could just be parsed like a -mcpu option?

Yeah, it emulates the DRIVER_SELF_SPECS stuff using string macros.

>> Do we still need both selected_arch and explicit_arch?  explicit_arch
>> seems a misnomer now, since it includes implicit as well as explicit
>> choices.  Same for selected_tune and explicit_tune_core.
>
> At the moment we do since these are settings that must be saved/restored
> since they can be overridden.

Right, but I was wondering if we could save/restore them based on the
selected_* values instead.  However…

> However it may be possible to do further cleanups to remove some of this.

…I agree that might as well be a separate clean-up.

> I also wonder whether we can remove the internal override feature
> (override_tune_string) since that further complicates the tunings, and
> I'm not convinced that it is either useful or being used at all.

It's a developer option rather than a user-facing thing.  I found it
really useful when doing the Neoverse V1 tuning, and there are some
tests that rely on it.  I'd prefer to keep it if possible.

>> aarch64_option_restore has:
>>
>>  if (opts->x_explicit_tune_core == aarch64_none
>>      && opts->x_explicit_arch != aarch64_no_arch)
>>    selected_tune = &all_cores[selected_arch->ident];
>>  else
>>    selected_tune = aarch64_get_tune_cpu (ptr->x_explicit_tune_core);
>>
>> Is the “if” condition ever true, or can we now restore the tune
>> info unconditionally?
>
> Yes that was added a year or so after I created this patch, so this
> is now redundant. I've removed it in v2:
>
>
> The --with-cpu/--with-arch configure option processing not only checks valid arguments
> but also sets TARGET_CPU_DEFAULT with a CPU and extension bitmask.  This isn't used
> however since a --with-cpu is translated into a -mcpu option which is processed as if
> written on the command-line (so TARGET_CPU_DEFAULT is never accessed).
>
> So remove all the complex processing and bitmask, and just validate the option.
> Fix a bug that always reports valid architecture extensions as invalid.  As a result
> the CPU processing in aarch64.c can be simplified.
>
> Bootstrap OK, regress pass, OK for commit?
>
> ChangeLog:
> 2022-04-19  Wilco Dijkstra  <wdijkstr@arm.com>
>
>         * config.gcc (aarch64*-*-*): Simplify --with-cpu and --with-arch
>         processing.  Add support for architectural extensions.
>         * config/aarch64/aarch64.h (TARGET_CPU_DEFAULT): Remove
>         AARCH64_CPU_DEFAULT_FLAGS.
>         (TARGET_CPU_NBITS): Remove.
>         (TARGET_CPU_MASK): Remove.
>         * config/aarch64/aarch64.cc (AARCH64_CPU_DEFAULT_FLAGS): Remove define.
>         (get_tune_cpu): Assert CPU is always valid.
>         (get_arch): Assert architecture is always valid.
>         (aarch64_override_options): Cleanup CPU selection code and simplify logic.
>         (aarch64_option_restore): Remove unnecessary checks on tune.

Looks like you might have attached the old patch.  The aarch64_option_restore
change is mentioned in the changelog but doesn't appear in the patch itself.

Thanks,
Richard

>
> ---
>
> diff --git a/gcc/config.gcc b/gcc/config.gcc
> index c5064dd376660c192d5573997b4fc86b6b3e3838..b48d5451e8027c93fb1f614812589183d0a88c4b 100644
> --- a/gcc/config.gcc
> +++ b/gcc/config.gcc
> @@ -4178,8 +4178,6 @@ case "${target}" in
>  			  pattern=AARCH64_CORE
>  			fi
>  
> -			ext_mask=AARCH64_CPU_DEFAULT_FLAGS
> -
>  			# Find the base CPU or ARCH id in aarch64-cores.def or
>  			# aarch64-arches.def
>  			if [ x"$base_val" = x ] \
> @@ -4187,23 +4185,6 @@ case "${target}" in
>  				    ${srcdir}/config/aarch64/$def \
>  				    > /dev/null; then
>  
> -			  if [ $which = arch ]; then
> -				base_id=`grep "^$pattern(\"$base_val\"," \
> -				  ${srcdir}/config/aarch64/$def | \
> -				  sed -e 's/^[^,]*,[ 	]*//' | \
> -				  sed -e 's/,.*$//'`
> -				# Extract the architecture flags from aarch64-arches.def
> -				ext_mask=`grep "^$pattern(\"$base_val\"," \
> -				   ${srcdir}/config/aarch64/$def | \
> -				   sed -e 's/)$//' | \
> -				   sed -e 's/^.*,//'`
> -			  else
> -				base_id=`grep "^$pattern(\"$base_val\"," \
> -				  ${srcdir}/config/aarch64/$def | \
> -				  sed -e 's/^[^,]*,[ 	]*//' | \
> -				  sed -e 's/,.*$//'`
> -			  fi
> -
>  			  # Disallow extensions in --with-tune=cortex-a53+crc.
>  			  if [ $which = tune ] && [ x"$ext_val" != x ]; then
>  			    echo "Architecture extensions not supported in --with-$which=$val" 1>&2
> @@ -4234,25 +4215,7 @@ case "${target}" in
>  					grep "^\"$base_ext\""`
>  
>  				if [ x"$base_ext" = x ] \
> -				    || [[ -n $opt_line ]]; then
> -
> -				  # These regexp extract the elements based on
> -				  # their group match index in the regexp.
> -				  ext_canon=`echo -e "$opt_line" | \
> -					sed -e "s/$sed_patt/\2/"`
> -				  ext_on=`echo -e "$opt_line" | \
> -					sed -e "s/$sed_patt/\3/"`
> -				  ext_off=`echo -e "$opt_line" | \
> -					sed -e "s/$sed_patt/\4/"`
> -
> -				  if [ $ext = $base_ext ]; then
> -					# Adding extension
> -					ext_mask="("$ext_mask") | ("$ext_on" | "$ext_canon")"
> -				  else
> -					# Removing extension
> -					ext_mask="("$ext_mask") & ~("$ext_off" | "$ext_canon")"
> -				  fi
> -
> +				    || [ x"$opt_line" != x ]; then
>  				  true
>  				else
>  				  echo "Unknown extension used in --with-$which=$val" 1>&2
> @@ -4261,10 +4224,6 @@ case "${target}" in
>  				ext_val=`echo $ext_val | sed -e 's/[a-z0-9]\+//'`
>  			  done
>  
> -			  ext_mask="(("$ext_mask") << TARGET_CPU_NBITS)"
> -			  if [ x"$base_id" != x ]; then
> -				target_cpu_cname="TARGET_CPU_$base_id | $ext_mask"
> -			  fi
>  			  true
>  			else
>  			  # Allow --with-$which=native.
> diff --git a/gcc/config/aarch64/aarch64.h b/gcc/config/aarch64/aarch64.h
> index 359b6e8561faa38f53a806b8c114c83ae37e7e17..f835da33b72f36bbf25a0e1328135411bd8ab4f6 100644
> --- a/gcc/config/aarch64/aarch64.h
> +++ b/gcc/config/aarch64/aarch64.h
> @@ -813,16 +813,9 @@ enum target_cpus
>    TARGET_CPU_generic
>  };
>  
> -/* Define how many bits are used to represent the CPU in TARGET_CPU_DEFAULT.
> -   This needs to be big enough to fit the value of TARGET_CPU_generic.
> -   All bits after this are used to represent the AARCH64_CPU_DEFAULT_FLAGS.  */
> -#define TARGET_CPU_NBITS 8
> -#define TARGET_CPU_MASK ((1 << TARGET_CPU_NBITS) - 1)
> -
>  /* If there is no CPU defined at configure, use generic as default.  */
>  #ifndef TARGET_CPU_DEFAULT
> -#define TARGET_CPU_DEFAULT \
> -  (TARGET_CPU_generic | (AARCH64_CPU_DEFAULT_FLAGS << TARGET_CPU_NBITS))
> +# define TARGET_CPU_DEFAULT TARGET_CPU_generic
>  #endif
>  
>  /* If inserting NOP before a mult-accumulate insn remember to adjust the
> diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc
> index f650abbc4ce49cf0947049931f86bad1130c3428..43d87d1b9c4ef1a85094e51f81745f98f1ef27fb 100644
> --- a/gcc/config/aarch64/aarch64.cc
> +++ b/gcc/config/aarch64/aarch64.cc
> @@ -2760,8 +2760,6 @@ static const struct attribute_spec aarch64_attribute_table[] =
>    { NULL,                 0, 0, false, false, false, false, NULL, NULL }
>  };
>  
> -#define AARCH64_CPU_DEFAULT_FLAGS ((selected_cpu) ? selected_cpu->flags : 0)
> -
>  /* An ISA extension in the co-processor and main instruction set space.  */
>  struct aarch64_option_extension
>  {
> @@ -18057,39 +18055,24 @@ aarch64_validate_mtune (const char *str, const struct processor **res)
>    return false;
>  }
>  
> -static_assert (TARGET_CPU_generic < TARGET_CPU_MASK,
> -	       "TARGET_CPU_NBITS is big enough");
> -
> -/* Return the CPU corresponding to the enum CPU.
> -   If it doesn't specify a cpu, return the default.  */
> +/* Return the CPU corresponding to the enum CPU.  */
>  
>  static const struct processor *
>  aarch64_get_tune_cpu (enum aarch64_processor cpu)
>  {
> -  if (cpu != aarch64_none)
> -    return &all_cores[cpu];
> +  gcc_assert (cpu != aarch64_none);
>  
> -  /* The & TARGET_CPU_MASK is to extract the bottom TARGET_CPU_NBITS bits that
> -     encode the default cpu as selected by the --with-cpu GCC configure option
> -     in config.gcc.
> -     ???: The whole TARGET_CPU_DEFAULT and AARCH64_CPU_DEFAULT_FLAGS
> -     flags mechanism should be reworked to make it more sane.  */
> -  return &all_cores[TARGET_CPU_DEFAULT & TARGET_CPU_MASK];
> +  return &all_cores[cpu];
>  }
>  
> -/* Return the architecture corresponding to the enum ARCH.
> -   If it doesn't specify a valid architecture, return the default.  */
> +/* Return the architecture corresponding to the enum ARCH.  */
>  
>  static const struct processor *
>  aarch64_get_arch (enum aarch64_arch arch)
>  {
> -  if (arch != aarch64_no_arch)
> -    return &all_architectures[arch];
> -
> -  const struct processor *cpu
> -    = &all_cores[TARGET_CPU_DEFAULT & TARGET_CPU_MASK];
> +  gcc_assert (arch != aarch64_no_arch);
>  
> -  return &all_architectures[cpu->arch];
> +  return &all_architectures[arch];
>  }
>  
>  /* Return the VG value associated with -msve-vector-bits= value VALUE.  */
> @@ -18127,10 +18110,6 @@ aarch64_override_options (void)
>    uint64_t arch_isa = 0;
>    aarch64_isa_flags = 0;
>  
> -  bool valid_cpu = true;
> -  bool valid_tune = true;
> -  bool valid_arch = true;
> -
>    selected_cpu = NULL;
>    selected_arch = NULL;
>    selected_tune = NULL;
> @@ -18145,77 +18124,56 @@ aarch64_override_options (void)
>       If either of -march or -mtune is given, they override their
>       respective component of -mcpu.  */
>    if (aarch64_cpu_string)
> -    valid_cpu = aarch64_validate_mcpu (aarch64_cpu_string, &selected_cpu,
> -					&cpu_isa);
> +    aarch64_validate_mcpu (aarch64_cpu_string, &selected_cpu, &cpu_isa);
>  
>    if (aarch64_arch_string)
> -    valid_arch = aarch64_validate_march (aarch64_arch_string, &selected_arch,
> -					  &arch_isa);
> +    aarch64_validate_march (aarch64_arch_string, &selected_arch, &arch_isa);
>  
>    if (aarch64_tune_string)
> -    valid_tune = aarch64_validate_mtune (aarch64_tune_string, &selected_tune);
> +    aarch64_validate_mtune (aarch64_tune_string, &selected_tune);
>  
>  #ifdef SUBTARGET_OVERRIDE_OPTIONS
>    SUBTARGET_OVERRIDE_OPTIONS;
>  #endif
>  
> -  /* If the user did not specify a processor, choose the default
> -     one for them.  This will be the CPU set during configuration using
> -     --with-cpu, otherwise it is "generic".  */
> -  if (!selected_cpu)
> -    {
> -      if (selected_arch)
> -	{
> -	  selected_cpu = &all_cores[selected_arch->ident];
> -	  aarch64_isa_flags = arch_isa;
> -	  explicit_arch = selected_arch->arch;
> -	}
> -      else
> -	{
> -	  /* Get default configure-time CPU.  */
> -	  selected_cpu = aarch64_get_tune_cpu (aarch64_none);
> -	  aarch64_isa_flags = TARGET_CPU_DEFAULT >> TARGET_CPU_NBITS;
> -	}
> -
> -      if (selected_tune)
> -	explicit_tune_core = selected_tune->ident;
> -    }
> -  /* If both -mcpu and -march are specified check that they are architecturally
> -     compatible, warn if they're not and prefer the -march ISA flags.  */
> -  else if (selected_arch)
> +  if (selected_cpu && selected_arch)
>      {
> +      /* If both -mcpu and -march are specified, warn if they are not
> +	 architecturally compatible and prefer the -march ISA flags.  */
>        if (selected_arch->arch != selected_cpu->arch)
>  	{
>  	  warning (0, "switch %<-mcpu=%s%> conflicts with %<-march=%s%> switch",
>  		       aarch64_cpu_string,
>  		       aarch64_arch_string);
>  	}
> +
>        aarch64_isa_flags = arch_isa;
> -      explicit_arch = selected_arch->arch;
> -      explicit_tune_core = selected_tune ? selected_tune->ident
> -					  : selected_cpu->ident;
>      }
> -  else
> +  else if (selected_cpu)
>      {
> -      /* -mcpu but no -march.  */
> -      aarch64_isa_flags = cpu_isa;
> -      explicit_tune_core = selected_tune ? selected_tune->ident
> -					  : selected_cpu->ident;
> -      gcc_assert (selected_cpu);
>        selected_arch = &all_architectures[selected_cpu->arch];
> -      explicit_arch = selected_arch->arch;
> +      aarch64_isa_flags = cpu_isa;
>      }
> -
> -  /* Set the arch as well as we will need it when outputing
> -     the .arch directive in assembly.  */
> -  if (!selected_arch)
> +  else if (selected_arch)
>      {
> -      gcc_assert (selected_cpu);
> +      selected_cpu = &all_cores[selected_arch->ident];
> +      aarch64_isa_flags = arch_isa;
> +    }
> +  else
> +    {
> +      /* No -mcpu or -march specified, so use the default CPU.  */
> +      selected_cpu = &all_cores[TARGET_CPU_DEFAULT];
>        selected_arch = &all_architectures[selected_cpu->arch];
> +      aarch64_isa_flags = selected_cpu->flags;
>      }
>  
> +  explicit_arch = selected_arch->arch;
>    if (!selected_tune)
>      selected_tune = selected_cpu;
> +  explicit_tune_core = selected_tune->ident;
> +
> +  gcc_assert (explicit_tune_core != aarch64_none);
> +  gcc_assert (explicit_arch != aarch64_no_arch);
>  
>    if (aarch64_enable_bti == 2)
>      {
> @@ -18251,15 +18209,6 @@ aarch64_override_options (void)
>    if (aarch64_ra_sign_scope != AARCH64_FUNCTION_NONE && TARGET_ILP32)
>      sorry ("return address signing is only supported for %<-mabi=lp64%>");
>  
> -  /* Make sure we properly set up the explicit options.  */
> -  if ((aarch64_cpu_string && valid_cpu)
> -       || (aarch64_tune_string && valid_tune))
> -    gcc_assert (explicit_tune_core != aarch64_none);
> -
> -  if ((aarch64_cpu_string && valid_cpu)
> -       || (aarch64_arch_string && valid_arch))
> -    gcc_assert (explicit_arch != aarch64_no_arch);
> -
>    /* The pass to insert speculation tracking runs before
>       shrink-wrapping and the latter does not know how to update the
>       tracking status.  So disable it in this case.  */
  

Hi Richard,

> Looks like you might have attached the old patch.  The aarch64_option_restore
> change is mentioned in the changelog but doesn't appear in the patch itself.

Indeed, not sure how that happened. Here is the correct v2 anyway.

Wilco


The --with-cpu/--with-arch configure option processing not only checks valid arguments
but also sets TARGET_CPU_DEFAULT with a CPU and extension bitmask.  This isn't used
however since a --with-cpu is translated into a -mcpu option which is processed as if
written on the command-line (so TARGET_CPU_DEFAULT is never accessed).

So remove all the complex processing and bitmask, and just validate the option.
Fix a bug that always reports valid architecture extensions as invalid.  As a result
the CPU processing in aarch64.c can be simplified.

Bootstrap OK, regress pass, OK for commit?

ChangeLog:
2022-04-19  Wilco Dijkstra  <wdijkstr@arm.com>

        * config.gcc (aarch64*-*-*): Simplify --with-cpu and --with-arch
        processing.  Add support for architectural extensions.
        * config/aarch64/aarch64.h (TARGET_CPU_DEFAULT): Remove
        AARCH64_CPU_DEFAULT_FLAGS.
        (TARGET_CPU_NBITS): Remove.
        (TARGET_CPU_MASK): Remove.
        * config/aarch64/aarch64.cc (AARCH64_CPU_DEFAULT_FLAGS): Remove define.
        (get_tune_cpu): Assert CPU is always valid.
        (get_arch): Assert architecture is always valid.
        (aarch64_override_options): Cleanup CPU selection code and simplify logic.
        (aarch64_option_restore): Remove unnecessary checks on tune.
---

diff --git a/gcc/config.gcc b/gcc/config.gcc
index c5064dd376660c192d5573997b4fc86b6b3e3838..b48d5451e8027c93fb1f614812589183d0a88c4b 100644
--- a/gcc/config.gcc
+++ b/gcc/config.gcc
@@ -4178,8 +4178,6 @@ case "${target}" in
 			  pattern=AARCH64_CORE
 			fi
 
-			ext_mask=AARCH64_CPU_DEFAULT_FLAGS
-
 			# Find the base CPU or ARCH id in aarch64-cores.def or
 			# aarch64-arches.def
 			if [ x"$base_val" = x ] \
@@ -4187,23 +4185,6 @@ case "${target}" in
 				    ${srcdir}/config/aarch64/$def \
 				    > /dev/null; then
 
-			  if [ $which = arch ]; then
-				base_id=`grep "^$pattern(\"$base_val\"," \
-				  ${srcdir}/config/aarch64/$def | \
-				  sed -e 's/^[^,]*,[ 	]*//' | \
-				  sed -e 's/,.*$//'`
-				# Extract the architecture flags from aarch64-arches.def
-				ext_mask=`grep "^$pattern(\"$base_val\"," \
-				   ${srcdir}/config/aarch64/$def | \
-				   sed -e 's/)$//' | \
-				   sed -e 's/^.*,//'`
-			  else
-				base_id=`grep "^$pattern(\"$base_val\"," \
-				  ${srcdir}/config/aarch64/$def | \
-				  sed -e 's/^[^,]*,[ 	]*//' | \
-				  sed -e 's/,.*$//'`
-			  fi
-
 			  # Disallow extensions in --with-tune=cortex-a53+crc.
 			  if [ $which = tune ] && [ x"$ext_val" != x ]; then
 			    echo "Architecture extensions not supported in --with-$which=$val" 1>&2
@@ -4234,25 +4215,7 @@ case "${target}" in
 					grep "^\"$base_ext\""`
 
 				if [ x"$base_ext" = x ] \
-				    || [[ -n $opt_line ]]; then
-
-				  # These regexp extract the elements based on
-				  # their group match index in the regexp.
-				  ext_canon=`echo -e "$opt_line" | \
-					sed -e "s/$sed_patt/\2/"`
-				  ext_on=`echo -e "$opt_line" | \
-					sed -e "s/$sed_patt/\3/"`
-				  ext_off=`echo -e "$opt_line" | \
-					sed -e "s/$sed_patt/\4/"`
-
-				  if [ $ext = $base_ext ]; then
-					# Adding extension
-					ext_mask="("$ext_mask") | ("$ext_on" | "$ext_canon")"
-				  else
-					# Removing extension
-					ext_mask="("$ext_mask") & ~("$ext_off" | "$ext_canon")"
-				  fi
-
+				    || [ x"$opt_line" != x ]; then
 				  true
 				else
 				  echo "Unknown extension used in --with-$which=$val" 1>&2
@@ -4261,10 +4224,6 @@ case "${target}" in
 				ext_val=`echo $ext_val | sed -e 's/[a-z0-9]\+//'`
 			  done
 
-			  ext_mask="(("$ext_mask") << TARGET_CPU_NBITS)"
-			  if [ x"$base_id" != x ]; then
-				target_cpu_cname="TARGET_CPU_$base_id | $ext_mask"
-			  fi
 			  true
 			else
 			  # Allow --with-$which=native.
diff --git a/gcc/config/aarch64/aarch64.h b/gcc/config/aarch64/aarch64.h
index 359b6e8561faa38f53a806b8c114c83ae37e7e17..f835da33b72f36bbf25a0e1328135411bd8ab4f6 100644
--- a/gcc/config/aarch64/aarch64.h
+++ b/gcc/config/aarch64/aarch64.h
@@ -813,16 +813,9 @@ enum target_cpus
   TARGET_CPU_generic
 };
 
-/* Define how many bits are used to represent the CPU in TARGET_CPU_DEFAULT.
-   This needs to be big enough to fit the value of TARGET_CPU_generic.
-   All bits after this are used to represent the AARCH64_CPU_DEFAULT_FLAGS.  */
-#define TARGET_CPU_NBITS 8
-#define TARGET_CPU_MASK ((1 << TARGET_CPU_NBITS) - 1)
-
 /* If there is no CPU defined at configure, use generic as default.  */
 #ifndef TARGET_CPU_DEFAULT
-#define TARGET_CPU_DEFAULT \
-  (TARGET_CPU_generic | (AARCH64_CPU_DEFAULT_FLAGS << TARGET_CPU_NBITS))
+# define TARGET_CPU_DEFAULT TARGET_CPU_generic
 #endif
 
 /* If inserting NOP before a mult-accumulate insn remember to adjust the
diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc
index 1ec15741a4dba055b02732985d5a92a9252b166b..9294de799461f7f94c563f56b02e6e485ab7f1e6 100644
--- a/gcc/config/aarch64/aarch64.cc
+++ b/gcc/config/aarch64/aarch64.cc
@@ -2760,8 +2760,6 @@ static const struct attribute_spec aarch64_attribute_table[] =
   { NULL,                 0, 0, false, false, false, false, NULL, NULL }
 };
 
-#define AARCH64_CPU_DEFAULT_FLAGS ((selected_cpu) ? selected_cpu->flags : 0)
-
 /* An ISA extension in the co-processor and main instruction set space.  */
 struct aarch64_option_extension
 {
@@ -18040,39 +18038,24 @@ aarch64_validate_mtune (const char *str, const struct processor **res)
   return false;
 }
 
-static_assert (TARGET_CPU_generic < TARGET_CPU_MASK,
-	       "TARGET_CPU_NBITS is big enough");
-
-/* Return the CPU corresponding to the enum CPU.
-   If it doesn't specify a cpu, return the default.  */
+/* Return the CPU corresponding to the enum CPU.  */
 
 static const struct processor *
 aarch64_get_tune_cpu (enum aarch64_processor cpu)
 {
-  if (cpu != aarch64_none)
-    return &all_cores[cpu];
+  gcc_assert (cpu != aarch64_none);
 
-  /* The & TARGET_CPU_MASK is to extract the bottom TARGET_CPU_NBITS bits that
-     encode the default cpu as selected by the --with-cpu GCC configure option
-     in config.gcc.
-     ???: The whole TARGET_CPU_DEFAULT and AARCH64_CPU_DEFAULT_FLAGS
-     flags mechanism should be reworked to make it more sane.  */
-  return &all_cores[TARGET_CPU_DEFAULT & TARGET_CPU_MASK];
+  return &all_cores[cpu];
 }
 
-/* Return the architecture corresponding to the enum ARCH.
-   If it doesn't specify a valid architecture, return the default.  */
+/* Return the architecture corresponding to the enum ARCH.  */
 
 static const struct processor *
 aarch64_get_arch (enum aarch64_arch arch)
 {
-  if (arch != aarch64_no_arch)
-    return &all_architectures[arch];
-
-  const struct processor *cpu
-    = &all_cores[TARGET_CPU_DEFAULT & TARGET_CPU_MASK];
+  gcc_assert (arch != aarch64_no_arch);
 
-  return &all_architectures[cpu->arch];
+  return &all_architectures[arch];
 }
 
 /* Return the VG value associated with -msve-vector-bits= value VALUE.  */
@@ -18110,10 +18093,6 @@ aarch64_override_options (void)
   uint64_t arch_isa = 0;
   aarch64_isa_flags = 0;
 
-  bool valid_cpu = true;
-  bool valid_tune = true;
-  bool valid_arch = true;
-
   selected_cpu = NULL;
   selected_arch = NULL;
   selected_tune = NULL;
@@ -18128,77 +18107,56 @@ aarch64_override_options (void)
      If either of -march or -mtune is given, they override their
      respective component of -mcpu.  */
   if (aarch64_cpu_string)
-    valid_cpu = aarch64_validate_mcpu (aarch64_cpu_string, &selected_cpu,
-					&cpu_isa);
+    aarch64_validate_mcpu (aarch64_cpu_string, &selected_cpu, &cpu_isa);
 
   if (aarch64_arch_string)
-    valid_arch = aarch64_validate_march (aarch64_arch_string, &selected_arch,
-					  &arch_isa);
+    aarch64_validate_march (aarch64_arch_string, &selected_arch, &arch_isa);
 
   if (aarch64_tune_string)
-    valid_tune = aarch64_validate_mtune (aarch64_tune_string, &selected_tune);
+    aarch64_validate_mtune (aarch64_tune_string, &selected_tune);
 
 #ifdef SUBTARGET_OVERRIDE_OPTIONS
   SUBTARGET_OVERRIDE_OPTIONS;
 #endif
 
-  /* If the user did not specify a processor, choose the default
-     one for them.  This will be the CPU set during configuration using
-     --with-cpu, otherwise it is "generic".  */
-  if (!selected_cpu)
-    {
-      if (selected_arch)
-	{
-	  selected_cpu = &all_cores[selected_arch->ident];
-	  aarch64_isa_flags = arch_isa;
-	  explicit_arch = selected_arch->arch;
-	}
-      else
-	{
-	  /* Get default configure-time CPU.  */
-	  selected_cpu = aarch64_get_tune_cpu (aarch64_none);
-	  aarch64_isa_flags = TARGET_CPU_DEFAULT >> TARGET_CPU_NBITS;
-	}
-
-      if (selected_tune)
-	explicit_tune_core = selected_tune->ident;
-    }
-  /* If both -mcpu and -march are specified check that they are architecturally
-     compatible, warn if they're not and prefer the -march ISA flags.  */
-  else if (selected_arch)
+  if (selected_cpu && selected_arch)
     {
+      /* If both -mcpu and -march are specified, warn if they are not
+	 architecturally compatible and prefer the -march ISA flags.  */
       if (selected_arch->arch != selected_cpu->arch)
 	{
 	  warning (0, "switch %<-mcpu=%s%> conflicts with %<-march=%s%> switch",
 		       aarch64_cpu_string,
 		       aarch64_arch_string);
 	}
+
       aarch64_isa_flags = arch_isa;
-      explicit_arch = selected_arch->arch;
-      explicit_tune_core = selected_tune ? selected_tune->ident
-					  : selected_cpu->ident;
     }
-  else
+  else if (selected_cpu)
     {
-      /* -mcpu but no -march.  */
-      aarch64_isa_flags = cpu_isa;
-      explicit_tune_core = selected_tune ? selected_tune->ident
-					  : selected_cpu->ident;
-      gcc_assert (selected_cpu);
       selected_arch = &all_architectures[selected_cpu->arch];
-      explicit_arch = selected_arch->arch;
+      aarch64_isa_flags = cpu_isa;
     }
-
-  /* Set the arch as well as we will need it when outputing
-     the .arch directive in assembly.  */
-  if (!selected_arch)
+  else if (selected_arch)
     {
-      gcc_assert (selected_cpu);
+      selected_cpu = &all_cores[selected_arch->ident];
+      aarch64_isa_flags = arch_isa;
+    }
+  else
+    {
+      /* No -mcpu or -march specified, so use the default CPU.  */
+      selected_cpu = &all_cores[TARGET_CPU_DEFAULT];
       selected_arch = &all_architectures[selected_cpu->arch];
+      aarch64_isa_flags = selected_cpu->flags;
     }
 
+  explicit_arch = selected_arch->arch;
   if (!selected_tune)
     selected_tune = selected_cpu;
+  explicit_tune_core = selected_tune->ident;
+
+  gcc_assert (explicit_tune_core != aarch64_none);
+  gcc_assert (explicit_arch != aarch64_no_arch);
 
   if (aarch64_enable_bti == 2)
     {
@@ -18234,15 +18192,6 @@ aarch64_override_options (void)
   if (aarch64_ra_sign_scope != AARCH64_FUNCTION_NONE && TARGET_ILP32)
     sorry ("return address signing is only supported for %<-mabi=lp64%>");
 
-  /* Make sure we properly set up the explicit options.  */
-  if ((aarch64_cpu_string && valid_cpu)
-       || (aarch64_tune_string && valid_tune))
-    gcc_assert (explicit_tune_core != aarch64_none);
-
-  if ((aarch64_cpu_string && valid_cpu)
-       || (aarch64_arch_string && valid_arch))
-    gcc_assert (explicit_arch != aarch64_no_arch);
-
   /* The pass to insert speculation tracking runs before
      shrink-wrapping and the latter does not know how to update the
      tracking status.  So disable it in this case.  */
@@ -18348,11 +18297,7 @@ aarch64_option_restore (struct gcc_options *opts,
   opts->x_explicit_arch = ptr->x_explicit_arch;
   selected_arch = aarch64_get_arch (ptr->x_explicit_arch);
   opts->x_explicit_tune_core = ptr->x_explicit_tune_core;
-  if (opts->x_explicit_tune_core == aarch64_none
-      && opts->x_explicit_arch != aarch64_no_arch)
-    selected_tune = &all_cores[selected_arch->ident];
-  else
-    selected_tune = aarch64_get_tune_cpu (ptr->x_explicit_tune_core);
+  selected_tune = aarch64_get_tune_cpu (ptr->x_explicit_tune_core);
   opts->x_aarch64_override_tune_string = ptr->x_aarch64_override_tune_string;
   opts->x_aarch64_branch_protection_string
     = ptr->x_aarch64_branch_protection_string;
  

Wilco Dijkstra <Wilco.Dijkstra@arm.com> writes:
> Hi Richard,
>
>> Looks like you might have attached the old patch.  The aarch64_option_restore
>> change is mentioned in the changelog but doesn't appear in the patch itself.
>
> Indeed, not sure how that happened. Here is the correct v2 anyway.
>
> Wilco
>
>
> The --with-cpu/--with-arch configure option processing not only checks valid arguments
> but also sets TARGET_CPU_DEFAULT with a CPU and extension bitmask.  This isn't used
> however since a --with-cpu is translated into a -mcpu option which is processed as if
> written on the command-line (so TARGET_CPU_DEFAULT is never accessed).
>
> So remove all the complex processing and bitmask, and just validate the option.
> Fix a bug that always reports valid architecture extensions as invalid.  As a result
> the CPU processing in aarch64.c can be simplified.
>
> Bootstrap OK, regress pass, OK for commit?
>
> ChangeLog:
> 2022-04-19  Wilco Dijkstra  <wdijkstr@arm.com>
>
>         * config.gcc (aarch64*-*-*): Simplify --with-cpu and --with-arch
>         processing.  Add support for architectural extensions.
>         * config/aarch64/aarch64.h (TARGET_CPU_DEFAULT): Remove
>         AARCH64_CPU_DEFAULT_FLAGS.
>         (TARGET_CPU_NBITS): Remove.
>         (TARGET_CPU_MASK): Remove.
>         * config/aarch64/aarch64.cc (AARCH64_CPU_DEFAULT_FLAGS): Remove define.
>         (get_tune_cpu): Assert CPU is always valid.
>         (get_arch): Assert architecture is always valid.
>         (aarch64_override_options): Cleanup CPU selection code and simplify logic.
>         (aarch64_option_restore): Remove unnecessary checks on tune.

OK, thanks.

Richard

> ---
>
> diff --git a/gcc/config.gcc b/gcc/config.gcc
> index c5064dd376660c192d5573997b4fc86b6b3e3838..b48d5451e8027c93fb1f614812589183d0a88c4b 100644
> --- a/gcc/config.gcc
> +++ b/gcc/config.gcc
> @@ -4178,8 +4178,6 @@ case "${target}" in
>                           pattern=AARCH64_CORE
>                         fi
>
> -                       ext_mask=AARCH64_CPU_DEFAULT_FLAGS
> -
>                         # Find the base CPU or ARCH id in aarch64-cores.def or
>                         # aarch64-arches.def
>                         if [ x"$base_val" = x ] \
> @@ -4187,23 +4185,6 @@ case "${target}" in
>                                     ${srcdir}/config/aarch64/$def \
>                                     > /dev/null; then
>
> -                         if [ $which = arch ]; then
> -                               base_id=`grep "^$pattern(\"$base_val\"," \
> -                                 ${srcdir}/config/aarch64/$def | \
> -                                 sed -e 's/^[^,]*,[    ]*//' | \
> -                                 sed -e 's/,.*$//'`
> -                               # Extract the architecture flags from aarch64-arches.def
> -                               ext_mask=`grep "^$pattern(\"$base_val\"," \
> -                                  ${srcdir}/config/aarch64/$def | \
> -                                  sed -e 's/)$//' | \
> -                                  sed -e 's/^.*,//'`
> -                         else
> -                               base_id=`grep "^$pattern(\"$base_val\"," \
> -                                 ${srcdir}/config/aarch64/$def | \
> -                                 sed -e 's/^[^,]*,[    ]*//' | \
> -                                 sed -e 's/,.*$//'`
> -                         fi
> -
>                           # Disallow extensions in --with-tune=cortex-a53+crc.
>                           if [ $which = tune ] && [ x"$ext_val" != x ]; then
>                             echo "Architecture extensions not supported in --with-$which=$val" 1>&2
> @@ -4234,25 +4215,7 @@ case "${target}" in
>                                         grep "^\"$base_ext\""`
>
>                                 if [ x"$base_ext" = x ] \
> -                                   || [[ -n $opt_line ]]; then
> -
> -                                 # These regexp extract the elements based on
> -                                 # their group match index in the regexp.
> -                                 ext_canon=`echo -e "$opt_line" | \
> -                                       sed -e "s/$sed_patt/\2/"`
> -                                 ext_on=`echo -e "$opt_line" | \
> -                                       sed -e "s/$sed_patt/\3/"`
> -                                 ext_off=`echo -e "$opt_line" | \
> -                                       sed -e "s/$sed_patt/\4/"`
> -
> -                                 if [ $ext = $base_ext ]; then
> -                                       # Adding extension
> -                                       ext_mask="("$ext_mask") | ("$ext_on" | "$ext_canon")"
> -                                 else
> -                                       # Removing extension
> -                                       ext_mask="("$ext_mask") & ~("$ext_off" | "$ext_canon")"
> -                                 fi
> -
> +                                   || [ x"$opt_line" != x ]; then
>                                   true
>                                 else
>                                   echo "Unknown extension used in --with-$which=$val" 1>&2
> @@ -4261,10 +4224,6 @@ case "${target}" in
>                                 ext_val=`echo $ext_val | sed -e 's/[a-z0-9]\+//'`
>                           done
>
> -                         ext_mask="(("$ext_mask") << TARGET_CPU_NBITS)"
> -                         if [ x"$base_id" != x ]; then
> -                               target_cpu_cname="TARGET_CPU_$base_id | $ext_mask"
> -                         fi
>                           true
>                         else
>                           # Allow --with-$which=native.
> diff --git a/gcc/config/aarch64/aarch64.h b/gcc/config/aarch64/aarch64.h
> index 359b6e8561faa38f53a806b8c114c83ae37e7e17..f835da33b72f36bbf25a0e1328135411bd8ab4f6 100644
> --- a/gcc/config/aarch64/aarch64.h
> +++ b/gcc/config/aarch64/aarch64.h
> @@ -813,16 +813,9 @@ enum target_cpus
>    TARGET_CPU_generic
>  };
>
> -/* Define how many bits are used to represent the CPU in TARGET_CPU_DEFAULT.
> -   This needs to be big enough to fit the value of TARGET_CPU_generic.
> -   All bits after this are used to represent the AARCH64_CPU_DEFAULT_FLAGS.  */
> -#define TARGET_CPU_NBITS 8
> -#define TARGET_CPU_MASK ((1 << TARGET_CPU_NBITS) - 1)
> -
>  /* If there is no CPU defined at configure, use generic as default.  */
>  #ifndef TARGET_CPU_DEFAULT
> -#define TARGET_CPU_DEFAULT \
> -  (TARGET_CPU_generic | (AARCH64_CPU_DEFAULT_FLAGS << TARGET_CPU_NBITS))
> +# define TARGET_CPU_DEFAULT TARGET_CPU_generic
>  #endif
>
>  /* If inserting NOP before a mult-accumulate insn remember to adjust the
> diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc
> index 1ec15741a4dba055b02732985d5a92a9252b166b..9294de799461f7f94c563f56b02e6e485ab7f1e6 100644
> --- a/gcc/config/aarch64/aarch64.cc
> +++ b/gcc/config/aarch64/aarch64.cc
> @@ -2760,8 +2760,6 @@ static const struct attribute_spec aarch64_attribute_table[] =
>    { NULL,                 0, 0, false, false, false, false, NULL, NULL }
>  };
>
> -#define AARCH64_CPU_DEFAULT_FLAGS ((selected_cpu) ? selected_cpu->flags : 0)
> -
>  /* An ISA extension in the co-processor and main instruction set space.  */
>  struct aarch64_option_extension
>  {
> @@ -18040,39 +18038,24 @@ aarch64_validate_mtune (const char *str, const struct processor **res)
>    return false;
>  }
>
> -static_assert (TARGET_CPU_generic < TARGET_CPU_MASK,
> -              "TARGET_CPU_NBITS is big enough");
> -
> -/* Return the CPU corresponding to the enum CPU.
> -   If it doesn't specify a cpu, return the default.  */
> +/* Return the CPU corresponding to the enum CPU.  */
>
>  static const struct processor *
>  aarch64_get_tune_cpu (enum aarch64_processor cpu)
>  {
> -  if (cpu != aarch64_none)
> -    return &all_cores[cpu];
> +  gcc_assert (cpu != aarch64_none);
>
> -  /* The & TARGET_CPU_MASK is to extract the bottom TARGET_CPU_NBITS bits that
> -     encode the default cpu as selected by the --with-cpu GCC configure option
> -     in config.gcc.
> -     ???: The whole TARGET_CPU_DEFAULT and AARCH64_CPU_DEFAULT_FLAGS
> -     flags mechanism should be reworked to make it more sane.  */
> -  return &all_cores[TARGET_CPU_DEFAULT & TARGET_CPU_MASK];
> +  return &all_cores[cpu];
>  }
>
> -/* Return the architecture corresponding to the enum ARCH.
> -   If it doesn't specify a valid architecture, return the default.  */
> +/* Return the architecture corresponding to the enum ARCH.  */
>
>  static const struct processor *
>  aarch64_get_arch (enum aarch64_arch arch)
>  {
> -  if (arch != aarch64_no_arch)
> -    return &all_architectures[arch];
> -
> -  const struct processor *cpu
> -    = &all_cores[TARGET_CPU_DEFAULT & TARGET_CPU_MASK];
> +  gcc_assert (arch != aarch64_no_arch);
>
> -  return &all_architectures[cpu->arch];
> +  return &all_architectures[arch];
>  }
>
>  /* Return the VG value associated with -msve-vector-bits= value VALUE.  */
> @@ -18110,10 +18093,6 @@ aarch64_override_options (void)
>    uint64_t arch_isa = 0;
>    aarch64_isa_flags = 0;
>
> -  bool valid_cpu = true;
> -  bool valid_tune = true;
> -  bool valid_arch = true;
> -
>    selected_cpu = NULL;
>    selected_arch = NULL;
>    selected_tune = NULL;
> @@ -18128,77 +18107,56 @@ aarch64_override_options (void)
>       If either of -march or -mtune is given, they override their
>       respective component of -mcpu.  */
>    if (aarch64_cpu_string)
> -    valid_cpu = aarch64_validate_mcpu (aarch64_cpu_string, &selected_cpu,
> -                                       &cpu_isa);
> +    aarch64_validate_mcpu (aarch64_cpu_string, &selected_cpu, &cpu_isa);
>
>    if (aarch64_arch_string)
> -    valid_arch = aarch64_validate_march (aarch64_arch_string, &selected_arch,
> -                                         &arch_isa);
> +    aarch64_validate_march (aarch64_arch_string, &selected_arch, &arch_isa);
>
>    if (aarch64_tune_string)
> -    valid_tune = aarch64_validate_mtune (aarch64_tune_string, &selected_tune);
> +    aarch64_validate_mtune (aarch64_tune_string, &selected_tune);
>
>  #ifdef SUBTARGET_OVERRIDE_OPTIONS
>    SUBTARGET_OVERRIDE_OPTIONS;
>  #endif
>
> -  /* If the user did not specify a processor, choose the default
> -     one for them.  This will be the CPU set during configuration using
> -     --with-cpu, otherwise it is "generic".  */
> -  if (!selected_cpu)
> -    {
> -      if (selected_arch)
> -       {
> -         selected_cpu = &all_cores[selected_arch->ident];
> -         aarch64_isa_flags = arch_isa;
> -         explicit_arch = selected_arch->arch;
> -       }
> -      else
> -       {
> -         /* Get default configure-time CPU.  */
> -         selected_cpu = aarch64_get_tune_cpu (aarch64_none);
> -         aarch64_isa_flags = TARGET_CPU_DEFAULT >> TARGET_CPU_NBITS;
> -       }
> -
> -      if (selected_tune)
> -       explicit_tune_core = selected_tune->ident;
> -    }
> -  /* If both -mcpu and -march are specified check that they are architecturally
> -     compatible, warn if they're not and prefer the -march ISA flags.  */
> -  else if (selected_arch)
> +  if (selected_cpu && selected_arch)
>      {
> +      /* If both -mcpu and -march are specified, warn if they are not
> +        architecturally compatible and prefer the -march ISA flags.  */
>        if (selected_arch->arch != selected_cpu->arch)
>         {
>           warning (0, "switch %<-mcpu=%s%> conflicts with %<-march=%s%> switch",
>                        aarch64_cpu_string,
>                        aarch64_arch_string);
>         }
> +
>        aarch64_isa_flags = arch_isa;
> -      explicit_arch = selected_arch->arch;
> -      explicit_tune_core = selected_tune ? selected_tune->ident
> -                                         : selected_cpu->ident;
>      }
> -  else
> +  else if (selected_cpu)
>      {
> -      /* -mcpu but no -march.  */
> -      aarch64_isa_flags = cpu_isa;
> -      explicit_tune_core = selected_tune ? selected_tune->ident
> -                                         : selected_cpu->ident;
> -      gcc_assert (selected_cpu);
>        selected_arch = &all_architectures[selected_cpu->arch];
> -      explicit_arch = selected_arch->arch;
> +      aarch64_isa_flags = cpu_isa;
>      }
> -
> -  /* Set the arch as well as we will need it when outputing
> -     the .arch directive in assembly.  */
> -  if (!selected_arch)
> +  else if (selected_arch)
>      {
> -      gcc_assert (selected_cpu);
> +      selected_cpu = &all_cores[selected_arch->ident];
> +      aarch64_isa_flags = arch_isa;
> +    }
> +  else
> +    {
> +      /* No -mcpu or -march specified, so use the default CPU.  */
> +      selected_cpu = &all_cores[TARGET_CPU_DEFAULT];
>        selected_arch = &all_architectures[selected_cpu->arch];
> +      aarch64_isa_flags = selected_cpu->flags;
>      }
>
> +  explicit_arch = selected_arch->arch;
>    if (!selected_tune)
>      selected_tune = selected_cpu;
> +  explicit_tune_core = selected_tune->ident;
> +
> +  gcc_assert (explicit_tune_core != aarch64_none);
> +  gcc_assert (explicit_arch != aarch64_no_arch);
>
>    if (aarch64_enable_bti == 2)
>      {
> @@ -18234,15 +18192,6 @@ aarch64_override_options (void)
>    if (aarch64_ra_sign_scope != AARCH64_FUNCTION_NONE && TARGET_ILP32)
>      sorry ("return address signing is only supported for %<-mabi=lp64%>");
>
> -  /* Make sure we properly set up the explicit options.  */
> -  if ((aarch64_cpu_string && valid_cpu)
> -       || (aarch64_tune_string && valid_tune))
> -    gcc_assert (explicit_tune_core != aarch64_none);
> -
> -  if ((aarch64_cpu_string && valid_cpu)
> -       || (aarch64_arch_string && valid_arch))
> -    gcc_assert (explicit_arch != aarch64_no_arch);
> -
>    /* The pass to insert speculation tracking runs before
>       shrink-wrapping and the latter does not know how to update the
>       tracking status.  So disable it in this case.  */
> @@ -18348,11 +18297,7 @@ aarch64_option_restore (struct gcc_options *opts,
>    opts->x_explicit_arch = ptr->x_explicit_arch;
>    selected_arch = aarch64_get_arch (ptr->x_explicit_arch);
>    opts->x_explicit_tune_core = ptr->x_explicit_tune_core;
> -  if (opts->x_explicit_tune_core == aarch64_none
> -      && opts->x_explicit_arch != aarch64_no_arch)
> -    selected_tune = &all_cores[selected_arch->ident];
> -  else
> -    selected_tune = aarch64_get_tune_cpu (ptr->x_explicit_tune_core);
> +  selected_tune = aarch64_get_tune_cpu (ptr->x_explicit_tune_core);
>    opts->x_aarch64_override_tune_string = ptr->x_aarch64_override_tune_string;
>    opts->x_aarch64_branch_protection_string
>      = ptr->x_aarch64_branch_protection_string;