[PATCH][GCC][AArch64] Fix command line options canonicalization. (PR target/88530)

public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed

* [PATCH][GCC][AArch64] Fix command line options canonicalization. (PR target/88530)
@ 2018-12-17 19:19 Tamar Christina
  2019-01-10 17:15 ` Kyrill Tkachov
  0 siblings, 1 reply; 5+ messages in thread
From: Tamar Christina @ 2018-12-17 19:19 UTC (permalink / raw)
  To: gcc-patches; +Cc: nd, James Greenhalgh, Richard Earnshaw, Marcus Shawcroft

[-- Attachment #1: Type: text/plain, Size: 3614 bytes --]

Hi All,

The options don't seem to 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, so we need to get the smallest possible set, which would also fix the
problem with older the assemblers and the retro-active split.

Admittedly this should be done earlier in options processing, but the problem
with the way AArch64 currently processes options is that where the isa_bits are
determined we don't know which options are part of the default set yet.

Which is why we instead do it late in processing when we have all the
information.  This however requires us to make a duplicate of the extensions
list.

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 has a bit.

e.g. +crypto isn't an actual bit, it just enables other bits, but other features flags
like +rdma also enable multiple options but are themselves 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 and give the least surprises.

This is a stop-gap measure that's has the lowest impact and is back-portable.

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:

2018-12-17  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): Set is_synthetic to false.
	(crypto): Set is_synthetic to true.

gcc/testsuite/ChangeLog:

2018-12-17  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.

-- 

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: rb10429.patch --]
[-- Type: text/x-diff; name="rb10429.patch", Size: 20435 bytes --]

diff --git a/gcc/common/config/aarch64/aarch64-common.c b/gcc/common/config/aarch64/aarch64-common.c
index dd7d42673402c3cf16ebce009d263d62d574690a..d14237d229fd958c940aee32d3d6404b04cc137e 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}
+  {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, false}
 };
 
 struct processor_name_to_arch
@@ -298,6 +311,69 @@ 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 an option to set the bit it turns on, and the other flags it
+     turns on as well.  */
+  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, try to 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
+       bits first.  */
+    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 is enabled in
+   ISA_FLAG_BITS to be able to replace the invididual options with the
+   canonicalized version of the option.  The rules are a bit tricky as we have
+   two kinds of options.
+
+   1) Synthetic groups, such as +crypto which on their own don't have a feature
+      bit but turn on multiple bits.  These kind of options should be used when
+      ever the bits they turn on are all available.
+
+   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.
+*/
+
+static bool
+aarch64_contains_opt (unsigned long isa_flag_bits, opt_ext *opt)
+{
+  unsigned long flags_check = opt->flags_on;
+  if (!opt->is_synthetic)
+    flags_check |= 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,27 +387,49 @@ 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
+  unsigned long isa_flag_bits = isa_flags;
+
+  /* Pass one: For the bits that turn on multiple bits, remove the invidual bits
+     it they are all present. The option on it's own is enough to trigger their
+     inclusion.  */
+  for (opt = all_extensions_by_on; opt->name != NULL; opt++)
+    if ((popcount_hwi ((HOST_WIDE_INT)opt->flags_on) > 1
+	 && aarch64_contains_opt (isa_flag_bits, opt)
+	 && !(default_arch_flags & opt->flag_canonical)))
+      {
+	isa_flag_bits &= ~opt->flags_on;
+	isa_flag_bits |= opt->flag_canonical;
+      }
+
+  /* Pass two: Find all the things we need to turn on.  As a special case,
+     we always want to put out +crc if it is enabled.  Once a bit has been
+     "claimed" but an option we clear it so another option doesn't get set
+     due to it.  This relies on the options being processed in order of largest
+     population to smallest.  */
+  for (opt = all_extensions_by_on; opt->name != NULL; opt++)
+    if ((isa_flag_bits & 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;
+	isa_flag_bits &= ~(opt->flag_canonical | opt->flags_on);
       }
 
-  /* Pass two: Find all the things we need to turn off.  */
-  for (opt = all_extensions; opt->name != NULL; opt++)
+  /* Pass three: Find all the things we need to turn off, we aim to give the
+     small-ish set possible by looking at the amount of things the feature turns
+     on.  By reasoning it probably also turns off a bunch of things when you
+     turn it off.  Unlike the above we don't care about the smallest set in this
+     case as "+no" flags are rarely passed along to the assembler.  */
+  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;
+	isa_flags &= ~(opt->flag_canonical | opt->flags_off);
       }
-
   return outstr;
 }
 
@@ -411,5 +509,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 69ab796a4e1a959b89ebb55b599919c442cfb088..cdf04e2d5fcccb8b9a32af8f83501ce23212bbab 100644
--- a/gcc/config/aarch64/aarch64-option-extensions.def
+++ b/gcc/config/aarch64/aarch64-option-extensions.def
@@ -30,6 +30,13 @@
    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 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
@@ -43,7 +50,8 @@
    "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,61 +59,71 @@ 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, "")
 
 #undef AARCH64_OPT_EXTENSION
diff --git a/gcc/config/aarch64/driver-aarch64.c b/gcc/config/aarch64/driver-aarch64.c
index 4e83c7a7679fa683af221fb5f726bc8da339081e..98f9d7959506338bd6a8524500a168cc22ef5396 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!.  */


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH][GCC][AArch64] Fix command line options canonicalization. (PR target/88530)
  2018-12-17 19:19 [PATCH][GCC][AArch64] Fix command line options canonicalization. (PR target/88530) Tamar Christina
@ 2019-01-10 17:15 ` Kyrill Tkachov
  2019-01-15 17:13   ` Tamar Christina
  0 siblings, 1 reply; 5+ messages in thread
From: Kyrill Tkachov @ 2019-01-10 17:15 UTC (permalink / raw)
  To: Tamar Christina, gcc-patches
  Cc: nd, James Greenhalgh, Richard Earnshaw, Marcus Shawcroft

Hi Tamar,

On 17/12/18 19:18, Tamar Christina wrote:
> Hi All,
>
> The options don't seem to 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, so we need to get the smallest possible set, which would also fix the
> problem with older the assemblers and the retro-active split.
>
> Admittedly this should be done earlier in options processing, but the problem
> with the way AArch64 currently processes options is that where the isa_bits are
> determined we don't know which options are part of the default set yet.
>
> Which is why we instead do it late in processing when we have all the
> information.  This however requires us to make a duplicate of the extensions
> list.
>
> 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 has a bit.
>
> e.g. +crypto isn't an actual bit, it just enables other bits, but other features flags
> like +rdma also enable multiple options but are themselves 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 and give the least surprises.
>
> This is a stop-gap measure that's has the lowest impact and is back-portable.
>
> 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?
>

This will need rebasing over the Armv8.5-A patches as there are new entries in config/aarch64/aarch64-option-extensions.def.
Since this has to be done anyway, I've also pointed out a few comment typos inline.

Apart from that, the patch looks good to me (this is a subtle area of GCC).

Thanks,
Kyrill


> Thanks,
> Tamar
>
> gcc/ChangeLog:
>
> 2018-12-17  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): Set is_synthetic to false.
>         (crypto): Set is_synthetic to true.
>
> gcc/testsuite/ChangeLog:
>
> 2018-12-17  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 dd7d42673402c3cf16ebce009d263d62d574690a..d14237d229fd958c940aee32d3d6404b04cc137e 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}
+  {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, false}
  };
  
  struct processor_name_to_arch
@@ -298,6 +311,69 @@ 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 an option to set the bit it turns on, and the other flags it
+     turns on as well.  */
+  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, try to 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
+       bits first.  */
+    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 is enabled in

"is enabled" -> "are enabled"

  +   ISA_FLAG_BITS to be able to replace the invididual options with the

"individual"

+   canonicalized version of the option.  The rules are a bit tricky as we have
+   two kinds of options.
+
+   1) Synthetic groups, such as +crypto which on their own don't have a feature
+      bit but turn on multiple bits.  These kind of options should be used when
+      ever the bits they turn on are all available.
+
+   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.
+*/
+
+static bool
+aarch64_contains_opt (unsigned long isa_flag_bits, opt_ext *opt)
+{
+  unsigned long flags_check = opt->flags_on;
+  if (!opt->is_synthetic)
+    flags_check |= 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,27 +387,49 @@ 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
+  unsigned long isa_flag_bits = isa_flags;
+
+  /* Pass one: For the bits that turn on multiple bits, remove the invidual bits

"individual" again.

+     it they are all present. The option on it's own is enough to trigger their
+     inclusion.  */
+  for (opt = all_extensions_by_on; opt->name != NULL; opt++)
+    if ((popcount_hwi ((HOST_WIDE_INT)opt->flags_on) > 1
+	 && aarch64_contains_opt (isa_flag_bits, opt)
+	 && !(default_arch_flags & opt->flag_canonical)))
+      {
+	isa_flag_bits &= ~opt->flags_on;
+	isa_flag_bits |= opt->flag_canonical;
+      }
+
+  /* Pass two: Find all the things we need to turn on.  As a special case,
+     we always want to put out +crc if it is enabled.  Once a bit has been
+     "claimed" but an option we clear it so another option doesn't get set

"but" -> "by"?

+     due to it.  This relies on the options being processed in order of largest
+     population to smallest.  */
+  for (opt = all_extensions_by_on; opt->name != NULL; opt++)
+    if ((isa_flag_bits & 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;
+	isa_flag_bits &= ~(opt->flag_canonical | opt->flags_on);
        }
  
-  /* Pass two: Find all the things we need to turn off.  */
-  for (opt = all_extensions; opt->name != NULL; opt++)
+  /* Pass three: Find all the things we need to turn off, we aim to give the
+     small-ish set possible by looking at the amount of things the feature turns
+     on.  By reasoning it probably also turns off a bunch of things when you
+     turn it off.  Unlike the above we don't care about the smallest set in this
+     case as "+no" flags are rarely passed along to the assembler.  */
+  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;
+	isa_flags &= ~(opt->flag_canonical | opt->flags_off);
        }
-
    return outstr;
  }
  
@@ -411,5 +509,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 69ab796a4e1a959b89ebb55b599919c442cfb088..cdf04e2d5fcccb8b9a32af8f83501ce23212bbab 100644
--- a/gcc/config/aarch64/aarch64-option-extensions.def
+++ b/gcc/config/aarch64/aarch64-option-extensions.def
@@ -30,6 +30,13 @@
     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 replaced when FLAGS_ON | FLAG_CANONICAL

"can only replaced" -> "can only be replaced"

+   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
@@ -43,7 +50,8 @@
     "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,61 +59,71 @@ 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, "")
  
  #undef AARCH64_OPT_EXTENSION
diff --git a/gcc/config/aarch64/driver-aarch64.c b/gcc/config/aarch64/driver-aarch64.c
index 4e83c7a7679fa683af221fb5f726bc8da339081e..98f9d7959506338bd6a8524500a168cc22ef5396 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!.  */



^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH][GCC][AArch64] Fix command line options canonicalization. (PR target/88530)
  2019-01-10 17:15 ` Kyrill Tkachov
@ 2019-01-15 17:13   ` Tamar Christina
  2019-01-30 14:06     ` Tamar Christina
  0 siblings, 1 reply; 5+ messages in thread
From: Tamar Christina @ 2019-01-15 17:13 UTC (permalink / raw)
  To: Kyrill Tkachov
  Cc: gcc-patches, nd, James Greenhalgh, Richard Earnshaw, Marcus Shawcroft

[-- Attachment #1: Type: text/plain, Size: 27965 bytes --]

Hi Kyrill,

Thanks for the review,

I have respun the patch on top of trunk and
here is the new changelog to account for the
updates of the new extensions.

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

Ok for trunk?

Thanks,
Tamar

gcc/ChangeLog:

2019-01-15  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.

gcc/testsuite/ChangeLog:

2019-01-15  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 01/10/2019 17:15, Kyrill Tkachov wrote:
> Hi Tamar,
> 
> On 17/12/18 19:18, Tamar Christina wrote:
> > Hi All,
> >
> > The options don't seem to 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, so we need to get the smallest possible set, which would also fix the
> > problem with older the assemblers and the retro-active split.
> >
> > Admittedly this should be done earlier in options processing, but the problem
> > with the way AArch64 currently processes options is that where the isa_bits are
> > determined we don't know which options are part of the default set yet.
> >
> > Which is why we instead do it late in processing when we have all the
> > information.  This however requires us to make a duplicate of the extensions
> > list.
> >
> > 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 has a bit.
> >
> > e.g. +crypto isn't an actual bit, it just enables other bits, but other features flags
> > like +rdma also enable multiple options but are themselves 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 and give the least surprises.
> >
> > This is a stop-gap measure that's has the lowest impact and is back-portable.
> >
> > 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?
> >
> 
> This will need rebasing over the Armv8.5-A patches as there are new entries in config/aarch64/aarch64-option-extensions.def.
> Since this has to be done anyway, I've also pointed out a few comment typos inline.
> 
> Apart from that, the patch looks good to me (this is a subtle area of GCC).
> 
> Thanks,
> Kyrill
> 
> 
> > Thanks,
> > Tamar
> >
> > gcc/ChangeLog:
> >
> > 2018-12-17  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): Set is_synthetic to false.
> >         (crypto): Set is_synthetic to true.
> >
> > gcc/testsuite/ChangeLog:
> >
> > 2018-12-17  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 dd7d42673402c3cf16ebce009d263d62d574690a..d14237d229fd958c940aee32d3d6404b04cc137e 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}
> +  {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, false}
>   };
>   
>   struct processor_name_to_arch
> @@ -298,6 +311,69 @@ 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 an option to set the bit it turns on, and the other flags it
> +     turns on as well.  */
> +  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, try to 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
> +       bits first.  */
> +    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 is enabled in
> 
> "is enabled" -> "are enabled"
> 
>   +   ISA_FLAG_BITS to be able to replace the invididual options with the
> 
> "individual"
> 
> +   canonicalized version of the option.  The rules are a bit tricky as we have
> +   two kinds of options.
> +
> +   1) Synthetic groups, such as +crypto which on their own don't have a feature
> +      bit but turn on multiple bits.  These kind of options should be used when
> +      ever the bits they turn on are all available.
> +
> +   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.
> +*/
> +
> +static bool
> +aarch64_contains_opt (unsigned long isa_flag_bits, opt_ext *opt)
> +{
> +  unsigned long flags_check = opt->flags_on;
> +  if (!opt->is_synthetic)
> +    flags_check |= 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,27 +387,49 @@ 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
> +  unsigned long isa_flag_bits = isa_flags;
> +
> +  /* Pass one: For the bits that turn on multiple bits, remove the invidual bits
> 
> "individual" again.
> 
> +     it they are all present. The option on it's own is enough to trigger their
> +     inclusion.  */
> +  for (opt = all_extensions_by_on; opt->name != NULL; opt++)
> +    if ((popcount_hwi ((HOST_WIDE_INT)opt->flags_on) > 1
> +	 && aarch64_contains_opt (isa_flag_bits, opt)
> +	 && !(default_arch_flags & opt->flag_canonical)))
> +      {
> +	isa_flag_bits &= ~opt->flags_on;
> +	isa_flag_bits |= opt->flag_canonical;
> +      }
> +
> +  /* Pass two: Find all the things we need to turn on.  As a special case,
> +     we always want to put out +crc if it is enabled.  Once a bit has been
> +     "claimed" but an option we clear it so another option doesn't get set
> 
> "but" -> "by"?
> 
> +     due to it.  This relies on the options being processed in order of largest
> +     population to smallest.  */
> +  for (opt = all_extensions_by_on; opt->name != NULL; opt++)
> +    if ((isa_flag_bits & 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;
> +	isa_flag_bits &= ~(opt->flag_canonical | opt->flags_on);
>         }
>   
> -  /* Pass two: Find all the things we need to turn off.  */
> -  for (opt = all_extensions; opt->name != NULL; opt++)
> +  /* Pass three: Find all the things we need to turn off, we aim to give the
> +     small-ish set possible by looking at the amount of things the feature turns
> +     on.  By reasoning it probably also turns off a bunch of things when you
> +     turn it off.  Unlike the above we don't care about the smallest set in this
> +     case as "+no" flags are rarely passed along to the assembler.  */
> +  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;
> +	isa_flags &= ~(opt->flag_canonical | opt->flags_off);
>         }
> -
>     return outstr;
>   }
>   
> @@ -411,5 +509,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 69ab796a4e1a959b89ebb55b599919c442cfb088..cdf04e2d5fcccb8b9a32af8f83501ce23212bbab 100644
> --- a/gcc/config/aarch64/aarch64-option-extensions.def
> +++ b/gcc/config/aarch64/aarch64-option-extensions.def
> @@ -30,6 +30,13 @@
>      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 replaced when FLAGS_ON | FLAG_CANONICAL
> 
> "can only replaced" -> "can only be replaced"
> 
> +   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
> @@ -43,7 +50,8 @@
>      "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,61 +59,71 @@ 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, "")
>   
>   #undef AARCH64_OPT_EXTENSION
> diff --git a/gcc/config/aarch64/driver-aarch64.c b/gcc/config/aarch64/driver-aarch64.c
> index 4e83c7a7679fa683af221fb5f726bc8da339081e..98f9d7959506338bd6a8524500a168cc22ef5396 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!.  */
> 
> 
> 

-- 

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: pr10429.patch --]
[-- Type: text/x-diff; name="pr10429.patch", Size: 21372 bytes --]

diff --git a/gcc/common/config/aarch64/aarch64-common.c b/gcc/common/config/aarch64/aarch64-common.c
index fd870e187a6634b929bc058f99e768e829008179..45f56ebad5848c646dcb2c5640ddf93a083d2edf 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}
+  {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, false}
 };
 
 struct processor_name_to_arch
@@ -298,6 +311,69 @@ 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 an option to set the bit it turns on, and the other flags it
+     turns on as well.  */
+  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, try to 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
+       bits first.  */
+    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.  The rules are a bit tricky as we have
+   two kinds of options.
+
+   1) Synthetic groups, such as +crypto which on their own don't have a feature
+      bit but turn on multiple bits.  These kind of options should be used when
+      ever the bits they turn on are all available.
+
+   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.
+*/
+
+static bool
+aarch64_contains_opt (unsigned long isa_flag_bits, opt_ext *opt)
+{
+  unsigned long flags_check = opt->flags_on;
+  if (!opt->is_synthetic)
+    flags_check |= 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,27 +387,49 @@ 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
+  unsigned long isa_flag_bits = isa_flags;
+
+  /* Pass one: For the bits that turn on multiple bits, remove the individual bits
+     it they are all present. The option on it's own is enough to trigger their
+     inclusion.  */
+  for (opt = all_extensions_by_on; opt->name != NULL; opt++)
+    if ((popcount_hwi ((HOST_WIDE_INT)opt->flags_on) > 1
+	 && aarch64_contains_opt (isa_flag_bits, opt)
+	 && !(default_arch_flags & opt->flag_canonical)))
+      {
+	isa_flag_bits &= ~opt->flags_on;
+	isa_flag_bits |= opt->flag_canonical;
+      }
+
+  /* Pass two: Find all the things we need to turn on.  As a special case,
+     we always want to put out +crc if it is enabled.  Once a bit has been
+     "claimed" by an option we clear it so another option doesn't get set
+     due to it.  This relies on the options being processed in order of largest
+     population to smallest.  */
+  for (opt = all_extensions_by_on; opt->name != NULL; opt++)
+    if ((isa_flag_bits & 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;
+	isa_flag_bits &= ~(opt->flag_canonical | opt->flags_on);
       }
 
-  /* Pass two: Find all the things we need to turn off.  */
-  for (opt = all_extensions; opt->name != NULL; opt++)
+  /* Pass three: Find all the things we need to turn off, we aim to give the
+     small-ish set possible by looking at the amount of things the feature turns
+     on.  By reasoning it probably also turns off a bunch of things when you
+     turn it off.  Unlike the above we don't care about the smallest set in this
+     case as "+no" flags are rarely passed along to the assembler.  */
+  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;
+	isa_flags &= ~(opt->flag_canonical | opt->flags_off);
       }
-
   return outstr;
 }
 
@@ -411,5 +509,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..50909debf5455d57b86db91a0a5539fed1d5b91e 100644
--- a/gcc/config/aarch64/aarch64-option-extensions.def
+++ b/gcc/config/aarch64/aarch64-option-extensions.def
@@ -30,6 +30,13 @@
    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
@@ -43,7 +50,8 @@
    "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 +59,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 2bf1f9a8c13880dd53980cfb7b829d4263d3d7b6..05f1360d48583473820c8008cc09ed139bddc0ce 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!.  */


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH][GCC][AArch64] Fix command line options canonicalization. (PR target/88530)
  2019-01-15 17:13   ` Tamar Christina
@ 2019-01-30 14:06     ` Tamar Christina
  2019-02-06 15:08       ` Tamar Christina
  0 siblings, 1 reply; 5+ messages in thread
From: Tamar Christina @ 2019-01-30 14:06 UTC (permalink / raw)
  To: Kyrill Tkachov
  Cc: gcc-patches, nd, James Greenhalgh, Richard Earnshaw, Marcus Shawcroft

Ping.

________________________________________
From: gcc-patches-owner@gcc.gnu.org <gcc-patches-owner@gcc.gnu.org> on behalf of Tamar Christina <Tamar.Christina@arm.com>
Sent: Tuesday, January 15, 2019 5:12:46 PM
To: Kyrill Tkachov
Cc: gcc-patches@gcc.gnu.org; nd; James Greenhalgh; Richard Earnshaw; Marcus Shawcroft
Subject: Re: [PATCH][GCC][AArch64] Fix command line options canonicalization. (PR target/88530)

Hi Kyrill,

Thanks for the review,

I have respun the patch on top of trunk and
here is the new changelog to account for the
updates of the new extensions.

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

Ok for trunk?

Thanks,
Tamar

gcc/ChangeLog:

2019-01-15  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.

gcc/testsuite/ChangeLog:

2019-01-15  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 01/10/2019 17:15, Kyrill Tkachov wrote:
> Hi Tamar,
>
> On 17/12/18 19:18, Tamar Christina wrote:
> > Hi All,
> >
> > The options don't seem to 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, so we need to get the smallest possible set, which would also fix the
> > problem with older the assemblers and the retro-active split.
> >
> > Admittedly this should be done earlier in options processing, but the problem
> > with the way AArch64 currently processes options is that where the isa_bits are
> > determined we don't know which options are part of the default set yet.
> >
> > Which is why we instead do it late in processing when we have all the
> > information.  This however requires us to make a duplicate of the extensions
> > list.
> >
> > 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 has a bit.
> >
> > e.g. +crypto isn't an actual bit, it just enables other bits, but other features flags
> > like +rdma also enable multiple options but are themselves 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 and give the least surprises.
> >
> > This is a stop-gap measure that's has the lowest impact and is back-portable.
> >
> > 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?
> >
>
> This will need rebasing over the Armv8.5-A patches as there are new entries in config/aarch64/aarch64-option-extensions.def.
> Since this has to be done anyway, I've also pointed out a few comment typos inline.
>
> Apart from that, the patch looks good to me (this is a subtle area of GCC).
>
> Thanks,
> Kyrill
>
>
> > Thanks,
> > Tamar
> >
> > gcc/ChangeLog:
> >
> > 2018-12-17  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): Set is_synthetic to false.
> >         (crypto): Set is_synthetic to true.
> >
> > gcc/testsuite/ChangeLog:
> >
> > 2018-12-17  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 dd7d42673402c3cf16ebce009d263d62d574690a..d14237d229fd958c940aee32d3d6404b04cc137e 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}
> +  {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, false}
>   };
>
>   struct processor_name_to_arch
> @@ -298,6 +311,69 @@ 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 an option to set the bit it turns on, and the other flags it
> +     turns on as well.  */
> +  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, try to 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
> +       bits first.  */
> +    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 is enabled in
>
> "is enabled" -> "are enabled"
>
>   +   ISA_FLAG_BITS to be able to replace the invididual options with the
>
> "individual"
>
> +   canonicalized version of the option.  The rules are a bit tricky as we have
> +   two kinds of options.
> +
> +   1) Synthetic groups, such as +crypto which on their own don't have a feature
> +      bit but turn on multiple bits.  These kind of options should be used when
> +      ever the bits they turn on are all available.
> +
> +   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.
> +*/
> +
> +static bool
> +aarch64_contains_opt (unsigned long isa_flag_bits, opt_ext *opt)
> +{
> +  unsigned long flags_check = opt->flags_on;
> +  if (!opt->is_synthetic)
> +    flags_check |= 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,27 +387,49 @@ 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
> +  unsigned long isa_flag_bits = isa_flags;
> +
> +  /* Pass one: For the bits that turn on multiple bits, remove the invidual bits
>
> "individual" again.
>
> +     it they are all present. The option on it's own is enough to trigger their
> +     inclusion.  */
> +  for (opt = all_extensions_by_on; opt->name != NULL; opt++)
> +    if ((popcount_hwi ((HOST_WIDE_INT)opt->flags_on) > 1
> +      && aarch64_contains_opt (isa_flag_bits, opt)
> +      && !(default_arch_flags & opt->flag_canonical)))
> +      {
> +     isa_flag_bits &= ~opt->flags_on;
> +     isa_flag_bits |= opt->flag_canonical;
> +      }
> +
> +  /* Pass two: Find all the things we need to turn on.  As a special case,
> +     we always want to put out +crc if it is enabled.  Once a bit has been
> +     "claimed" but an option we clear it so another option doesn't get set
>
> "but" -> "by"?
>
> +     due to it.  This relies on the options being processed in order of largest
> +     population to smallest.  */
> +  for (opt = all_extensions_by_on; opt->name != NULL; opt++)
> +    if ((isa_flag_bits & 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;
> +     isa_flag_bits &= ~(opt->flag_canonical | opt->flags_on);
>         }
>
> -  /* Pass two: Find all the things we need to turn off.  */
> -  for (opt = all_extensions; opt->name != NULL; opt++)
> +  /* Pass three: Find all the things we need to turn off, we aim to give the
> +     small-ish set possible by looking at the amount of things the feature turns
> +     on.  By reasoning it probably also turns off a bunch of things when you
> +     turn it off.  Unlike the above we don't care about the smallest set in this
> +     case as "+no" flags are rarely passed along to the assembler.  */
> +  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;
> +     isa_flags &= ~(opt->flag_canonical | opt->flags_off);
>         }
> -
>     return outstr;
>   }
>
> @@ -411,5 +509,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 69ab796a4e1a959b89ebb55b599919c442cfb088..cdf04e2d5fcccb8b9a32af8f83501ce23212bbab 100644
> --- a/gcc/config/aarch64/aarch64-option-extensions.def
> +++ b/gcc/config/aarch64/aarch64-option-extensions.def
> @@ -30,6 +30,13 @@
>      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 replaced when FLAGS_ON | FLAG_CANONICAL
>
> "can only replaced" -> "can only be replaced"
>
> +   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
> @@ -43,7 +50,8 @@
>      "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,61 +59,71 @@ 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, "")
>
>   #undef AARCH64_OPT_EXTENSION
> diff --git a/gcc/config/aarch64/driver-aarch64.c b/gcc/config/aarch64/driver-aarch64.c
> index 4e83c7a7679fa683af221fb5f726bc8da339081e..98f9d7959506338bd6a8524500a168cc22ef5396 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!.  */
>
>
>

--

^ permalink raw reply	[flat|nested] 5+ messages in thread

* RE: [PATCH][GCC][AArch64] Fix command line options canonicalization. (PR target/88530)
  2019-01-30 14:06     ` Tamar Christina
@ 2019-02-06 15:08       ` Tamar Christina
  0 siblings, 0 replies; 5+ messages in thread
From: Tamar Christina @ 2019-02-06 15:08 UTC (permalink / raw)
  To: Tamar Christina, Kyrill Tkachov
  Cc: gcc-patches, nd, James Greenhalgh, Richard Earnshaw, Marcus Shawcroft

Friendly Ping, is there anything I can do to move this one along?

Thanks,
Tamar

> -----Original Message-----
> From: gcc-patches-owner@gcc.gnu.org <gcc-patches-owner@gcc.gnu.org>
> On Behalf Of Tamar Christina
> Sent: Wednesday, January 30, 2019 14:02
> To: Kyrill Tkachov <kyrylo.tkachov@foss.arm.com>
> Cc: gcc-patches@gcc.gnu.org; nd <nd@arm.com>; James Greenhalgh
> <James.Greenhalgh@arm.com>; Richard Earnshaw
> <Richard.Earnshaw@arm.com>; Marcus Shawcroft
> <Marcus.Shawcroft@arm.com>
> Subject: Re: [PATCH][GCC][AArch64] Fix command line options
> canonicalization. (PR target/88530)
> 
> Ping.
> 
> ________________________________________
> From: gcc-patches-owner@gcc.gnu.org <gcc-patches-owner@gcc.gnu.org>
> on behalf of Tamar Christina <Tamar.Christina@arm.com>
> Sent: Tuesday, January 15, 2019 5:12:46 PM
> To: Kyrill Tkachov
> Cc: gcc-patches@gcc.gnu.org; nd; James Greenhalgh; Richard Earnshaw;
> Marcus Shawcroft
> Subject: Re: [PATCH][GCC][AArch64] Fix command line options
> canonicalization. (PR target/88530)
> 
> Hi Kyrill,
> 
> Thanks for the review,
> 
> I have respun the patch on top of trunk and here is the new changelog to
> account for the updates of the new extensions.
> 
> Bootstrapped Regtested on aarch64-none-linux-gnu and no issues.
> 
> Ok for trunk?
> 
> Thanks,
> Tamar
> 
> gcc/ChangeLog:
> 
> 2019-01-15  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.
> 
> gcc/testsuite/ChangeLog:
> 
> 2019-01-15  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 01/10/2019 17:15, Kyrill Tkachov wrote:
> > Hi Tamar,
> >
> > On 17/12/18 19:18, Tamar Christina wrote:
> > > Hi All,
> > >
> > > The options don't seem to 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, so we need to get the smallest possible
> > > set, which would also fix the problem with older the assemblers and the
> retro-active split.
> > >
> > > Admittedly this should be done earlier in options processing, but
> > > the problem with the way AArch64 currently processes options is that
> > > where the isa_bits are determined we don't know which options are part
> of the default set yet.
> > >
> > > Which is why we instead do it late in processing when we have all
> > > the information.  This however requires us to make a duplicate of
> > > the extensions list.
> > >
> > > 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 has a bit.
> > >
> > > e.g. +crypto isn't an actual bit, it just enables other bits, but
> > > other features flags like +rdma also enable multiple options but are
> themselves 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 and give the least surprises.
> > >
> > > This is a stop-gap measure that's has the lowest impact and is back-
> portable.
> > >
> > > 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?
> > >
> >
> > This will need rebasing over the Armv8.5-A patches as there are new
> entries in config/aarch64/aarch64-option-extensions.def.
> > Since this has to be done anyway, I've also pointed out a few comment
> typos inline.
> >
> > Apart from that, the patch looks good to me (this is a subtle area of GCC).
> >
> > Thanks,
> > Kyrill
> >
> >
> > > Thanks,
> > > Tamar
> > >
> > > gcc/ChangeLog:
> > >
> > > 2018-12-17  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): Set is_synthetic to false.
> > >         (crypto): Set is_synthetic to true.
> > >
> > > gcc/testsuite/ChangeLog:
> > >
> > > 2018-12-17  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
> >
> dd7d42673402c3cf16ebce009d263d62d574690a..d14237d229fd958c940aee32d
> 3d6
> > 404b04cc137e 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}
> > +  {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, false}
> >   };
> >
> >   struct processor_name_to_arch
> > @@ -298,6 +311,69 @@ 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 an option to set the bit it turns on, and the other flags it
> > +     turns on as well.  */
> > +  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, try to 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
> > +       bits first.  */
> > +    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 is enabled in
> >
> > "is enabled" -> "are enabled"
> >
> >   +   ISA_FLAG_BITS to be able to replace the invididual options with the
> >
> > "individual"
> >
> > +   canonicalized version of the option.  The rules are a bit tricky as we have
> > +   two kinds of options.
> > +
> > +   1) Synthetic groups, such as +crypto which on their own don't have a
> feature
> > +      bit but turn on multiple bits.  These kind of options should be used
> when
> > +      ever the bits they turn on are all available.
> > +
> > +   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.
> > +*/
> > +
> > +static bool
> > +aarch64_contains_opt (unsigned long isa_flag_bits, opt_ext *opt) {
> > +  unsigned long flags_check = opt->flags_on;
> > +  if (!opt->is_synthetic)
> > +    flags_check |= 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,27 +387,49 @@ 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
> > +  unsigned long isa_flag_bits = isa_flags;
> > +
> > +  /* Pass one: For the bits that turn on multiple bits, remove the
> > + invidual bits
> >
> > "individual" again.
> >
> > +     it they are all present. The option on it's own is enough to trigger their
> > +     inclusion.  */
> > +  for (opt = all_extensions_by_on; opt->name != NULL; opt++)
> > +    if ((popcount_hwi ((HOST_WIDE_INT)opt->flags_on) > 1
> > +      && aarch64_contains_opt (isa_flag_bits, opt)
> > +      && !(default_arch_flags & opt->flag_canonical)))
> > +      {
> > +     isa_flag_bits &= ~opt->flags_on;
> > +     isa_flag_bits |= opt->flag_canonical;
> > +      }
> > +
> > +  /* Pass two: Find all the things we need to turn on.  As a special case,
> > +     we always want to put out +crc if it is enabled.  Once a bit has been
> > +     "claimed" but an option we clear it so another option doesn't
> > + get set
> >
> > "but" -> "by"?
> >
> > +     due to it.  This relies on the options being processed in order of largest
> > +     population to smallest.  */
> > +  for (opt = all_extensions_by_on; opt->name != NULL; opt++)
> > +    if ((isa_flag_bits & 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;
> > +     isa_flag_bits &= ~(opt->flag_canonical | opt->flags_on);
> >         }
> >
> > -  /* Pass two: Find all the things we need to turn off.  */
> > -  for (opt = all_extensions; opt->name != NULL; opt++)
> > +  /* Pass three: Find all the things we need to turn off, we aim to give the
> > +     small-ish set possible by looking at the amount of things the feature
> turns
> > +     on.  By reasoning it probably also turns off a bunch of things when you
> > +     turn it off.  Unlike the above we don't care about the smallest set in this
> > +     case as "+no" flags are rarely passed along to the assembler.
> > + */  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;
> > +     isa_flags &= ~(opt->flag_canonical | opt->flags_off);
> >         }
> > -
> >     return outstr;
> >   }
> >
> > @@ -411,5 +509,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
> >
> 69ab796a4e1a959b89ebb55b599919c442cfb088..cdf04e2d5fcccb8b9a32af8f83
> 50
> > 1ce23212bbab 100644
> > --- a/gcc/config/aarch64/aarch64-option-extensions.def
> > +++ b/gcc/config/aarch64/aarch64-option-extensions.def
> > @@ -30,6 +30,13 @@
> >      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 replaced when FLAGS_ON |
> > + FLAG_CANONICAL
> >
> > "can only replaced" -> "can only be replaced"
> >
> > +   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 @@ -43,7 +50,8 @@
> >      "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,61 +59,71 @@ 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, "")
> >
> >   #undef AARCH64_OPT_EXTENSION
> > diff --git a/gcc/config/aarch64/driver-aarch64.c
> > b/gcc/config/aarch64/driver-aarch64.c
> > index
> >
> 4e83c7a7679fa683af221fb5f726bc8da339081e..98f9d7959506338bd6a8524500
> a1
> > 68cc22ef5396 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..40d9a05c905eb07103d3b437b
> 5c1
> > 351e8620ab33
> > --- /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..3476febce706b34430682e879
> a4a
> > a3aac8f752db
> > --- /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..4558339f16c19555801899c357
> c5
> > 0cedb23c28b0
> > --- /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..15514bfe93e61e63cbce1262e
> e95
> > 1358cd22d6ce
> > --- /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..b4c0901195ede4fe0dd71fbe0
> 2a4
> > 7c35e9dedbbd
> > --- /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..90a055928a273f06e08124a250
> e3
> > 107ad0704e47
> > --- /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..71a2c8a19058c0ec2554608507
> 65
> > 03d206129e10
> > --- /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..83be1bd7a5c3f2bc8d11a14f2c
> 16
> > 415c6a7056f2
> > --- /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..e3c7cdc54ffb0616877260c562
> 35
> > 4496cfdcb688
> > --- /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!.  */
> >
> >
> >
> 
> --

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2019-02-06 15:08 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-12-17 19:19 [PATCH][GCC][AArch64] Fix command line options canonicalization. (PR target/88530) Tamar Christina
2019-01-10 17:15 ` Kyrill Tkachov
2019-01-15 17:13   ` Tamar Christina
2019-01-30 14:06     ` Tamar Christina
2019-02-06 15:08       ` Tamar Christina

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).