[AArch64] Fix command line options canonicalization version #2. (PR target/88530)

Hi All,

Commandline options on AArch64 don't get canonicalized into the smallest
possible set before output to the assembler. This means that overlapping feature
sets are emitted with superfluous parts.

Normally this isn't an issue, but in the case of crypto we have retro-actively
split it into aes and sha2. We need to emit only +crypto to the assembler
so old assemblers continue to work.

Because of how -mcpu=native and -march=native work they end up enabling all
feature bits. Instead we need to get the smallest possible set, which would also
fix the problem with older the assemblers and the retro-active split.

The function that handles this is called quite often.  It is called for any
push/pop options or attribute that changes arch, cpu etc.  In order to not make
this search for the smallest set too expensive we sort the options based on the
number of features (bits) they enable.  This allows us to process the list
linearly instead of quadratically (Once we have enabled a feature, we know that
anything else that enables it can be ignored.  By sorting we'll get the biggest
groups first and thus the smallest combination of commandline flags).

The Option handling structures have been extended to have a boolean to indicate
whether the option is synthetic, with that I mean if the option flag itself
enables a new feature.

e.g. +crypto isn't an actual feature, it just enables other features, but others
like +rdma enable multiple dependent features but is itself also a feature.

There are two ways to solve this.

1) Either have the options that are feature bits also turn themselves on, e.g.
   change rdma to turn on FP, SIMD and RDMA as dependency bits.

2) Make a distinction between these two different type of features and have the
   framework handle it correctly.

Even though it's more code I went for the second approach, as it's the one
that'll be less fragile (people can't forget it) and gives the least surprises.

Effectively this patch changes the following:

The values before the => are the old compiler and after the => the new code.

-march=armv8.2-a+crypto+sha2 => -march=armv8.2-a+crypto
-march=armv8.2-a+sha2+aes => -march=armv8.2-a+crypto

The remaining behaviors stay the same.

Bootstrapped Regtested on aarch64-none-linux-gnu and no issues.

Ok for trunk?

Thanks,
Tamar

gcc/ChangeLog:

2019-02-20  Tamar Christina  <tamar.christina@arm.com>

	PR target/88530
	* common/config/aarch64/aarch64-common.c
	(struct aarch64_option_extension): Add is_synthetic.
	(all_extensions): Use it.
	(TARGET_OPTION_INIT_STRUCT): Define hook.
	(struct gcc_targetm_common): Moved to end.
	(all_extensions_by_on): New.
	(opt_ext_cmp, typedef opt_ext): New.
	(aarch64_option_init_struct): New.
	(aarch64_contains_opt): New.
	(aarch64_get_extension_string_for_isa_flags): Output smallest set.
	* config/aarch64/aarch64-option-extensions.def
	(AARCH64_OPT_EXTENSION): Explicitly include AES and SHA2 in crypto.
	(fp, simd, crc, lse, fp16, rcpc, rdma, dotprod, aes, sha2, sha3,
	sm4, fp16fml, sve, profile, rng, memtag, sb, ssbs, predres):
	Set is_synthetic to false.
	(crypto): Set is_synthetic to true.
	* config/aarch64/driver-aarch64.c (AARCH64_OPT_EXTENSION): Add
	SYNTHETIC.

gcc/testsuite/ChangeLog:

2019-02-20  Tamar Christina  <tamar.christina@arm.com>

	PR target/88530
	* gcc.target/aarch64/options_set_1.c: New test.
	* gcc.target/aarch64/options_set_2.c: New test.
	* gcc.target/aarch64/options_set_3.c: New test.
	* gcc.target/aarch64/options_set_4.c: New test.
	* gcc.target/aarch64/options_set_5.c: New test.
	* gcc.target/aarch64/options_set_6.c: New test.
	* gcc.target/aarch64/options_set_7.c: New test.
	* gcc.target/aarch64/options_set_8.c: New test.
	* gcc.target/aarch64/options_set_9.c: New test.

--

On Wed, Feb 20, 2019 at 08:00:38AM -0600, Tamar Christina wrote:
> Hi All,
> 
> Commandline options on AArch64 don't get canonicalized into the smallest
> possible set before output to the assembler. This means that overlapping feature
> sets are emitted with superfluous parts.
> 
> Normally this isn't an issue, but in the case of crypto we have retro-actively
> split it into aes and sha2. We need to emit only +crypto to the assembler
> so old assemblers continue to work.
> 
> Because of how -mcpu=native and -march=native work they end up enabling all
> feature bits. Instead we need to get the smallest possible set, which would also
> fix the problem with older the assemblers and the retro-active split.
> 
> The function that handles this is called quite often.  It is called for any
> push/pop options or attribute that changes arch, cpu etc.  In order to not make
> this search for the smallest set too expensive we sort the options based on the
> number of features (bits) they enable.  This allows us to process the list
> linearly instead of quadratically (Once we have enabled a feature, we know that
> anything else that enables it can be ignored.  By sorting we'll get the biggest
> groups first and thus the smallest combination of commandline flags).
> 
> The Option handling structures have been extended to have a boolean to indicate
> whether the option is synthetic, with that I mean if the option flag itself
> enables a new feature.
> 
> e.g. +crypto isn't an actual feature, it just enables other features, but others
> like +rdma enable multiple dependent features but is itself also a feature.
> 
> There are two ways to solve this.
> 
> 1) Either have the options that are feature bits also turn themselves on, e.g.
>    change rdma to turn on FP, SIMD and RDMA as dependency bits.
> 
> 2) Make a distinction between these two different type of features and have the
>    framework handle it correctly.
> 
> Even though it's more code I went for the second approach, as it's the one
> that'll be less fragile (people can't forget it) and gives the least surprises.
> 
> Effectively this patch changes the following:
> 
> The values before the => are the old compiler and after the => the new code.
> 
> -march=armv8.2-a+crypto+sha2 => -march=armv8.2-a+crypto
> -march=armv8.2-a+sha2+aes => -march=armv8.2-a+crypto
> 
> The remaining behaviors stay the same.
> 
> Bootstrapped Regtested on aarch64-none-linux-gnu and no issues.
> 
> Ok for trunk?

OK, but I don't understand why the CRC special case is needed. My copy of
the Arm Architecture Reference Manual suggests that all versions of the
architceture from ARmv8.1-A are required to implement the CRC32 extension.
Is there some old assembler that doesn't honour that? Whatever is driving
that requirement could usefully be added to the comments.

I find it very hard to believe that this code is what we need for correct
behaviour on AArch64; this level of complexity implies we're doing something
very wrong with either the definition or the implementation of these features
bits which makes it hard for us to maintain, and hard for users and dependent
tools (e.g. assemblers) to know what to expect from the compiler.

I've seen a number of bugs in this code recently. While I appreciate your
patch for fixing one of them, I find the cases and expectations so hard
to reason about that I can't say I am sure we are now bug free.

OK for trunk as an improvement over today, and to help us get towards a
release; but I'm very unhappy with this corner of the compiler!

Thanks,
James

> gcc/ChangeLog:
> 
> 2019-02-20  Tamar Christina  <tamar.christina@arm.com>
> 
> 	PR target/88530
> 	* common/config/aarch64/aarch64-common.c
> 	(struct aarch64_option_extension): Add is_synthetic.
> 	(all_extensions): Use it.
> 	(TARGET_OPTION_INIT_STRUCT): Define hook.
> 	(struct gcc_targetm_common): Moved to end.
> 	(all_extensions_by_on): New.
> 	(opt_ext_cmp, typedef opt_ext): New.
> 	(aarch64_option_init_struct): New.
> 	(aarch64_contains_opt): New.
> 	(aarch64_get_extension_string_for_isa_flags): Output smallest set.
> 	* config/aarch64/aarch64-option-extensions.def
> 	(AARCH64_OPT_EXTENSION): Explicitly include AES and SHA2 in crypto.
> 	(fp, simd, crc, lse, fp16, rcpc, rdma, dotprod, aes, sha2, sha3,
> 	sm4, fp16fml, sve, profile, rng, memtag, sb, ssbs, predres):
> 	Set is_synthetic to false.
> 	(crypto): Set is_synthetic to true.
> 	* config/aarch64/driver-aarch64.c (AARCH64_OPT_EXTENSION): Add
> 	SYNTHETIC.
> 
> gcc/testsuite/ChangeLog:
> 
> 2019-02-20  Tamar Christina  <tamar.christina@arm.com>
> 
> 	PR target/88530
> 	* gcc.target/aarch64/options_set_1.c: New test.
> 	* gcc.target/aarch64/options_set_2.c: New test.
> 	* gcc.target/aarch64/options_set_3.c: New test.
> 	* gcc.target/aarch64/options_set_4.c: New test.
> 	* gcc.target/aarch64/options_set_5.c: New test.
> 	* gcc.target/aarch64/options_set_6.c: New test.
> 	* gcc.target/aarch64/options_set_7.c: New test.
> 	* gcc.target/aarch64/options_set_8.c: New test.
> 	* gcc.target/aarch64/options_set_9.c: New test.
> 
> --

The 02/21/2019 22:34, James Greenhalgh wrote:
> On Wed, Feb 20, 2019 at 08:00:38AM -0600, Tamar Christina wrote:
> > Hi All,
> > 
> > Commandline options on AArch64 don't get canonicalized into the smallest
> > possible set before output to the assembler. This means that overlapping feature
> > sets are emitted with superfluous parts.
> > 
> > Normally this isn't an issue, but in the case of crypto we have retro-actively
> > split it into aes and sha2. We need to emit only +crypto to the assembler
> > so old assemblers continue to work.
> > 
> > Because of how -mcpu=native and -march=native work they end up enabling all
> > feature bits. Instead we need to get the smallest possible set, which would also
> > fix the problem with older the assemblers and the retro-active split.
> > 
> > The function that handles this is called quite often.  It is called for any
> > push/pop options or attribute that changes arch, cpu etc.  In order to not make
> > this search for the smallest set too expensive we sort the options based on the
> > number of features (bits) they enable.  This allows us to process the list
> > linearly instead of quadratically (Once we have enabled a feature, we know that
> > anything else that enables it can be ignored.  By sorting we'll get the biggest
> > groups first and thus the smallest combination of commandline flags).
> > 
> > The Option handling structures have been extended to have a boolean to indicate
> > whether the option is synthetic, with that I mean if the option flag itself
> > enables a new feature.
> > 
> > e.g. +crypto isn't an actual feature, it just enables other features, but others
> > like +rdma enable multiple dependent features but is itself also a feature.
> > 
> > There are two ways to solve this.
> > 
> > 1) Either have the options that are feature bits also turn themselves on, e.g.
> >    change rdma to turn on FP, SIMD and RDMA as dependency bits.
> > 
> > 2) Make a distinction between these two different type of features and have the
> >    framework handle it correctly.
> > 
> > Even though it's more code I went for the second approach, as it's the one
> > that'll be less fragile (people can't forget it) and gives the least surprises.
> > 
> > Effectively this patch changes the following:
> > 
> > The values before the => are the old compiler and after the => the new code.
> > 
> > -march=armv8.2-a+crypto+sha2 => -march=armv8.2-a+crypto
> > -march=armv8.2-a+sha2+aes => -march=armv8.2-a+crypto
> > 
> > The remaining behaviors stay the same.
> > 
> > Bootstrapped Regtested on aarch64-none-linux-gnu and no issues.
> > 
> > Ok for trunk?
> 
> OK, but I don't understand why the CRC special case is needed. My copy of
> the Arm Architecture Reference Manual suggests that all versions of the
> architceture from ARmv8.1-A are required to implement the CRC32 extension.
> Is there some old assembler that doesn't honour that? Whatever is driving
> that requirement could usefully be added to the comments.

From my understanding it's because of a bug in an older assembler.  I am not
sure really what our support Window for this kind of stuff is to be honest.
But probably somewhere, someone will still be using the latest GCC with a
very old binutils.

I have updated the patch with a comment to this effect.  I have also made a
small change in splitting the loop that emits +<opt> and +no<opt> into two
different ones.  No change aside from the copied "for..".  The reason for this
is that the assemblers expect all + before +no, as a final full toolchain
regtest pointed out.

I have committed this as trivial as no change was done to the working of the
options loop. And also added a comment to this requirement in the code.

Thanks,
Tamar

> 
> I find it very hard to believe that this code is what we need for correct
> behaviour on AArch64; this level of complexity implies we're doing something
> very wrong with either the definition or the implementation of these features
> bits which makes it hard for us to maintain, and hard for users and dependent
> tools (e.g. assemblers) to know what to expect from the compiler.
> 
> I've seen a number of bugs in this code recently. While I appreciate your
> patch for fixing one of them, I find the cases and expectations so hard
> to reason about that I can't say I am sure we are now bug free.
> 
> OK for trunk as an improvement over today, and to help us get towards a
> release; but I'm very unhappy with this corner of the compiler!
> 
> Thanks,
> James
> 
> > gcc/ChangeLog:
> > 
> > 2019-02-20  Tamar Christina  <tamar.christina@arm.com>
> > 
> > 	PR target/88530
> > 	* common/config/aarch64/aarch64-common.c
> > 	(struct aarch64_option_extension): Add is_synthetic.
> > 	(all_extensions): Use it.
> > 	(TARGET_OPTION_INIT_STRUCT): Define hook.
> > 	(struct gcc_targetm_common): Moved to end.
> > 	(all_extensions_by_on): New.
> > 	(opt_ext_cmp, typedef opt_ext): New.
> > 	(aarch64_option_init_struct): New.
> > 	(aarch64_contains_opt): New.
> > 	(aarch64_get_extension_string_for_isa_flags): Output smallest set.
> > 	* config/aarch64/aarch64-option-extensions.def
> > 	(AARCH64_OPT_EXTENSION): Explicitly include AES and SHA2 in crypto.
> > 	(fp, simd, crc, lse, fp16, rcpc, rdma, dotprod, aes, sha2, sha3,
> > 	sm4, fp16fml, sve, profile, rng, memtag, sb, ssbs, predres):
> > 	Set is_synthetic to false.
> > 	(crypto): Set is_synthetic to true.
> > 	* config/aarch64/driver-aarch64.c (AARCH64_OPT_EXTENSION): Add
> > 	SYNTHETIC.
> > 
> > gcc/testsuite/ChangeLog:
> > 
> > 2019-02-20  Tamar Christina  <tamar.christina@arm.com>
> > 
> > 	PR target/88530
> > 	* gcc.target/aarch64/options_set_1.c: New test.
> > 	* gcc.target/aarch64/options_set_2.c: New test.
> > 	* gcc.target/aarch64/options_set_3.c: New test.
> > 	* gcc.target/aarch64/options_set_4.c: New test.
> > 	* gcc.target/aarch64/options_set_5.c: New test.
> > 	* gcc.target/aarch64/options_set_6.c: New test.
> > 	* gcc.target/aarch64/options_set_7.c: New test.
> > 	* gcc.target/aarch64/options_set_8.c: New test.
> > 	* gcc.target/aarch64/options_set_9.c: New test.
> > 
> > -- 
> 

--
diff --git a/gcc/common/config/aarch64/aarch64-common.c b/gcc/common/config/aarch64/aarch64-common.c
index fd870e187a6634b929bc058f99e768e829008179..5329471dc574b745c50c5a9b1000e749157b87f4 100644
--- a/gcc/common/config/aarch64/aarch64-common.c
+++ b/gcc/common/config/aarch64/aarch64-common.c
@@ -46,6 +46,8 @@
 #define TARGET_OPTION_DEFAULT_PARAMS aarch64_option_default_params
 #undef TARGET_OPTION_VALIDATE_PARAM
 #define TARGET_OPTION_VALIDATE_PARAM aarch64_option_validate_param
+#undef TARGET_OPTION_INIT_STRUCT
+#define TARGET_OPTION_INIT_STRUCT aarch64_option_init_struct
 
 /* Set default optimization options.  */
 static const struct default_options aarch_option_optimization_table[] =
@@ -164,8 +166,6 @@ aarch64_handle_option (struct gcc_options *opts,
     }
 }
 
-struct gcc_targetm_common targetm_common = TARGETM_COMMON_INITIALIZER;
-
 /* An ISA extension in the co-processor and main instruction set space.  */
 struct aarch64_option_extension
 {
@@ -173,15 +173,28 @@ struct aarch64_option_extension
   const unsigned long flag_canonical;
   const unsigned long flags_on;
   const unsigned long flags_off;
+  const bool is_synthetic;
 };
 
 /* ISA extensions in AArch64.  */
 static const struct aarch64_option_extension all_extensions[] =
 {
-#define AARCH64_OPT_EXTENSION(NAME, FLAG_CANONICAL, FLAGS_ON, FLAGS_OFF, Z) \
-  {NAME, FLAG_CANONICAL, FLAGS_ON, FLAGS_OFF},
+#define AARCH64_OPT_EXTENSION(NAME, FLAG_CANONICAL, FLAGS_ON, FLAGS_OFF, \
+			      SYNTHETIC, Z) \
+  {NAME, FLAG_CANONICAL, FLAGS_ON, FLAGS_OFF, SYNTHETIC},
+#include "config/aarch64/aarch64-option-extensions.def"
+  {NULL, 0, 0, 0, false}
+};
+
+/* A copy of the ISA extensions list for AArch64 sorted by the popcount of
+   bits and extension turned on.  Cached for efficiency.  */
+static struct aarch64_option_extension all_extensions_by_on[] =
+{
+#define AARCH64_OPT_EXTENSION(NAME, FLAG_CANONICAL, FLAGS_ON, FLAGS_OFF, \
+			      SYNTHETIC, Z) \
+  {NAME, FLAG_CANONICAL, FLAGS_ON, FLAGS_OFF, SYNTHETIC},
 #include "config/aarch64/aarch64-option-extensions.def"
-  {NULL, 0, 0, 0}
+  {NULL, 0, 0, 0, false}
 };
 
 struct processor_name_to_arch
@@ -298,6 +311,76 @@ aarch64_get_all_extension_candidates (auto_vec<const char *> *candidates)
     candidates->safe_push (opt->name);
 }
 
+/* Comparer to sort aarch64's feature extensions by population count. Largest
+   first.  */
+
+typedef const struct aarch64_option_extension opt_ext;
+
+int opt_ext_cmp (const void* a, const void* b)
+{
+  opt_ext *opt_a = (opt_ext *)a;
+  opt_ext *opt_b = (opt_ext *)b;
+
+  /* We consider the total set of bits an options turns on to be the union of
+     the singleton set containing the option itself and the set of options it
+     turns on as a dependency.  As an example +dotprod turns on FL_DOTPROD and
+     FL_SIMD.  As such the set of bits represented by this option is
+     {FL_DOTPROD, FL_SIMD}. */
+  unsigned long total_flags_a = opt_a->flag_canonical & opt_a->flags_on;
+  unsigned long total_flags_b = opt_b->flag_canonical & opt_b->flags_on;
+  int popcnt_a = popcount_hwi ((HOST_WIDE_INT)total_flags_a);
+  int popcnt_b = popcount_hwi ((HOST_WIDE_INT)total_flags_b);
+  int order = popcnt_b - popcnt_a;
+
+  /* If they have the same amount of bits set, give it a more
+     deterministic ordering by using the value of the bits themselves.  */
+  if (order == 0)
+    return total_flags_b - total_flags_a;
+
+  return order;
+}
+
+/* Implement TARGET_OPTION_INIT_STRUCT.  */
+
+static void
+aarch64_option_init_struct (struct gcc_options *opts ATTRIBUTE_UNUSED)
+{
+    /* Sort the extensions based on how many bits they set, order the larger
+       counts first.  We sort the list because this makes processing the
+       feature bits O(n) instead of O(n^2).  While n is small, the function
+       to calculate the feature strings is called on every options push,
+       pop and attribute change (arm_neon headers, lto etc all cause this to
+       happen quite frequently).  It is a trade-off between time and space and
+       so time won.  */
+    int n_extensions
+      = sizeof (all_extensions) / sizeof (struct aarch64_option_extension);
+    qsort (&all_extensions_by_on, n_extensions,
+	   sizeof (struct aarch64_option_extension), opt_ext_cmp);
+}
+
+/* Checks to see if enough bits from the option OPT are enabled in
+   ISA_FLAG_BITS to be able to replace the individual options with the
+   canonicalized version of the option.  This is done based on two rules:
+
+   1) Synthetic groups, such as +crypto we only care about the bits that are
+      turned on. e.g. +aes+sha2 can be replaced with +crypto.
+
+   2) Options that themselves have a bit, such as +rdma, in this case, all the
+      feature bits they turn on must be available and the bit for the option
+      itself must be.  In this case it's effectively a reduction rather than a
+      grouping. e.g. +fp+simd is not enough to turn on +rdma, for that you would
+      need +rdma+fp+simd which is reduced down to +rdma.
+*/
+
+static bool
+aarch64_contains_opt (unsigned long isa_flag_bits, opt_ext *opt)
+{
+  unsigned long flags_check
+    = opt->is_synthetic ? opt->flags_on : opt->flag_canonical;
+
+  return (isa_flag_bits & flags_check) == flags_check;
+}
+
 /* Return a string representation of ISA_FLAGS.  DEFAULT_ARCH_FLAGS
    gives the default set of flags which are implied by whatever -march
    we'd put out.  Our job is to figure out the minimal set of "+" and
@@ -311,26 +394,97 @@ aarch64_get_extension_string_for_isa_flags (unsigned long isa_flags,
   const struct aarch64_option_extension *opt = NULL;
   std::string outstr = "";
 
-  /* Pass one: Find all the things we need to turn on.  As a special case,
-     we always want to put out +crc if it is enabled.  */
-  for (opt = all_extensions; opt->name != NULL; opt++)
-    if ((isa_flags & opt->flag_canonical
-	 && !(default_arch_flags & opt->flag_canonical))
-	|| (default_arch_flags & opt->flag_canonical
-            && opt->flag_canonical == AARCH64_ISA_CRC))
-      {
-	outstr += "+";
-	outstr += opt->name;
-      }
+  unsigned long isa_flag_bits = isa_flags;
 
-  /* Pass two: Find all the things we need to turn off.  */
-  for (opt = all_extensions; opt->name != NULL; opt++)
-    if ((~isa_flags) & opt->flag_canonical
-	&& !((~default_arch_flags) & opt->flag_canonical))
+  /* Pass one: Minimize the search space by reducing the set of options
+     to the smallest set that still turns on the same features as before in
+     conjunction with the bits that are turned on by default for the selected
+     architecture.  */
+  for (opt = all_extensions_by_on; opt->name != NULL; opt++)
+    {
+      /* If the bit is on by default, then all the options it turns on are also
+	 on by default due to the transitive dependencies.
+
+         If the option is enabled explicitly in the set then we need to emit
+	 an option for it.  Since this list is sorted by extensions setting the
+	 largest number of featers first, we can be sure that nothing else will
+	 ever need to set the bits we already set.  Consider the following
+	 situation:
+
+	  Feat1 = A + B + C
+	  Feat2 = A + B
+	  Feat3 = A + D
+	  Feat4 = B + C
+	  Feat5 = C
+
+	The following results are expected:
+
+	  A + C = A + Feat5
+	  B + C = Feat4
+	  Feat4 + A = Feat1
+	  Feat2 + Feat5 = Feat1
+	  Feat1 + C = Feat1
+          Feat3 + Feat4 = Feat1 + D
+
+	This search assumes that all invidual feature bits are use visible,
+	in other words the user must be able to do +A, +B, +C and +D.  */
+      if (aarch64_contains_opt (isa_flag_bits | default_arch_flags, opt))
       {
-	outstr += "+no";
-	outstr += opt->name;
+	/* We remove all the dependent bits, to prevent them from being turned
+	   on twice.  This only works because we assume that all there are
+	   individual options to set all bits standalone.  */
+	isa_flag_bits &= ~opt->flags_on;
+	isa_flag_bits |= opt->flag_canonical;
       }
+    }
+
+   /* By toggling bits on and off, we may have set bits on that are already
+      enabled by default.  So we mask the default set out so we don't emit an
+      option for them.  Instead of checking for this each time during Pass One
+      we just mask all default bits away at the end.  */
+   isa_flag_bits &= ~default_arch_flags;
+
+   /* We now have the smallest set of features we need to process.  A subsequent
+      linear scan of the bits in isa_flag_bits will allow us to print the ext
+      names.  However as a special case if CRC was enabled before, always print
+      it.  This is required because some CPUs have an incorrect specification
+      in older assemblers.  Even though CRC should be the default for these
+      cases the -mcpu values won't turn it on.  */
+  if (isa_flags & AARCH64_ISA_CRC)
+    isa_flag_bits |= AARCH64_ISA_CRC;
+
+  /* Pass Two:
+     Print the option names that we're sure we must turn on.  These are only
+     optional extension names.  Mandatory ones have already been removed and
+     ones we explicitly want off have been too.  */
+  for (opt = all_extensions_by_on; opt->name != NULL; opt++)
+    {
+      if (isa_flag_bits & opt->flag_canonical)
+	{
+	  outstr += "+";
+	  outstr += opt->name;
+	}
+    }
+
+  /* Pass Three:
+     Print out a +no for any mandatory extension that we are
+     turning off.  By this point aarch64_parse_extension would have ensured
+     that any optional extensions are turned off.  The only things left are
+     things that can't be turned off usually, e.g. something that is on by
+     default because it's mandatory and we want it off.  For turning off bits
+     we don't guarantee the smallest set of flags, but instead just emit all
+     options the user has specified.
+
+     The assembler requires all +<opts> to be printed before +no<opts>.  */
+  for (opt = all_extensions_by_on; opt->name != NULL; opt++)
+    {
+      if ((~isa_flags) & opt->flag_canonical
+		&& !((~default_arch_flags) & opt->flag_canonical))
+	{
+	  outstr += "+no";
+	  outstr += opt->name;
+	}
+    }
 
   return outstr;
 }
@@ -411,5 +565,7 @@ aarch64_rewrite_mcpu (int argc, const char **argv)
   return aarch64_rewrite_selected_cpu (argv[argc - 1]);
 }
 
+struct gcc_targetm_common targetm_common = TARGETM_COMMON_INITIALIZER;
+
 #undef AARCH64_CPU_NAME_LENGTH
 
diff --git a/gcc/config/aarch64/aarch64-option-extensions.def b/gcc/config/aarch64/aarch64-option-extensions.def
index 2879e35cf2d41d96cb41bb3edd82c0f50091b077..1b2f4abbd5b850135af2cb7010921b45c03516a9 100644
--- a/gcc/config/aarch64/aarch64-option-extensions.def
+++ b/gcc/config/aarch64/aarch64-option-extensions.def
@@ -21,29 +21,38 @@
 
    Before using #include to read this file, define a macro:
 
-      AARCH64_OPT_EXTENSION(EXT_NAME, FLAG_CANONICAL, FLAGS_ON, FLAGS_OFF, FEATURE_STRING)
-
-   EXT_NAME is the name of the extension, represented as a string constant.
-   FLAGS_CANONICAL is the canonical internal name for this flag.
-   FLAGS_ON are the bitwise-or of the features that enabling the extension
-   adds, or zero if enabling this extension has no effect on other features.
-   FLAGS_OFF are the bitwise-or of the features that disabling the extension
-   removes, or zero if disabling this extension has no effect on other
-   features.
-   FEAT_STRING is a string containing the entries in the 'Features' field of
-   /proc/cpuinfo on a GNU/Linux system that correspond to this architecture
-   extension being available.  Sometimes multiple entries are needed to enable
-   the extension (for example, the 'crypto' extension depends on four
-   entries: aes, pmull, sha1, sha2 being present).  In that case this field
-   should contain a space (" ") separated list of the strings in 'Features'
-   that are required.  Their order is not important.  */
+      AARCH64_OPT_EXTENSION(EXT_NAME, FLAG_CANONICAL, FLAGS_ON, FLAGS_OFF,
+			    SYNTHETIC, FEATURE_STRING)
+
+   - EXT_NAME is the name of the extension, represented as a string constant.
+   - FLAGS_CANONICAL is the canonical internal name for this flag.
+   - FLAGS_ON are the bitwise-or of the features that enabling the extension
+     adds, or zero if enabling this extension has no effect on other features.
+   - FLAGS_OFF are the bitwise-or of the features that disabling the extension
+     removes, or zero if disabling this extension has no effect on other
+     features.
+   - SYNTHETIC is a boolean to indicate whether the option is a purely synthetic
+     grouping of options and that the option itself has no feature bit (e.g.
+     crypto).  This is used to determine when sum of the individual options in
+     FLAGS_ON can be replaced by FLAG_CANONICAL in options minimization.  If the
+     group is synthetic then they can be replaced when all options in FLAGS_ON
+     are enabled, otherwise they can only be replaced when
+     FLAGS_ON | FLAG_CANONICAL are enabled.
+   - FEAT_STRING is a string containing the entries in the 'Features' field of
+     /proc/cpuinfo on a GNU/Linux system that correspond to this architecture
+     extension being available.  Sometimes multiple entries are needed to enable
+     the extension (for example, the 'crypto' extension depends on four
+     entries: aes, pmull, sha1, sha2 being present).  In that case this field
+     should contain a space (" ") separated list of the strings in 'Features'
+     that are required.  Their order is not important.  */
 
 /* Enabling "fp" just enables "fp".
    Disabling "fp" also disables "simd", "crypto", "fp16", "aes", "sha2",
    "sha3", sm3/sm4 and "sve".  */
 AARCH64_OPT_EXTENSION("fp", AARCH64_FL_FP, 0, AARCH64_FL_SIMD | AARCH64_FL_CRYPTO |\
 		      AARCH64_FL_F16 | AARCH64_FL_AES | AARCH64_FL_SHA2 |\
-		      AARCH64_FL_SHA3 | AARCH64_FL_SM4 | AARCH64_FL_SVE, "fp")
+		      AARCH64_FL_SHA3 | AARCH64_FL_SM4 | AARCH64_FL_SVE, \
+		      false, "fp")
 
 /* Enabling "simd" also enables "fp".
    Disabling "simd" also disables "crypto", "dotprod", "aes", "sha2", "sha3",
@@ -51,76 +60,86 @@ AARCH64_OPT_EXTENSION("fp", AARCH64_FL_FP, 0, AARCH64_FL_SIMD | AARCH64_FL_CRYPT
 AARCH64_OPT_EXTENSION("simd", AARCH64_FL_SIMD, AARCH64_FL_FP, AARCH64_FL_CRYPTO |\
 		      AARCH64_FL_DOTPROD | AARCH64_FL_AES | AARCH64_FL_SHA2 |\
 		      AARCH64_FL_SHA3 | AARCH64_FL_SM4 | AARCH64_FL_SVE,
-		      "asimd")
+		      false, "asimd")
 
-/* Enabling "crypto" also enables "fp" and "simd".
+/* Enabling "crypto" also enables "fp", "simd", "aes" and "sha2".
    Disabling "crypto" disables "crypto", "aes", "sha2", "sha3" and "sm3/sm4".  */
-AARCH64_OPT_EXTENSION("crypto", AARCH64_FL_CRYPTO, AARCH64_FL_FP | AARCH64_FL_SIMD,\
+AARCH64_OPT_EXTENSION("crypto", AARCH64_FL_CRYPTO,
+		      AARCH64_FL_FP | AARCH64_FL_SIMD | AARCH64_FL_AES |\
+		      AARCH64_FL_SHA2,\
 		      AARCH64_FL_AES | AARCH64_FL_SHA2 |AARCH64_FL_SHA3 | AARCH64_FL_SM4,\
-		      "aes pmull sha1 sha2")
+		      true, "aes pmull sha1 sha2")
 
 /* Enabling or disabling "crc" only changes "crc".  */
-AARCH64_OPT_EXTENSION("crc", AARCH64_FL_CRC, 0, 0, "crc32")
+AARCH64_OPT_EXTENSION("crc", AARCH64_FL_CRC, 0, 0, false, "crc32")
 
 /* Enabling or disabling "lse" only changes "lse".  */
-AARCH64_OPT_EXTENSION("lse", AARCH64_FL_LSE, 0, 0, "atomics")
+AARCH64_OPT_EXTENSION("lse", AARCH64_FL_LSE, 0, 0, false, "atomics")
 
 /* Enabling "fp16" also enables "fp".
    Disabling "fp16" disables "fp16", "fp16fml" and "sve".  */
 AARCH64_OPT_EXTENSION("fp16", AARCH64_FL_F16, AARCH64_FL_FP,
-		      AARCH64_FL_F16FML | AARCH64_FL_SVE, "fphp asimdhp")
+		      AARCH64_FL_F16FML | AARCH64_FL_SVE, false, "fphp asimdhp")
 
 /* Enabling or disabling "rcpc" only changes "rcpc".  */
-AARCH64_OPT_EXTENSION("rcpc", AARCH64_FL_RCPC, 0, 0, "lrcpc")
+AARCH64_OPT_EXTENSION("rcpc", AARCH64_FL_RCPC, 0, 0, false, "lrcpc")
 
 /* Enabling "rdma" also enables "fp", "simd".
    Disabling "rdma" just disables "rdma".  */
-AARCH64_OPT_EXTENSION("rdma", AARCH64_FL_RDMA, AARCH64_FL_FP | AARCH64_FL_SIMD, 0, "asimdrdm")
+AARCH64_OPT_EXTENSION("rdma", AARCH64_FL_RDMA, \
+		      AARCH64_FL_FP | AARCH64_FL_SIMD, 0, false, "asimdrdm")
 
 /* Enabling "dotprod" also enables "simd".
    Disabling "dotprod" only disables "dotprod".  */
-AARCH64_OPT_EXTENSION("dotprod", AARCH64_FL_DOTPROD, AARCH64_FL_SIMD, 0, "asimddp")
+AARCH64_OPT_EXTENSION("dotprod", AARCH64_FL_DOTPROD, AARCH64_FL_SIMD, 0, \
+		      false, "asimddp")
 
 /* Enabling "aes" also enables "simd".
    Disabling "aes" just disables "aes".  */
-AARCH64_OPT_EXTENSION("aes", AARCH64_FL_AES, AARCH64_FL_SIMD, 0, "aes")
+AARCH64_OPT_EXTENSION("aes", AARCH64_FL_AES, AARCH64_FL_SIMD, 0, false, "aes")
 
 /* Enabling "sha2" also enables "simd".
    Disabling "sha2" just disables "sha2".  */
-AARCH64_OPT_EXTENSION("sha2", AARCH64_FL_SHA2, AARCH64_FL_SIMD, 0, "sha1 sha2")
+AARCH64_OPT_EXTENSION("sha2", AARCH64_FL_SHA2, AARCH64_FL_SIMD, 0, false, \
+		      "sha1 sha2")
 
 /* Enabling "sha3" enables "simd" and "sha2".
    Disabling "sha3" just disables "sha3".  */
-AARCH64_OPT_EXTENSION("sha3", AARCH64_FL_SHA3, AARCH64_FL_SIMD | AARCH64_FL_SHA2, 0, "sha3 sha512")
+AARCH64_OPT_EXTENSION("sha3", AARCH64_FL_SHA3, \
+		      AARCH64_FL_SIMD | AARCH64_FL_SHA2, 0, false, \
+		      "sha3 sha512")
 
 /* Enabling "sm4" also enables "simd".
    Disabling "sm4" just disables "sm4".  */
-AARCH64_OPT_EXTENSION("sm4", AARCH64_FL_SM4, AARCH64_FL_SIMD, 0, "sm3 sm4")
+AARCH64_OPT_EXTENSION("sm4", AARCH64_FL_SM4, AARCH64_FL_SIMD, 0, false, \
+		      "sm3 sm4")
 
 /* Enabling "fp16fml" also enables "fp" and "fp16".
    Disabling "fp16fml" just disables "fp16fml".  */
-AARCH64_OPT_EXTENSION("fp16fml", AARCH64_FL_F16FML, AARCH64_FL_FP | AARCH64_FL_F16, 0, "asimdfml")
+AARCH64_OPT_EXTENSION("fp16fml", AARCH64_FL_F16FML, \
+		      AARCH64_FL_FP | AARCH64_FL_F16, 0, false, "asimdfml")
 
 /* Enabling "sve" also enables "fp16", "fp" and "simd".
    Disabling "sve" just disables "sve".  */
-AARCH64_OPT_EXTENSION("sve", AARCH64_FL_SVE, AARCH64_FL_FP | AARCH64_FL_SIMD | AARCH64_FL_F16, 0, "sve")
+AARCH64_OPT_EXTENSION("sve", AARCH64_FL_SVE, AARCH64_FL_FP | AARCH64_FL_SIMD | \
+		      AARCH64_FL_F16, 0, false, "sve")
 
 /* Enabling/Disabling "profile" does not enable/disable any other feature.  */
-AARCH64_OPT_EXTENSION("profile", AARCH64_FL_PROFILE, 0, 0, "")
+AARCH64_OPT_EXTENSION("profile", AARCH64_FL_PROFILE, 0, 0, false, "")
 
 /* Enabling/Disabling "rng" only changes "rng".  */
-AARCH64_OPT_EXTENSION("rng", AARCH64_FL_RNG, 0, 0, "")
+AARCH64_OPT_EXTENSION("rng", AARCH64_FL_RNG, 0, 0, false, "")
 
 /* Enabling/Disabling "memtag" only changes "memtag".  */
-AARCH64_OPT_EXTENSION("memtag", AARCH64_FL_MEMTAG, 0, 0, "")
+AARCH64_OPT_EXTENSION("memtag", AARCH64_FL_MEMTAG, 0, 0, false, "")
 
 /* Enabling/Disabling "sb" only changes "sb".  */
-AARCH64_OPT_EXTENSION("sb", AARCH64_FL_SB, 0, 0, "")
+AARCH64_OPT_EXTENSION("sb", AARCH64_FL_SB, 0, 0, false, "")
 
 /* Enabling/Disabling "ssbs" only changes "ssbs".  */
-AARCH64_OPT_EXTENSION("ssbs", AARCH64_FL_SSBS, 0, 0, "")
+AARCH64_OPT_EXTENSION("ssbs", AARCH64_FL_SSBS, 0, 0, false, "")
 
 /* Enabling/Disabling "predres" only changes "predres".  */
-AARCH64_OPT_EXTENSION("predres", AARCH64_FL_PREDRES, 0, 0, "")
+AARCH64_OPT_EXTENSION("predres", AARCH64_FL_PREDRES, 0, 0, false, "")
 
 #undef AARCH64_OPT_EXTENSION
diff --git a/gcc/config/aarch64/driver-aarch64.c b/gcc/config/aarch64/driver-aarch64.c
index 100e0c3529cee21a7c07122d9eea33c8dca83aca..6051443d9268c0d1d4c024be8ba0731c03340e59 100644
--- a/gcc/config/aarch64/driver-aarch64.c
+++ b/gcc/config/aarch64/driver-aarch64.c
@@ -36,7 +36,8 @@ struct aarch64_arch_extension
   const char *feat_string;
 };
 
-#define AARCH64_OPT_EXTENSION(EXT_NAME, FLAG_CANONICAL, FLAGS_ON, FLAGS_OFF, FEATURE_STRING) \
+#define AARCH64_OPT_EXTENSION(EXT_NAME, FLAG_CANONICAL, FLAGS_ON, FLAGS_OFF, \
+			      SYNTHETIC, FEATURE_STRING) \
   { EXT_NAME, FLAG_CANONICAL, FEATURE_STRING },
 static struct aarch64_arch_extension aarch64_extensions[] =
 {
diff --git a/gcc/testsuite/gcc.target/aarch64/options_set_1.c b/gcc/testsuite/gcc.target/aarch64/options_set_1.c
new file mode 100644
index 0000000000000000000000000000000000000000..40d9a05c905eb07103d3b437b5c1351e8620ab33
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/options_set_1.c
@@ -0,0 +1,11 @@
+/* { dg-do compile } */
+/* { dg-additional-options "-march=armv8.2-a" } */
+
+int main ()
+{
+  return 0;
+}
+
+/* { dg-final { scan-assembler-times {\.arch armv8\.2\-a\+crc} 1 } } */
+
+/* Check to see if crc is output by default.  */
diff --git a/gcc/testsuite/gcc.target/aarch64/options_set_2.c b/gcc/testsuite/gcc.target/aarch64/options_set_2.c
new file mode 100644
index 0000000000000000000000000000000000000000..3476febce706b34430682e879a4aa3aac8f752db
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/options_set_2.c
@@ -0,0 +1,11 @@
+/* { dg-do compile } */
+/* { dg-additional-options "-march=armv8.2-a+crypto" } */
+
+int main ()
+{
+  return 0;
+}
+
+/* { dg-final { scan-assembler-times {\.arch armv8\.2\-a\+crypto\+crc} 1 } } */
+
+/* Check to see if crc and crypto are maintained if crypto specified.  */
diff --git a/gcc/testsuite/gcc.target/aarch64/options_set_3.c b/gcc/testsuite/gcc.target/aarch64/options_set_3.c
new file mode 100644
index 0000000000000000000000000000000000000000..4558339f16c19555801899c357c50cedb23c28b0
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/options_set_3.c
@@ -0,0 +1,11 @@
+/* { dg-do compile } */
+/* { dg-additional-options "-march=armv8.2-a+aes+sha2+crypto" } */
+
+int main ()
+{
+  return 0;
+}
+
+/* { dg-final { scan-assembler-times {\.arch armv8\.2\-a\+crypto\+crc} 1 } } */
+
+/* Check if smallest set is maintained when outputting. */
diff --git a/gcc/testsuite/gcc.target/aarch64/options_set_4.c b/gcc/testsuite/gcc.target/aarch64/options_set_4.c
new file mode 100644
index 0000000000000000000000000000000000000000..15514bfe93e61e63cbce1262ee951358cd22d6ce
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/options_set_4.c
@@ -0,0 +1,12 @@
+/* { 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\+crypto\+crc} 1 } } */
+
+/* Check if individual bits that make up a grouping is specified that only the
+   grouping is kept. */
\ No newline at end of file
diff --git a/gcc/testsuite/gcc.target/aarch64/options_set_5.c b/gcc/testsuite/gcc.target/aarch64/options_set_5.c
new file mode 100644
index 0000000000000000000000000000000000000000..b4c0901195ede4fe0dd71fbe02a47c35e9dedbbd
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/options_set_5.c
@@ -0,0 +1,12 @@
+/* { dg-do compile } */
+/* { dg-additional-options "-march=armv8.2-a+aes+sha2+nosha2" } */
+
+int main ()
+{
+  return 0;
+}
+
+/* { dg-final { scan-assembler-times {\.arch armv8\.2\-a\+crc\+aes} 1 } } */
+
+/* Check if turning off feature bits works correctly and grouping is no
+   longer valid.   */
diff --git a/gcc/testsuite/gcc.target/aarch64/options_set_6.c b/gcc/testsuite/gcc.target/aarch64/options_set_6.c
new file mode 100644
index 0000000000000000000000000000000000000000..90a055928a273f06e08124a250e3107ad0704e47
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/options_set_6.c
@@ -0,0 +1,12 @@
+/* { dg-do compile } */
+/* { dg-additional-options "-march=armv8.2-a+crypto+nosha2" } */
+
+int main ()
+{
+  return 0;
+}
+
+/* { dg-final { scan-assembler-times {\.arch armv8\.2\-a\+crypto\+crc} 1 } } */
+
+/* Group as a whole was requested to be turned on, crypto itself is a bit and so
+   just turning off one feature can't turn it off.   */
diff --git a/gcc/testsuite/gcc.target/aarch64/options_set_7.c b/gcc/testsuite/gcc.target/aarch64/options_set_7.c
new file mode 100644
index 0000000000000000000000000000000000000000..71a2c8a19058c0ec25546085076503d206129e10
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/options_set_7.c
@@ -0,0 +1,11 @@
+/* { dg-do compile } */
+/* { dg-additional-options "-march=armv8.4-a+dotprod" } */
+
+int main ()
+{
+  return 0;
+}
+
+/* { dg-final { scan-assembler-times {\.arch armv8\.4\-a} 1 } } */
+
+/* Checking if enabling default features drops the superfluous bits.   */
diff --git a/gcc/testsuite/gcc.target/aarch64/options_set_8.c b/gcc/testsuite/gcc.target/aarch64/options_set_8.c
new file mode 100644
index 0000000000000000000000000000000000000000..83be1bd7a5c3f2bc8d11a14f2c16415c6a7056f2
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/options_set_8.c
@@ -0,0 +1,12 @@
+/* { dg-do compile } */
+/* { dg-additional-options "-march=armv8.4-a+nodotprod" } */
+
+int main ()
+{
+  return 0;
+}
+
+/* { dg-final { scan-assembler-times {\.arch armv8\.4\-a} 1 } } */
+
+/* Checking if trying to turn off default features propagates the commandline
+   option.  */
diff --git a/gcc/testsuite/gcc.target/aarch64/options_set_9.c b/gcc/testsuite/gcc.target/aarch64/options_set_9.c
new file mode 100644
index 0000000000000000000000000000000000000000..e3c7cdc54ffb0616877260c562354496cfdcb688
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/options_set_9.c
@@ -0,0 +1,14 @@
+/* { dg-do compile } */
+/* { dg-additional-options "-march=armv8-a+simd+fp" } */
+
+int main ()
+{
+  return 0;
+}
+
+/* { dg-final { scan-assembler-times {\.arch armv8\-a} 1 } } */
+
+ /* Check that grouping of bits that don't form a synthetic group don't turn
+    on the parent. e.g. rdma turns on simd+fp, but simd+fp does not turn on
+    rdma since rdma is it's own group.  crypto however turns on aes and sha2
+    and turning on sha2 and eas should turn on crypto!.  */