public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* [PATCH 2/5] [AARCH64] Change IMP and PART over to integers from strings.
  2015-11-17 22:10 [PATCH 0/5] Add support -mcpu=thunderxt88pass1 and needed changes to support that Andrew Pinski
@ 2015-11-17 22:10 ` Andrew Pinski
  2015-11-20 11:07   ` Kyrill Tkachov
  2015-11-25 10:35   ` James Greenhalgh
  2015-11-17 22:10 ` [PATCH 4/5] {AARCH64] Add comment for the company's cores Andrew Pinski
                   ` (4 subsequent siblings)
  5 siblings, 2 replies; 19+ messages in thread
From: Andrew Pinski @ 2015-11-17 22:10 UTC (permalink / raw)
  To: gcc-patches; +Cc: Andrew Pinski


Because the imp and parts are really integer rather than strings, this patch
moves the comparisons to be integer.  Also allows saving around integers are
easier than doing string comparisons.  This allows for the next change.

The way I store BIG.little is (big<<12)|little as each part num is only 12bits
long.  So it would be nice if someone could test -mpu=native on a big.little
system to make sure it works still.

OK?  Bootstrapped and tested on aarch64-linux-gnu with no regressions.

Thanks,
Andrew Pinski



* config/aarch64/aarch64-cores.def: Rewrite so IMP and PART are integer constants.
* config/aarch64/driver-aarch64.c (struct aarch64_core_data): Change implementer_id to unsigned char.
Change part_no to unsigned int.
(AARCH64_BIG_LITTLE): New define.
(INVALID_IMP): New define.
(INVALID_CORE): New define.
(cpu_data): Change the last element's implementer_id and part_no to integers.
(valid_bL_string_p): Rewrite to ..
(valid_bL_core_p): this for integers instead of strings.
(parse_field): New function.
(contains_string_p): Rewrite to ...
(contains_core_p): this for integers and only for the part_no.
(host_detect_local_cpu): Rewrite handling of implementation and part num to be integers;
simplifying the code.
---
 gcc/config/aarch64/aarch64-cores.def | 25 +++++-----
 gcc/config/aarch64/driver-aarch64.c  | 90 ++++++++++++++++++++----------------
 2 files changed, 62 insertions(+), 53 deletions(-)

diff --git a/gcc/config/aarch64/aarch64-cores.def b/gcc/config/aarch64/aarch64-cores.def
index 0b456f7..798f3e3 100644
--- a/gcc/config/aarch64/aarch64-cores.def
+++ b/gcc/config/aarch64/aarch64-cores.def
@@ -33,25 +33,26 @@
    This need not include flags implied by the architecture.
    COSTS is the name of the rtx_costs routine to use.
    IMP is the implementer ID of the CPU vendor.  On a GNU/Linux system it can
-   be found in /proc/cpuinfo.
+   be found in /proc/cpuinfo.  There is a list in the ARM ARM.
    PART is the part number of the CPU.  On a GNU/Linux system it can be found
-   in /proc/cpuinfo.  For big.LITTLE systems this should have the form at of
-   "<big core part number>.<LITTLE core part number>".  */
+   in /proc/cpuinfo.  For big.LITTLE systems this should use the macro AARCH64_BIG_LITTLE
+   where the big part number comes as the first arugment to the macro and little is the
+   second.  */
 
 /* V8 Architecture Processors.  */
 
-AARCH64_CORE("cortex-a53",  cortexa53, cortexa53, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC, cortexa53, "0x41", "0xd03")
-AARCH64_CORE("cortex-a57",  cortexa57, cortexa57, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC, cortexa57, "0x41", "0xd07")
-AARCH64_CORE("cortex-a72",  cortexa72, cortexa57, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC, cortexa72, "0x41", "0xd08")
-AARCH64_CORE("exynos-m1",   exynosm1,  cortexa57, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC | AARCH64_FL_CRYPTO, cortexa72, "0x53", "0x001")
-AARCH64_CORE("qdf24xx",     qdf24xx,   cortexa57, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC | AARCH64_FL_CRYPTO, cortexa57, "0x51", "0x800")
-AARCH64_CORE("thunderx",    thunderx,  thunderx,  8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC | AARCH64_FL_CRYPTO, thunderx,  "0x43", "0x0a1")
-AARCH64_CORE("xgene1",      xgene1,    xgene1,    8A,  AARCH64_FL_FOR_ARCH8, xgene1, "0x50", "0x000")
+AARCH64_CORE("cortex-a53",  cortexa53, cortexa53, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC, cortexa53, 0x41, 0xd03)
+AARCH64_CORE("cortex-a57",  cortexa57, cortexa57, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC, cortexa57, 0x41, 0xd07)
+AARCH64_CORE("cortex-a72",  cortexa72, cortexa57, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC, cortexa72, 0x41, 0xd08)
+AARCH64_CORE("exynos-m1",   exynosm1,  cortexa57, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC | AARCH64_FL_CRYPTO, cortexa72, 0x53, 0x001)
+AARCH64_CORE("qdf24xx",     qdf24xx,   cortexa57, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC | AARCH64_FL_CRYPTO, cortexa57, 0x51, 0x800)
+AARCH64_CORE("thunderx",    thunderx,  thunderx,  8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC | AARCH64_FL_CRYPTO, thunderx,  0x43, 0x0a1)
+AARCH64_CORE("xgene1",      xgene1,    xgene1,    8A,  AARCH64_FL_FOR_ARCH8, xgene1, 0x50, 0x000)
 
 /* V8 big.LITTLE implementations.  */
 
-AARCH64_CORE("cortex-a57.cortex-a53",  cortexa57cortexa53, cortexa53, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC, cortexa57, "0x41", "0xd07.0xd03")
-AARCH64_CORE("cortex-a72.cortex-a53",  cortexa72cortexa53, cortexa53, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC, cortexa72, "0x41", "0xd08.0xd03")
+AARCH64_CORE("cortex-a57.cortex-a53",  cortexa57cortexa53, cortexa53, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC, cortexa57, 0x41, AARCH64_BIG_LITTLE(0xd07, 0xd03))
+AARCH64_CORE("cortex-a72.cortex-a53",  cortexa72cortexa53, cortexa53, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC, cortexa72, 0x41, AARCH64_BIG_LITTLE(0xd08, 0xd03))
 
 
 #undef AARCH64_CORE
diff --git a/gcc/config/aarch64/driver-aarch64.c b/gcc/config/aarch64/driver-aarch64.c
index d03654c..92388a9 100644
--- a/gcc/config/aarch64/driver-aarch64.c
+++ b/gcc/config/aarch64/driver-aarch64.c
@@ -37,18 +37,23 @@ static struct arch_extension ext_to_feat_string[] =
 struct aarch64_core_data
 {
   const char* name;
-  const char* arch;
-  const char* implementer_id;
-  const char* part_no;
+  const char *arch;
+  unsigned char implementer_id; /* Exactly 8 bits */
+  unsigned int part_no; /* 12 bits + 12 bits */
 };
 
+#define AARCH64_BIG_LITTLE(BIG, LITTLE) \
+  (((BIG)&0xFFFu) << 12 | ((LITTLE) & 0xFFFu))
+#define INVALID_IMP ((unsigned char) -1)
+#define INVALID_CORE ((unsigned)-1)
+
 #define AARCH64_CORE(CORE_NAME, CORE_IDENT, SCHED, ARCH, FLAGS, COSTS, IMP, PART) \
   { CORE_NAME, #ARCH, IMP, PART },
 
 static struct aarch64_core_data cpu_data [] =
 {
 #include "aarch64-cores.def"
-  { NULL, NULL, NULL, NULL }
+  { NULL, NULL, INVALID_IMP, INVALID_CORE }
 };
 
 
@@ -91,27 +96,35 @@ get_arch_name_from_id (const char* id)
    should return true.  */
 
 static bool
-valid_bL_string_p (const char** core, const char* bL_string)
+valid_bL_core_p (unsigned int *core, unsigned int bL_core)
 {
-  return strstr (bL_string, core[0]) != NULL
-         && strstr (bL_string, core[1]) != NULL;
+  return AARCH64_BIG_LITTLE (core[0], core[1]) == bL_core;
 }
 
-/*  Return true iff ARR contains STR in one of its two elements.  */
 
-static bool
-contains_string_p (const char** arr, const char* str)
+/* Returns the integer that is after ':' for the field. */
+static unsigned parse_field (const char *field)
 {
-  bool res = false;
+  const char *rest = strchr (field, ':');
+  char *after;
+  unsigned fint = strtol (rest+1, &after, 16);
+  if (after == rest+1)
+    return -1;
+  return fint;
+}
+
+/*  Return true iff ARR contains CORE, in either of the two elements. */
 
-  if (arr[0] != NULL)
+static bool
+contains_core_p (unsigned *arr, unsigned core)
+{
+  if (arr[0] != INVALID_CORE)
     {
-      res = strstr (arr[0], str) != NULL;
-      if (res)
-        return res;
+      if (arr[0] == core)
+        return true;
 
-      if (arr[1] != NULL)
-        return strstr (arr[1], str) != NULL;
+      if (arr[1] != INVALID_CORE)
+        return arr[1] == core;
     }
 
   return false;
@@ -146,10 +159,9 @@ host_detect_local_cpu (int argc, const char **argv)
   bool cpu = false;
   unsigned int i = 0;
   unsigned int core_idx = 0;
-  const char* imps[2] = { NULL, NULL };
-  const char* cores[2] = { NULL, NULL };
+  unsigned char imp = INVALID_IMP;
+  unsigned int cores[2] = { INVALID_CORE, INVALID_CORE };
   unsigned int n_cores = 0;
-  unsigned int n_imps = 0;
   bool processed_exts = false;
   const char *ext_string = "";
 
@@ -180,30 +192,28 @@ host_detect_local_cpu (int argc, const char **argv)
     {
       if (strstr (buf, "implementer") != NULL)
 	{
-	  for (i = 0; cpu_data[i].name != NULL; i++)
-	    if (strstr (buf, cpu_data[i].implementer_id) != NULL
-                && !contains_string_p (imps, cpu_data[i].implementer_id))
-	      {
-                if (n_imps == 2)
-                  goto not_found;
-
-                imps[n_imps++] = cpu_data[i].implementer_id;
-
-                break;
-	      }
-          continue;
+	  unsigned cimp = parse_field (buf);
+	  if (cimp == INVALID_IMP)
+	    goto not_found;
+
+	  if (imp == INVALID_IMP)
+	    imp = cimp;
+	  /* BIG.little implementers are always equal. */
+	  else if (imp != cimp)
+	    goto not_found;
 	}
 
       if (strstr (buf, "part") != NULL)
 	{
+	  unsigned ccore = parse_field (buf);
 	  for (i = 0; cpu_data[i].name != NULL; i++)
-	    if (strstr (buf, cpu_data[i].part_no) != NULL
-                && !contains_string_p (cores, cpu_data[i].part_no))
+	    if (ccore == cpu_data[i].part_no
+                && !contains_core_p (cores, ccore))
 	      {
                 if (n_cores == 2)
                   goto not_found;
 
-                cores[n_cores++] = cpu_data[i].part_no;
+                cores[n_cores++] = ccore;
 	        core_idx = i;
 	        arch_id = cpu_data[i].arch;
 	        break;
@@ -240,7 +250,7 @@ host_detect_local_cpu (int argc, const char **argv)
   f = NULL;
 
   /* Weird cpuinfo format that we don't know how to handle.  */
-  if (n_cores == 0 || n_cores > 2 || n_imps != 1)
+  if (n_cores == 0 || n_cores > 2 || imp == INVALID_IMP)
     goto not_found;
 
   if (arch && !arch_id)
@@ -261,9 +271,8 @@ host_detect_local_cpu (int argc, const char **argv)
     {
       for (i = 0; cpu_data[i].name != NULL; i++)
         {
-          if (strchr (cpu_data[i].part_no, '.') != NULL
-              && strncmp (cpu_data[i].implementer_id, imps[0], strlen (imps[0]) - 1) == 0
-              && valid_bL_string_p (cores, cpu_data[i].part_no))
+          if (cpu_data[i].implementer_id == imp
+              && valid_bL_core_p (cores, cpu_data[i].part_no))
             {
               res = concat ("-m", cpu ? "cpu" : "tune", "=", cpu_data[i].name, NULL);
               break;
@@ -275,8 +284,7 @@ host_detect_local_cpu (int argc, const char **argv)
   /* The simple, non-big.LITTLE case.  */
   else
     {
-      if (strncmp (cpu_data[core_idx].implementer_id, imps[0],
-                   strlen (imps[0]) - 1) != 0)
+      if (cpu_data[core_idx].implementer_id != imp)
         goto not_found;
 
       res = concat ("-m", cpu ? "cpu" : "tune", "=",
-- 
1.9.1

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

* [PATCH 3/5] [AARCH64] Fix part num and implement indendent.
  2015-11-17 22:10 [PATCH 0/5] Add support -mcpu=thunderxt88pass1 and needed changes to support that Andrew Pinski
  2015-11-17 22:10 ` [PATCH 2/5] [AARCH64] Change IMP and PART over to integers from strings Andrew Pinski
  2015-11-17 22:10 ` [PATCH 4/5] {AARCH64] Add comment for the company's cores Andrew Pinski
@ 2015-11-17 22:10 ` Andrew Pinski
  2015-11-17 22:11 ` [PATCH 1/5] [AARCH64]: Move #undef into .def files Andrew Pinski
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 19+ messages in thread
From: Andrew Pinski @ 2015-11-17 22:10 UTC (permalink / raw)
  To: gcc-patches; +Cc: Andrew Pinski


The way the current code was written assumes all cores have an unique part
num which is not true.  What they have is an unique pair of implementer and
part num.  This changes the code to look up that pair after the parsing
of the two is done.

Someone should test this on a big.little target too just to make sure
the parsing is done correctly as I don't have access to one off hand.

OK?  Bootstrapped and tested on aarch64-linux-gnu with no regressions.

Thanks,
Andrew Pinski


* config/aarch64/driver-aarch64.c (host_detect_local_cpu):
Rewrite handling of part num to handle the case where
multiple implementers share the same part num.
---
 gcc/config/aarch64/driver-aarch64.c | 46 ++++++++++++++++++++++---------------
 1 file changed, 28 insertions(+), 18 deletions(-)

diff --git a/gcc/config/aarch64/driver-aarch64.c b/gcc/config/aarch64/driver-aarch64.c
index 92388a9..ea1e856 100644
--- a/gcc/config/aarch64/driver-aarch64.c
+++ b/gcc/config/aarch64/driver-aarch64.c
@@ -158,7 +158,7 @@ host_detect_local_cpu (int argc, const char **argv)
   bool tune = false;
   bool cpu = false;
   unsigned int i = 0;
-  unsigned int core_idx = 0;
+  int core_idx = -1;
   unsigned char imp = INVALID_IMP;
   unsigned int cores[2] = { INVALID_CORE, INVALID_CORE };
   unsigned int n_cores = 0;
@@ -206,18 +206,13 @@ host_detect_local_cpu (int argc, const char **argv)
       if (strstr (buf, "part") != NULL)
 	{
 	  unsigned ccore = parse_field (buf);
-	  for (i = 0; cpu_data[i].name != NULL; i++)
-	    if (ccore == cpu_data[i].part_no
-                && !contains_core_p (cores, ccore))
-	      {
-                if (n_cores == 2)
-                  goto not_found;
-
-                cores[n_cores++] = ccore;
-	        core_idx = i;
-	        arch_id = cpu_data[i].arch;
-	        break;
-	      }
+	  if (!contains_core_p (cores, ccore))
+	    {
+              if (n_cores == 2)
+                goto not_found;
+
+              cores[n_cores++] = ccore;
+	    }
           continue;
         }
       if (!tune && !processed_exts && strstr (buf, "Features") != NULL)
@@ -253,11 +248,19 @@ host_detect_local_cpu (int argc, const char **argv)
   if (n_cores == 0 || n_cores > 2 || imp == INVALID_IMP)
     goto not_found;
 
-  if (arch && !arch_id)
-    goto not_found;
-
   if (arch)
     {
+      /* Search for one of the cores in the list. */
+      for (i = 0; cpu_data[i].name != NULL; i++)
+	if (cpu_data[i].implementer_id == imp
+	    && contains_core_p (cores, cpu_data[i].part_no))
+	  {
+	    arch_id = cpu_data[i].arch;
+	    break;
+	  }
+      if (!arch_id)
+	goto not_found;
+
       const char* arch_name = get_arch_name_from_id (arch_id);
 
       /* We got some arch indentifier that's not in aarch64-arches.def?  */
@@ -284,8 +287,15 @@ host_detect_local_cpu (int argc, const char **argv)
   /* The simple, non-big.LITTLE case.  */
   else
     {
-      if (cpu_data[core_idx].implementer_id != imp)
-        goto not_found;
+      for (i = 0; cpu_data[i].name != NULL; i++)
+	if (cores[0] == cpu_data[i].part_no
+	    && cpu_data[i].implementer_id == imp)
+	  {
+	    core_idx = i;
+	    break;
+	  }
+      if (core_idx == -1)
+	goto not_found;
 
       res = concat ("-m", cpu ? "cpu" : "tune", "=",
                       cpu_data[core_idx].name, NULL);
-- 
1.9.1

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

* [PATCH 4/5] {AARCH64] Add comment for the company's cores.
  2015-11-17 22:10 [PATCH 0/5] Add support -mcpu=thunderxt88pass1 and needed changes to support that Andrew Pinski
  2015-11-17 22:10 ` [PATCH 2/5] [AARCH64] Change IMP and PART over to integers from strings Andrew Pinski
@ 2015-11-17 22:10 ` Andrew Pinski
  2015-11-17 22:10 ` [PATCH 3/5] [AARCH64] Fix part num and implement indendent Andrew Pinski
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 19+ messages in thread
From: Andrew Pinski @ 2015-11-17 22:10 UTC (permalink / raw)
  To: gcc-patches; +Cc: Andrew Pinski

Just to make it easier to see which cores blong to which company and
the order clearier, add a comment in front of the cores sections.

OK?

Thanks,
Andrew Pinski


* config/aarch64/aarch64-cores.def: add a comment before each set
of cores.
---
 gcc/config/aarch64/aarch64-cores.def | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/gcc/config/aarch64/aarch64-cores.def b/gcc/config/aarch64/aarch64-cores.def
index 798f3e3..05ee525 100644
--- a/gcc/config/aarch64/aarch64-cores.def
+++ b/gcc/config/aarch64/aarch64-cores.def
@@ -41,12 +41,21 @@
 
 /* V8 Architecture Processors.  */
 
+/* ARM cores. */
 AARCH64_CORE("cortex-a53",  cortexa53, cortexa53, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC, cortexa53, 0x41, 0xd03)
 AARCH64_CORE("cortex-a57",  cortexa57, cortexa57, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC, cortexa57, 0x41, 0xd07)
 AARCH64_CORE("cortex-a72",  cortexa72, cortexa57, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC, cortexa72, 0x41, 0xd08)
+
+/* Samsung cores */
 AARCH64_CORE("exynos-m1",   exynosm1,  cortexa57, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC | AARCH64_FL_CRYPTO, cortexa72, 0x53, 0x001)
+
+/* Qualcomm cores */
 AARCH64_CORE("qdf24xx",     qdf24xx,   cortexa57, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC | AARCH64_FL_CRYPTO, cortexa57, 0x51, 0x800)
+
+/* Cavium cores */
 AARCH64_CORE("thunderx",    thunderx,  thunderx,  8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC | AARCH64_FL_CRYPTO, thunderx,  0x43, 0x0a1)
+
+/* APM cores */
 AARCH64_CORE("xgene1",      xgene1,    xgene1,    8A,  AARCH64_FL_FOR_ARCH8, xgene1, 0x50, 0x000)
 
 /* V8 big.LITTLE implementations.  */
-- 
1.9.1

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

* [PATCH 0/5] Add support -mcpu=thunderxt88pass1 and needed changes to support that
@ 2015-11-17 22:10 Andrew Pinski
  2015-11-17 22:10 ` [PATCH 2/5] [AARCH64] Change IMP and PART over to integers from strings Andrew Pinski
                   ` (5 more replies)
  0 siblings, 6 replies; 19+ messages in thread
From: Andrew Pinski @ 2015-11-17 22:10 UTC (permalink / raw)
  To: gcc-patches; +Cc: Andrew Pinski


To Add support for -mcpu=thunderxt88pass1, I needed to fix up a few
things in the support for -mcpu=native.  First was I wanted to do the same
cleanup that was done for some of the other .def files and move the
#undef into the .def files instead of the .h files.
Second to make it easier to understand the implemention_id and part numbers
are really numbers, move them over to use integer to compare against
instead of strings which allows for easier comparisons later on.
Third fix the way we compare imp and part num; that is instead of finding
the part number and then comparing the imp, we find both numbers first
and then search for the ones that match.
Add a comment to the aarch64-cores.def file in front of each group of cores
to signify the groups of company's cores.
And all of this allows for adding variant for the check of the cores
which allows for thunderxt88pass1 to be added.

The reason why thunderxt88pass1 is seperate from thunderx is because
thunderx is changed to be an ARMv8.1 arch core while thunderxt88pass1
is still an ARMv8 arch core.

I tested each of these patches seperately.

Ok for the trunk even though I missed out on stage 1?

Thanks,
Andrew

Andrew Pinski (5):
  [AARCH64]: Move #undef into .def files.
  [AARCH64] Change IMP and PART over to integers from strings.
  [AARCH64] Fix part num and implement indendent.
  {AARCH64] Add comment for the company's cores.
  [AARCH64] Add variant support to -m*=native and add thunderxt88pass1.

 gcc/common/config/aarch64/aarch64-common.c       |   5 +-
 gcc/config/aarch64/aarch64-arches.def            |   1 +
 gcc/config/aarch64/aarch64-cores.def             |  43 ++++--
 gcc/config/aarch64/aarch64-fusion-pairs.def      |   1 +
 gcc/config/aarch64/aarch64-option-extensions.def |   2 +
 gcc/config/aarch64/aarch64-opts.h                |   4 +-
 gcc/config/aarch64/aarch64-protos.h              |   4 -
 gcc/config/aarch64/aarch64-tune.md               |   2 +-
 gcc/config/aarch64/aarch64-tuning-flags.def      |   1 +
 gcc/config/aarch64/aarch64.c                     |   7 +-
 gcc/config/aarch64/aarch64.h                     |   3 +-
 gcc/config/aarch64/driver-aarch64.c              | 169 +++++++++++++----------
 12 files changed, 136 insertions(+), 106 deletions(-)

-- 
1.9.1

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

* [PATCH 5/5] [AARCH64] Add variant support to -m*=native and add thunderxt88pass1.
  2015-11-17 22:10 [PATCH 0/5] Add support -mcpu=thunderxt88pass1 and needed changes to support that Andrew Pinski
                   ` (3 preceding siblings ...)
  2015-11-17 22:11 ` [PATCH 1/5] [AARCH64]: Move #undef into .def files Andrew Pinski
@ 2015-11-17 22:11 ` Andrew Pinski
  2015-11-17 22:55   ` Joseph Myers
  2015-11-19 12:08 ` [PATCH 0/5] Add support -mcpu=thunderxt88pass1 and needed changes to support that Kyrill Tkachov
  5 siblings, 1 reply; 19+ messages in thread
From: Andrew Pinski @ 2015-11-17 22:11 UTC (permalink / raw)
  To: gcc-patches; +Cc: Andrew Pinski

Since ThunderX T88 pass 1 (variant 0) is a ARMv8 part while pass 2 (variant 1)
is an ARMv8.1 part, I needed to add detecting of the variant also for this
difference. Also I simplify a little bit and combined the single core and
arch detecting cases so it would be easier to add variant.

OK?  Bootstrapped and tested on aarch64-linux-gnu with no regressions.
Tested -mcpu=native on both T88 pass 1 and T88 pass 2 to make sure it is
deecting the two seperately.

Thanks,
Andrew Pinski


* config/aarch64/aarch64-cores.def: Add -1 as the variant to all of the cores.
(thunderxt88pass1): New core.
* config/aarch64/driver-aarch64.c (struct aarch64_core_data): Add variant field.
(ALL_VARIANTS): New define.
(AARCH64_CORE): Support VARIANT operand.
(cpu_data): Likewise.
(host_detect_local_cpu): Parse variant field of /proc/cpuinfo.  Combine the arch
and single core case and support variant searching.
* common/config/aarch64/aarch64-common.c (AARCH64_CORE): Add VARIANT operand.
* config/aarch64/aarch64-opts.h (AARCH64_CORE): Likewise.
* config/aarch64/aarch64.c (AARCH64_CORE): Likewise.
* config/aarch64/aarch64.h (AARCH64_CORE): Likewise.
* config/aarch64/aarch64-tune.md: Regernate.
---
 gcc/common/config/aarch64/aarch64-common.c |  2 +-
 gcc/config/aarch64/aarch64-cores.def       | 27 ++++++-----
 gcc/config/aarch64/aarch64-opts.h          |  2 +-
 gcc/config/aarch64/aarch64-tune.md         |  2 +-
 gcc/config/aarch64/aarch64.c               |  2 +-
 gcc/config/aarch64/aarch64.h               |  2 +-
 gcc/config/aarch64/driver-aarch64.c        | 78 ++++++++++++++++--------------
 7 files changed, 64 insertions(+), 51 deletions(-)

diff --git a/gcc/common/config/aarch64/aarch64-common.c b/gcc/common/config/aarch64/aarch64-common.c
index e312bbc..f6fd7e7 100644
--- a/gcc/common/config/aarch64/aarch64-common.c
+++ b/gcc/common/config/aarch64/aarch64-common.c
@@ -141,7 +141,7 @@ struct arch_to_arch_name
    the default set of architectural feature flags they support.  */
 static const struct processor_name_to_arch all_cores[] =
 {
-#define AARCH64_CORE(NAME, X, IDENT, ARCH_IDENT, FLAGS, COSTS, IMP, PART) \
+#define AARCH64_CORE(NAME, X, IDENT, ARCH_IDENT, FLAGS, COSTS, IMP, PART, VARIANT) \
   {NAME, AARCH64_ARCH_##ARCH_IDENT, FLAGS},
 #include "config/aarch64/aarch64-cores.def"
   {"generic", AARCH64_ARCH_8A, AARCH64_FL_FOR_ARCH8},
diff --git a/gcc/config/aarch64/aarch64-cores.def b/gcc/config/aarch64/aarch64-cores.def
index 05ee525..52a9906 100644
--- a/gcc/config/aarch64/aarch64-cores.def
+++ b/gcc/config/aarch64/aarch64-cores.def
@@ -21,7 +21,7 @@
 
    Before using #include to read this file, define a macro:
 
-      AARCH64_CORE(CORE_NAME, CORE_IDENT, SCHEDULER_IDENT, ARCH_IDENT, FLAGS, COSTS, IMP, PART)
+      AARCH64_CORE(CORE_NAME, CORE_IDENT, SCHEDULER_IDENT, ARCH_IDENT, FLAGS, COSTS, IMP, PART, VARIANT)
 
    The CORE_NAME is the name of the core, represented as a string constant.
    The CORE_IDENT is the name of the core, represented as an identifier.
@@ -37,31 +37,36 @@
    PART is the part number of the CPU.  On a GNU/Linux system it can be found
    in /proc/cpuinfo.  For big.LITTLE systems this should use the macro AARCH64_BIG_LITTLE
    where the big part number comes as the first arugment to the macro and little is the
-   second.  */
+   second.
+   VARIANT is the variant of the CPU.  In a GNU/Linux system it can found
+   in /proc/cpuinfo.  If this is -1, this means it can match any variant.  */
 
 /* V8 Architecture Processors.  */
 
 /* ARM cores. */
-AARCH64_CORE("cortex-a53",  cortexa53, cortexa53, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC, cortexa53, 0x41, 0xd03)
-AARCH64_CORE("cortex-a57",  cortexa57, cortexa57, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC, cortexa57, 0x41, 0xd07)
-AARCH64_CORE("cortex-a72",  cortexa72, cortexa57, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC, cortexa72, 0x41, 0xd08)
+AARCH64_CORE("cortex-a53",  cortexa53, cortexa53, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC, cortexa53, 0x41, 0xd03, -1)
+AARCH64_CORE("cortex-a57",  cortexa57, cortexa57, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC, cortexa57, 0x41, 0xd07, -1)
+AARCH64_CORE("cortex-a72",  cortexa72, cortexa57, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC, cortexa72, 0x41, 0xd08, -1)
 
 /* Samsung cores */
-AARCH64_CORE("exynos-m1",   exynosm1,  cortexa57, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC | AARCH64_FL_CRYPTO, cortexa72, 0x53, 0x001)
+AARCH64_CORE("exynos-m1",   exynosm1,  cortexa57, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC | AARCH64_FL_CRYPTO, cortexa72, 0x53, 0x001, -1)
 
 /* Qualcomm cores */
-AARCH64_CORE("qdf24xx",     qdf24xx,   cortexa57, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC | AARCH64_FL_CRYPTO, cortexa57, 0x51, 0x800)
+AARCH64_CORE("qdf24xx",     qdf24xx,   cortexa57, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC | AARCH64_FL_CRYPTO, cortexa57, 0x51, 0x800, -1)
 
 /* Cavium cores */
-AARCH64_CORE("thunderx",    thunderx,  thunderx,  8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC | AARCH64_FL_CRYPTO, thunderx,  0x43, 0x0a1)
+/* ThunderX T88 pass 1 is 8.0-a arch. */
+AARCH64_CORE("thunderxt88pass1", thunderxt88pass1,  thunderx,  8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC | AARCH64_FL_CRYPTO, thunderx,  0x43, 0x0a1, 0)
+/* ThunderX T88 pass 2 and on is 8.1-a arch. */
+AARCH64_CORE("thunderx",    thunderx,  thunderx,  8_1A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC | AARCH64_FL_CRYPTO | AARCH64_FL_LSE, thunderx,  0x43, 0x0a1, -1)
 
 /* APM cores */
-AARCH64_CORE("xgene1",      xgene1,    xgene1,    8A,  AARCH64_FL_FOR_ARCH8, xgene1, 0x50, 0x000)
+AARCH64_CORE("xgene1",      xgene1,    xgene1,    8A,  AARCH64_FL_FOR_ARCH8, xgene1, 0x50, 0x000, -1)
 
 /* V8 big.LITTLE implementations.  */
 
-AARCH64_CORE("cortex-a57.cortex-a53",  cortexa57cortexa53, cortexa53, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC, cortexa57, 0x41, AARCH64_BIG_LITTLE(0xd07, 0xd03))
-AARCH64_CORE("cortex-a72.cortex-a53",  cortexa72cortexa53, cortexa53, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC, cortexa72, 0x41, AARCH64_BIG_LITTLE(0xd08, 0xd03))
+AARCH64_CORE("cortex-a57.cortex-a53",  cortexa57cortexa53, cortexa53, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC, cortexa57, 0x41, AARCH64_BIG_LITTLE(0xd07, 0xd03), -1)
+AARCH64_CORE("cortex-a72.cortex-a53",  cortexa72cortexa53, cortexa53, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC, cortexa72, 0x41, AARCH64_BIG_LITTLE(0xd08, 0xd03), -1)
 
 
 #undef AARCH64_CORE
diff --git a/gcc/config/aarch64/aarch64-opts.h b/gcc/config/aarch64/aarch64-opts.h
index 5534867..c66cd72 100644
--- a/gcc/config/aarch64/aarch64-opts.h
+++ b/gcc/config/aarch64/aarch64-opts.h
@@ -25,7 +25,7 @@
 /* The various cores that implement AArch64.  */
 enum aarch64_processor
 {
-#define AARCH64_CORE(NAME, INTERNAL_IDENT, SCHED, ARCH, FLAGS, COSTS, IMP, PART) \
+#define AARCH64_CORE(NAME, INTERNAL_IDENT, SCHED, ARCH, FLAGS, COSTS, IMP, PART, VARIANT) \
   INTERNAL_IDENT,
 #include "aarch64-cores.def"
   /* Used to indicate that no processor has been specified.  */
diff --git a/gcc/config/aarch64/aarch64-tune.md b/gcc/config/aarch64/aarch64-tune.md
index c65a124..28b573e 100644
--- a/gcc/config/aarch64/aarch64-tune.md
+++ b/gcc/config/aarch64/aarch64-tune.md
@@ -1,5 +1,5 @@
 ;; -*- buffer-read-only: t -*-
 ;; Generated automatically by gentune.sh from aarch64-cores.def
 (define_attr "tune"
-	"cortexa53,cortexa57,cortexa72,exynosm1,qdf24xx,thunderx,xgene1,cortexa57cortexa53,cortexa72cortexa53"
+	"cortexa53,cortexa57,cortexa72,exynosm1,qdf24xx,thunderxt88pass1,thunderx,xgene1,cortexa57cortexa53,cortexa72cortexa53"
 	(const (symbol_ref "((enum attr_tune) aarch64_tune)")))
diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index a5971a1..d244846 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -526,7 +526,7 @@ static const struct processor all_architectures[] =
 /* Processor cores implementing AArch64.  */
 static const struct processor all_cores[] =
 {
-#define AARCH64_CORE(NAME, IDENT, SCHED, ARCH, FLAGS, COSTS, IMP, PART) \
+#define AARCH64_CORE(NAME, IDENT, SCHED, ARCH, FLAGS, COSTS, IMP, PART, VARIANT) \
   {NAME, IDENT, SCHED, AARCH64_ARCH_##ARCH,				\
   all_architectures[AARCH64_ARCH_##ARCH].architecture_version,	\
   FLAGS, &COSTS##_tunings},
diff --git a/gcc/config/aarch64/aarch64.h b/gcc/config/aarch64/aarch64.h
index c6582a4..6f06369 100644
--- a/gcc/config/aarch64/aarch64.h
+++ b/gcc/config/aarch64/aarch64.h
@@ -464,7 +464,7 @@ enum reg_class
 
 enum target_cpus
 {
-#define AARCH64_CORE(NAME, INTERNAL_IDENT, SCHED, ARCH, FLAGS, COSTS, IMP, PART) \
+#define AARCH64_CORE(NAME, INTERNAL_IDENT, SCHED, ARCH, FLAGS, COSTS, IMP, PART, VARIANT) \
   TARGET_CPU_##INTERNAL_IDENT,
 #include "aarch64-cores.def"
   TARGET_CPU_generic
diff --git a/gcc/config/aarch64/driver-aarch64.c b/gcc/config/aarch64/driver-aarch64.c
index ea1e856..b11d914 100644
--- a/gcc/config/aarch64/driver-aarch64.c
+++ b/gcc/config/aarch64/driver-aarch64.c
@@ -40,20 +40,22 @@ struct aarch64_core_data
   const char *arch;
   unsigned char implementer_id; /* Exactly 8 bits */
   unsigned int part_no; /* 12 bits + 12 bits */
+  unsigned variant;
 };
 
 #define AARCH64_BIG_LITTLE(BIG, LITTLE) \
   (((BIG)&0xFFFu) << 12 | ((LITTLE) & 0xFFFu))
 #define INVALID_IMP ((unsigned char) -1)
 #define INVALID_CORE ((unsigned)-1)
+#define ALL_VARIANTS ((unsigned)-1)
 
-#define AARCH64_CORE(CORE_NAME, CORE_IDENT, SCHED, ARCH, FLAGS, COSTS, IMP, PART) \
-  { CORE_NAME, #ARCH, IMP, PART },
+#define AARCH64_CORE(CORE_NAME, CORE_IDENT, SCHED, ARCH, FLAGS, COSTS, IMP, PART, VARIANT) \
+  { CORE_NAME, #ARCH, IMP, PART, VARIANT },
 
 static struct aarch64_core_data cpu_data [] =
 {
 #include "aarch64-cores.def"
-  { NULL, NULL, INVALID_IMP, INVALID_CORE }
+  { NULL, NULL, INVALID_IMP, INVALID_CORE, ALL_VARIANTS }
 };
 
 
@@ -149,7 +151,6 @@ contains_core_p (unsigned *arr, unsigned core)
 const char *
 host_detect_local_cpu (int argc, const char **argv)
 {
-  const char *arch_id = NULL;
   const char *res = NULL;
   static const int num_exts = ARRAY_SIZE (ext_to_feat_string);
   char buf[128];
@@ -158,10 +159,11 @@ host_detect_local_cpu (int argc, const char **argv)
   bool tune = false;
   bool cpu = false;
   unsigned int i = 0;
-  int core_idx = -1;
   unsigned char imp = INVALID_IMP;
   unsigned int cores[2] = { INVALID_CORE, INVALID_CORE };
   unsigned int n_cores = 0;
+  unsigned int variants[2] = { ALL_VARIANTS, ALL_VARIANTS };
+  unsigned int n_variants = 0;
   bool processed_exts = false;
   const char *ext_string = "";
 
@@ -203,6 +205,19 @@ host_detect_local_cpu (int argc, const char **argv)
 	    goto not_found;
 	}
 
+      if (strstr (buf, "variant") != NULL)
+	{
+	  unsigned cvariant = parse_field (buf);
+	  if (!contains_core_p (variants, cvariant))
+	    {
+              if (n_variants == 2)
+                goto not_found;
+
+              variants[n_variants++] = cvariant;
+	    }
+          continue;
+        }
+
       if (strstr (buf, "part") != NULL)
 	{
 	  unsigned ccore = parse_field (buf);
@@ -245,32 +260,41 @@ host_detect_local_cpu (int argc, const char **argv)
   f = NULL;
 
   /* Weird cpuinfo format that we don't know how to handle.  */
-  if (n_cores == 0 || n_cores > 2 || imp == INVALID_IMP)
+  if (n_cores == 0 || n_cores > 2
+      || (n_cores == 1 && n_variants != 1)
+      || imp == INVALID_IMP)
     goto not_found;
 
-  if (arch)
+
+  /* Simple case, one core type or just looking for the arch. */
+  if (n_cores == 1 || arch)
     {
       /* Search for one of the cores in the list. */
       for (i = 0; cpu_data[i].name != NULL; i++)
 	if (cpu_data[i].implementer_id == imp
-	    && contains_core_p (cores, cpu_data[i].part_no))
-	  {
-	    arch_id = cpu_data[i].arch;
-	    break;
-	  }
-      if (!arch_id)
+	    && cores[0] == cpu_data[i].part_no
+	    && (cpu_data[i].variant == ALL_VARIANTS
+	        || variants[0] == cpu_data[i].variant))
+	  break;
+      if (cpu_data[i].name == NULL)
 	goto not_found;
 
-      const char* arch_name = get_arch_name_from_id (arch_id);
+      if (arch)
+	{
+	  const char* arch_name = get_arch_name_from_id (cpu_data[i].arch);
 
-      /* We got some arch indentifier that's not in aarch64-arches.def?  */
-      if (!arch_name)
-        goto not_found;
+	  /* We got some arch indentifier that's not in aarch64-arches.def?  */
+	  if (!arch_name)
+	    goto not_found;
 
-      res = concat ("-march=", arch_name, NULL);
+	  res = concat ("-march=", arch_name, NULL);
+	}
+      else
+        res = concat ("-m", cpu ? "cpu" : "tune", "=",
+                      cpu_data[i].name, NULL);
     }
   /* We have big.LITTLE.  */
-  else if (n_cores == 2)
+  else
     {
       for (i = 0; cpu_data[i].name != NULL; i++)
         {
@@ -284,22 +308,6 @@ host_detect_local_cpu (int argc, const char **argv)
       if (!res)
         goto not_found;
     }
-  /* The simple, non-big.LITTLE case.  */
-  else
-    {
-      for (i = 0; cpu_data[i].name != NULL; i++)
-	if (cores[0] == cpu_data[i].part_no
-	    && cpu_data[i].implementer_id == imp)
-	  {
-	    core_idx = i;
-	    break;
-	  }
-      if (core_idx == -1)
-	goto not_found;
-
-      res = concat ("-m", cpu ? "cpu" : "tune", "=",
-                      cpu_data[core_idx].name, NULL);
-    }
 
   if (tune)
     return res;
-- 
1.9.1

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

* [PATCH 1/5] [AARCH64]: Move #undef into .def files.
  2015-11-17 22:10 [PATCH 0/5] Add support -mcpu=thunderxt88pass1 and needed changes to support that Andrew Pinski
                   ` (2 preceding siblings ...)
  2015-11-17 22:10 ` [PATCH 3/5] [AARCH64] Fix part num and implement indendent Andrew Pinski
@ 2015-11-17 22:11 ` Andrew Pinski
  2015-11-18 12:29   ` Marcus Shawcroft
  2015-11-17 22:11 ` [PATCH 5/5] [AARCH64] Add variant support to -m*=native and add thunderxt88pass1 Andrew Pinski
  2015-11-19 12:08 ` [PATCH 0/5] Add support -mcpu=thunderxt88pass1 and needed changes to support that Kyrill Tkachov
  5 siblings, 1 reply; 19+ messages in thread
From: Andrew Pinski @ 2015-11-17 22:11 UTC (permalink / raw)
  To: gcc-patches; +Cc: Andrew Pinski


This moves the #undef from the header files to the .def files like was done
for builtins.def (https://gcc.gnu.org/ml/gcc-patches/2015-11/msg00662.html).

OK?   Bootstrapped and tested on aarch64-linux-gnu with no regressions.

Thanks,
Andrew Pinski

* config/aarch64/aarch64-arches.def (AARCH64_ARCH): #undef at the end.
* config/aarch64/aarch64-cores.def (AARCH64_CORE): Likewise.
* config/aarch64/aarch64-fusion-pairs.def (AARCH64_FUSION_PAIR): Likewise.
* config/aarch64/aarch64-tuning-flags.def (AARCH64_EXTRA_TUNING_OPTION): Likewise.
* config/aarch64/aarch64-opts.h (AARCH64_CORE): Don't #undef here.
(AARCH64_ARCH): Likewise.
* config/aarch64/aarch64-protos.h (AARCH64_FUSION_PAIR): Likewise.
(AARCH64_EXTRA_TUNING_OPTION): Likewise.
* config/aarch64/aarch64.c (AARCH64_FUION_PAIR): Likewise.
(AARCH64_EXTRA_TUNING_OPTION): Likewise.
(AARCH64_ARCH): Likewise.
(AARCH64_CORE): Likewise.
(AARCH64_OPT_EXTENSION): Likewise.
* config/aarch64/aarch64.h (AARCH64_CORE): Likewise.
* config/aarch64/driver-aarch64.c (AARCH64_OPT_EXTENSION): Likewise.
(AARCH64_CORE): Likewise.
(AARCH64_ARCH): Likewise.
* common/config/aarch64/aarch64-common.c: Likewise.
---
 gcc/common/config/aarch64/aarch64-common.c       | 3 ---
 gcc/config/aarch64/aarch64-arches.def            | 1 +
 gcc/config/aarch64/aarch64-cores.def             | 2 ++
 gcc/config/aarch64/aarch64-fusion-pairs.def      | 1 +
 gcc/config/aarch64/aarch64-option-extensions.def | 2 ++
 gcc/config/aarch64/aarch64-opts.h                | 2 --
 gcc/config/aarch64/aarch64-protos.h              | 4 ----
 gcc/config/aarch64/aarch64-tuning-flags.def      | 1 +
 gcc/config/aarch64/aarch64.c                     | 5 -----
 gcc/config/aarch64/aarch64.h                     | 1 -
 gcc/config/aarch64/driver-aarch64.c              | 3 ---
 11 files changed, 7 insertions(+), 18 deletions(-)

diff --git a/gcc/common/config/aarch64/aarch64-common.c b/gcc/common/config/aarch64/aarch64-common.c
index 07c6bba..e312bbc 100644
--- a/gcc/common/config/aarch64/aarch64-common.c
+++ b/gcc/common/config/aarch64/aarch64-common.c
@@ -121,7 +121,6 @@ static const struct aarch64_option_extension all_extensions[] =
 #define AARCH64_OPT_EXTENSION(NAME, FLAGS_ON, FLAGS_OFF, FEATURE_STRING) \
   {NAME, FLAGS_ON, FLAGS_OFF},
 #include "config/aarch64/aarch64-option-extensions.def"
-#undef AARCH64_OPT_EXTENSION
   {NULL, 0, 0}
 };
 
@@ -145,7 +144,6 @@ static const struct processor_name_to_arch all_cores[] =
 #define AARCH64_CORE(NAME, X, IDENT, ARCH_IDENT, FLAGS, COSTS, IMP, PART) \
   {NAME, AARCH64_ARCH_##ARCH_IDENT, FLAGS},
 #include "config/aarch64/aarch64-cores.def"
-#undef AARCH64_CORE
   {"generic", AARCH64_ARCH_8A, AARCH64_FL_FOR_ARCH8},
   {"", aarch64_no_arch, 0}
 };
@@ -156,7 +154,6 @@ static const struct arch_to_arch_name all_architectures[] =
 #define AARCH64_ARCH(NAME, CORE, ARCH_IDENT, ARCH, FLAGS) \
   {AARCH64_ARCH_##ARCH_IDENT, NAME},
 #include "config/aarch64/aarch64-arches.def"
-#undef AARCH64_ARCH
   {aarch64_no_arch, ""}
 };
 
diff --git a/gcc/config/aarch64/aarch64-arches.def b/gcc/config/aarch64/aarch64-arches.def
index 3b4fb73..542722d 100644
--- a/gcc/config/aarch64/aarch64-arches.def
+++ b/gcc/config/aarch64/aarch64-arches.def
@@ -33,3 +33,4 @@
 AARCH64_ARCH("armv8-a",	      generic,	     8A,	8,  AARCH64_FL_FOR_ARCH8)
 AARCH64_ARCH("armv8.1-a",     generic,	     8_1A,	8,  AARCH64_FL_FOR_ARCH8_1)
 
+#undef AARCH64_ARCH
diff --git a/gcc/config/aarch64/aarch64-cores.def b/gcc/config/aarch64/aarch64-cores.def
index 4af70ca..0b456f7 100644
--- a/gcc/config/aarch64/aarch64-cores.def
+++ b/gcc/config/aarch64/aarch64-cores.def
@@ -53,3 +53,5 @@ AARCH64_CORE("xgene1",      xgene1,    xgene1,    8A,  AARCH64_FL_FOR_ARCH8, xge
 AARCH64_CORE("cortex-a57.cortex-a53",  cortexa57cortexa53, cortexa53, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC, cortexa57, "0x41", "0xd07.0xd03")
 AARCH64_CORE("cortex-a72.cortex-a53",  cortexa72cortexa53, cortexa53, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC, cortexa72, "0x41", "0xd08.0xd03")
 
+
+#undef AARCH64_CORE
diff --git a/gcc/config/aarch64/aarch64-fusion-pairs.def b/gcc/config/aarch64/aarch64-fusion-pairs.def
index 53bbef4..a216422 100644
--- a/gcc/config/aarch64/aarch64-fusion-pairs.def
+++ b/gcc/config/aarch64/aarch64-fusion-pairs.def
@@ -34,3 +34,4 @@ AARCH64_FUSION_PAIR ("movk+movk", MOVK_MOVK)
 AARCH64_FUSION_PAIR ("adrp+ldr", ADRP_LDR)
 AARCH64_FUSION_PAIR ("cmp+branch", CMP_BRANCH)
 
+#undef AARCH64_FUSION_PAIR
diff --git a/gcc/config/aarch64/aarch64-option-extensions.def b/gcc/config/aarch64/aarch64-option-extensions.def
index b261a0f..ea092f5 100644
--- a/gcc/config/aarch64/aarch64-option-extensions.def
+++ b/gcc/config/aarch64/aarch64-option-extensions.def
@@ -42,3 +42,5 @@ AARCH64_OPT_EXTENSION("lse",	AARCH64_FL_LSE,                         AARCH64_FL_
 AARCH64_OPT_EXTENSION("pan",	AARCH64_FL_PAN,		AARCH64_FL_PAN,		"pan")
 AARCH64_OPT_EXTENSION("lor",	AARCH64_FL_LOR,		AARCH64_FL_LOR,		"lor")
 AARCH64_OPT_EXTENSION("rdma",	AARCH64_FL_RDMA | AARCH64_FL_FPSIMD,	AARCH64_FL_RDMA,	"rdma")
+
+#undef AARCH64_OPT_EXTENSION
diff --git a/gcc/config/aarch64/aarch64-opts.h b/gcc/config/aarch64/aarch64-opts.h
index bf6bb7b..5534867 100644
--- a/gcc/config/aarch64/aarch64-opts.h
+++ b/gcc/config/aarch64/aarch64-opts.h
@@ -28,7 +28,6 @@ enum aarch64_processor
 #define AARCH64_CORE(NAME, INTERNAL_IDENT, SCHED, ARCH, FLAGS, COSTS, IMP, PART) \
   INTERNAL_IDENT,
 #include "aarch64-cores.def"
-#undef AARCH64_CORE
   /* Used to indicate that no processor has been specified.  */
   generic,
   /* Used to mark the end of the processor table.  */
@@ -40,7 +39,6 @@ enum aarch64_arch
 #define AARCH64_ARCH(NAME, CORE, ARCH_IDENT, ARCH_REV, FLAGS) \
   AARCH64_ARCH_##ARCH_IDENT,
 #include "aarch64-arches.def"
-#undef AARCH64_ARCH
   aarch64_no_arch
 };
 
diff --git a/gcc/config/aarch64/aarch64-protos.h b/gcc/config/aarch64/aarch64-protos.h
index 9000d67..90ed0ef 100644
--- a/gcc/config/aarch64/aarch64-protos.h
+++ b/gcc/config/aarch64/aarch64-protos.h
@@ -227,7 +227,6 @@ enum aarch64_fusion_pairs_index
 #include "aarch64-fusion-pairs.def"
   AARCH64_FUSE_index_END
 };
-#undef AARCH64_FUSION_PAIR
 
 #define AARCH64_FUSION_PAIR(x, name) \
   AARCH64_FUSE_##name = (1u << AARCH64_FUSE_##name##_index),
@@ -238,7 +237,6 @@ enum aarch64_fusion_pairs
 #include "aarch64-fusion-pairs.def"
   AARCH64_FUSE_ALL = (1u << AARCH64_FUSE_index_END) - 1
 };
-#undef AARCH64_FUSION_PAIR
 
 #define AARCH64_EXTRA_TUNING_OPTION(x, name) \
   AARCH64_EXTRA_TUNE_##name##_index,
@@ -248,7 +246,6 @@ enum aarch64_extra_tuning_flags_index
 #include "aarch64-tuning-flags.def"
   AARCH64_EXTRA_TUNE_index_END
 };
-#undef AARCH64_EXTRA_TUNING_OPTION
 
 
 #define AARCH64_EXTRA_TUNING_OPTION(x, name) \
@@ -260,7 +257,6 @@ enum aarch64_extra_tuning_flags
 #include "aarch64-tuning-flags.def"
   AARCH64_EXTRA_TUNE_ALL = (1u << AARCH64_EXTRA_TUNE_index_END) - 1
 };
-#undef AARCH64_EXTRA_TUNING_OPTION
 
 extern struct tune_params aarch64_tune_params;
 
diff --git a/gcc/config/aarch64/aarch64-tuning-flags.def b/gcc/config/aarch64/aarch64-tuning-flags.def
index 6f7dbce..8677c20 100644
--- a/gcc/config/aarch64/aarch64-tuning-flags.def
+++ b/gcc/config/aarch64/aarch64-tuning-flags.def
@@ -31,3 +31,4 @@
 AARCH64_EXTRA_TUNING_OPTION ("rename_fma_regs", RENAME_FMA_REGS)
 AARCH64_EXTRA_TUNING_OPTION ("recip_sqrt", RECIP_SQRT)
 
+#undef AARCH64_EXTRA_TUNING_OPTION
diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index 5ec7f08..a5971a1 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -169,7 +169,6 @@ static const struct aarch64_flag_desc aarch64_fusible_pairs[] =
   { "all", AARCH64_FUSE_ALL },
   { NULL, AARCH64_FUSE_NOTHING }
 };
-#undef AARCH64_FUION_PAIR
 
 #define AARCH64_EXTRA_TUNING_OPTION(name, internal_name) \
   { name, AARCH64_EXTRA_TUNE_##internal_name },
@@ -180,7 +179,6 @@ static const struct aarch64_flag_desc aarch64_tuning_flags[] =
   { "all", AARCH64_EXTRA_TUNE_ALL },
   { NULL, AARCH64_EXTRA_TUNE_NONE }
 };
-#undef AARCH64_EXTRA_TUNING_OPTION
 
 /* Tuning parameters.  */
 
@@ -522,7 +520,6 @@ static const struct processor all_architectures[] =
 #define AARCH64_ARCH(NAME, CORE, ARCH_IDENT, ARCH_REV, FLAGS) \
   {NAME, CORE, CORE, AARCH64_ARCH_##ARCH_IDENT, ARCH_REV, FLAGS, NULL},
 #include "aarch64-arches.def"
-#undef AARCH64_ARCH
   {NULL, aarch64_none, aarch64_none, aarch64_no_arch, 0, 0, NULL}
 };
 
@@ -534,7 +531,6 @@ static const struct processor all_cores[] =
   all_architectures[AARCH64_ARCH_##ARCH].architecture_version,	\
   FLAGS, &COSTS##_tunings},
 #include "aarch64-cores.def"
-#undef AARCH64_CORE
   {"generic", generic, cortexa53, AARCH64_ARCH_8A, 8,
     AARCH64_FL_FOR_ARCH8, &generic_tunings},
   {NULL, aarch64_none, aarch64_none, aarch64_no_arch, 0, 0, NULL}
@@ -566,7 +562,6 @@ static const struct aarch64_option_extension all_extensions[] =
 #define AARCH64_OPT_EXTENSION(NAME, FLAGS_ON, FLAGS_OFF, FEATURE_STRING) \
   {NAME, FLAGS_ON, FLAGS_OFF},
 #include "aarch64-option-extensions.def"
-#undef AARCH64_OPT_EXTENSION
   {NULL, 0, 0}
 };
 
diff --git a/gcc/config/aarch64/aarch64.h b/gcc/config/aarch64/aarch64.h
index 8834c9b..c6582a4 100644
--- a/gcc/config/aarch64/aarch64.h
+++ b/gcc/config/aarch64/aarch64.h
@@ -467,7 +467,6 @@ enum target_cpus
 #define AARCH64_CORE(NAME, INTERNAL_IDENT, SCHED, ARCH, FLAGS, COSTS, IMP, PART) \
   TARGET_CPU_##INTERNAL_IDENT,
 #include "aarch64-cores.def"
-#undef AARCH64_CORE
   TARGET_CPU_generic
 };
 
diff --git a/gcc/config/aarch64/driver-aarch64.c b/gcc/config/aarch64/driver-aarch64.c
index ae4d5a0..d03654c 100644
--- a/gcc/config/aarch64/driver-aarch64.c
+++ b/gcc/config/aarch64/driver-aarch64.c
@@ -32,7 +32,6 @@ static struct arch_extension ext_to_feat_string[] =
 {
 #include "aarch64-option-extensions.def"
 };
-#undef AARCH64_OPT_EXTENSION
 
 
 struct aarch64_core_data
@@ -52,7 +51,6 @@ static struct aarch64_core_data cpu_data [] =
   { NULL, NULL, NULL, NULL }
 };
 
-#undef AARCH64_CORE
 
 struct aarch64_arch_driver_info
 {
@@ -69,7 +67,6 @@ static struct aarch64_arch_driver_info aarch64_arches [] =
   {NULL, NULL}
 };
 
-#undef AARCH64_ARCH
 
 /* Return the full architecture name string corresponding to the
    identifier ID.  */
-- 
1.9.1

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

* Re: [PATCH 5/5] [AARCH64] Add variant support to -m*=native and add thunderxt88pass1.
  2015-11-17 22:11 ` [PATCH 5/5] [AARCH64] Add variant support to -m*=native and add thunderxt88pass1 Andrew Pinski
@ 2015-11-17 22:55   ` Joseph Myers
  0 siblings, 0 replies; 19+ messages in thread
From: Joseph Myers @ 2015-11-17 22:55 UTC (permalink / raw)
  To: Andrew Pinski; +Cc: gcc-patches

invoke.texi needs updating for thunderxt88pass1 support.

-- 
Joseph S. Myers
joseph@codesourcery.com

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

* Re: [PATCH 1/5] [AARCH64]: Move #undef into .def files.
  2015-11-17 22:11 ` [PATCH 1/5] [AARCH64]: Move #undef into .def files Andrew Pinski
@ 2015-11-18 12:29   ` Marcus Shawcroft
  2016-10-07 21:36     ` Andrew Pinski
  0 siblings, 1 reply; 19+ messages in thread
From: Marcus Shawcroft @ 2015-11-18 12:29 UTC (permalink / raw)
  To: Andrew Pinski; +Cc: gcc-patches

On 17 November 2015 at 22:10, Andrew Pinski <apinski@cavium.com> wrote:
>
> This moves the #undef from the header files to the .def files like was done
> for builtins.def (https://gcc.gnu.org/ml/gcc-patches/2015-11/msg00662.html).
>
> OK?   Bootstrapped and tested on aarch64-linux-gnu with no regressions.
>
> Thanks,
> Andrew Pinski
>
> * config/aarch64/aarch64-arches.def (AARCH64_ARCH): #undef at the end.
> * config/aarch64/aarch64-cores.def (AARCH64_CORE): Likewise.
> * config/aarch64/aarch64-fusion-pairs.def (AARCH64_FUSION_PAIR): Likewise.
> * config/aarch64/aarch64-tuning-flags.def (AARCH64_EXTRA_TUNING_OPTION): Likewise.
> * config/aarch64/aarch64-opts.h (AARCH64_CORE): Don't #undef here.
> (AARCH64_ARCH): Likewise.
> * config/aarch64/aarch64-protos.h (AARCH64_FUSION_PAIR): Likewise.
> (AARCH64_EXTRA_TUNING_OPTION): Likewise.
> * config/aarch64/aarch64.c (AARCH64_FUION_PAIR): Likewise.
> (AARCH64_EXTRA_TUNING_OPTION): Likewise.
> (AARCH64_ARCH): Likewise.
> (AARCH64_CORE): Likewise.
> (AARCH64_OPT_EXTENSION): Likewise.
> * config/aarch64/aarch64.h (AARCH64_CORE): Likewise.
> * config/aarch64/driver-aarch64.c (AARCH64_OPT_EXTENSION): Likewise.
> (AARCH64_CORE): Likewise.
> (AARCH64_ARCH): Likewise.
> * common/config/aarch64/aarch64-common.c: Likewise.

OK Thanks /Marcus

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

* Re: [PATCH 0/5] Add support -mcpu=thunderxt88pass1 and needed changes to support that
  2015-11-17 22:10 [PATCH 0/5] Add support -mcpu=thunderxt88pass1 and needed changes to support that Andrew Pinski
                   ` (4 preceding siblings ...)
  2015-11-17 22:11 ` [PATCH 5/5] [AARCH64] Add variant support to -m*=native and add thunderxt88pass1 Andrew Pinski
@ 2015-11-19 12:08 ` Kyrill Tkachov
  2015-11-20  0:04   ` Andrew Pinski
  5 siblings, 1 reply; 19+ messages in thread
From: Kyrill Tkachov @ 2015-11-19 12:08 UTC (permalink / raw)
  To: Andrew Pinski, gcc-patches

Hi Andrew,

On 17/11/15 22:10, Andrew Pinski wrote:
> To Add support for -mcpu=thunderxt88pass1, I needed to fix up a few
> things in the support for -mcpu=native.  First was I wanted to do the same
> cleanup that was done for some of the other .def files and move the
> #undef into the .def files instead of the .h files.
> Second to make it easier to understand the implemention_id and part numbers
> are really numbers, move them over to use integer to compare against
> instead of strings which allows for easier comparisons later on.
> Third fix the way we compare imp and part num; that is instead of finding
> the part number and then comparing the imp, we find both numbers first
> and then search for the ones that match.
> Add a comment to the aarch64-cores.def file in front of each group of cores
> to signify the groups of company's cores.
> And all of this allows for adding variant for the check of the cores
> which allows for thunderxt88pass1 to be added.
>
> The reason why thunderxt88pass1 is seperate from thunderx is because
> thunderx is changed to be an ARMv8.1 arch core while thunderxt88pass1
> is still an ARMv8 arch core.
>
> I tested each of these patches seperately.

I tried these patches out on a big.LITTLE system with 2 Cortex-A57 and 4 Cortex-A53
cores and the code doesn't detect it properly anymore. Can you have a look please?
The Linux /proc/cpuinfo for that system looks like what's described here:
https://lists.linaro.org/pipermail/cross-distro/2014-October/000750.html
Look for the entry "[3] arm64, v3.17 + this patch, Juno platform "
You can put that into a file, point the fopen call in driver-aarch64.c to that file
and use that to debug the issue.

Thanks,
Kyrill

> Ok for the trunk even though I missed out on stage 1?
>
> Thanks,
> Andrew
>
> Andrew Pinski (5):
>    [AARCH64]: Move #undef into .def files.
>    [AARCH64] Change IMP and PART over to integers from strings.
>    [AARCH64] Fix part num and implement indendent.
>    {AARCH64] Add comment for the company's cores.
>    [AARCH64] Add variant support to -m*=native and add thunderxt88pass1.
>
>   gcc/common/config/aarch64/aarch64-common.c       |   5 +-
>   gcc/config/aarch64/aarch64-arches.def            |   1 +
>   gcc/config/aarch64/aarch64-cores.def             |  43 ++++--
>   gcc/config/aarch64/aarch64-fusion-pairs.def      |   1 +
>   gcc/config/aarch64/aarch64-option-extensions.def |   2 +
>   gcc/config/aarch64/aarch64-opts.h                |   4 +-
>   gcc/config/aarch64/aarch64-protos.h              |   4 -
>   gcc/config/aarch64/aarch64-tune.md               |   2 +-
>   gcc/config/aarch64/aarch64-tuning-flags.def      |   1 +
>   gcc/config/aarch64/aarch64.c                     |   7 +-
>   gcc/config/aarch64/aarch64.h                     |   3 +-
>   gcc/config/aarch64/driver-aarch64.c              | 169 +++++++++++++----------
>   12 files changed, 136 insertions(+), 106 deletions(-)
>

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

* Re: [PATCH 0/5] Add support -mcpu=thunderxt88pass1 and needed changes to support that
  2015-11-19 12:08 ` [PATCH 0/5] Add support -mcpu=thunderxt88pass1 and needed changes to support that Kyrill Tkachov
@ 2015-11-20  0:04   ` Andrew Pinski
  0 siblings, 0 replies; 19+ messages in thread
From: Andrew Pinski @ 2015-11-20  0:04 UTC (permalink / raw)
  To: Kyrill Tkachov; +Cc: Andrew Pinski, GCC Patches

On Thu, Nov 19, 2015 at 4:08 AM, Kyrill Tkachov <kyrylo.tkachov@arm.com> wrote:
> Hi Andrew,
>
> On 17/11/15 22:10, Andrew Pinski wrote:
>>
>> To Add support for -mcpu=thunderxt88pass1, I needed to fix up a few
>> things in the support for -mcpu=native.  First was I wanted to do the same
>> cleanup that was done for some of the other .def files and move the
>> #undef into the .def files instead of the .h files.
>> Second to make it easier to understand the implemention_id and part
>> numbers
>> are really numbers, move them over to use integer to compare against
>> instead of strings which allows for easier comparisons later on.
>> Third fix the way we compare imp and part num; that is instead of finding
>> the part number and then comparing the imp, we find both numbers first
>> and then search for the ones that match.
>> Add a comment to the aarch64-cores.def file in front of each group of
>> cores
>> to signify the groups of company's cores.
>> And all of this allows for adding variant for the check of the cores
>> which allows for thunderxt88pass1 to be added.
>>
>> The reason why thunderxt88pass1 is seperate from thunderx is because
>> thunderx is changed to be an ARMv8.1 arch core while thunderxt88pass1
>> is still an ARMv8 arch core.
>>
>> I tested each of these patches seperately.
>
>
> I tried these patches out on a big.LITTLE system with 2 Cortex-A57 and 4
> Cortex-A53
> cores and the code doesn't detect it properly anymore. Can you have a look
> please?
> The Linux /proc/cpuinfo for that system looks like what's described here:
> https://lists.linaro.org/pipermail/cross-distro/2014-October/000750.html
> Look for the entry "[3] arm64, v3.17 + this patch, Juno platform "
> You can put that into a file, point the fopen call in driver-aarch64.c to
> that file
> and use that to debug the issue.

It is a simple fix, valid_bL_core_p  should be checking both orders of
the core as I thought it ordering the BIG as first always.  Like:
static bool
valid_bL_core_p (unsigned int *core, unsigned int bL_core)
{
  return AARCH64_BIG_LITTLE (core[0], core[1]) == bL_core
         || AARCH64_BIG_LITTLE (core[1], core[0]) == bL_core;
}


Thanks,
Andrew


>
> Thanks,
> Kyrill
>
>
>> Ok for the trunk even though I missed out on stage 1?
>>
>> Thanks,
>> Andrew
>>
>> Andrew Pinski (5):
>>    [AARCH64]: Move #undef into .def files.
>>    [AARCH64] Change IMP and PART over to integers from strings.
>>    [AARCH64] Fix part num and implement indendent.
>>    {AARCH64] Add comment for the company's cores.
>>    [AARCH64] Add variant support to -m*=native and add thunderxt88pass1.
>>
>>   gcc/common/config/aarch64/aarch64-common.c       |   5 +-
>>   gcc/config/aarch64/aarch64-arches.def            |   1 +
>>   gcc/config/aarch64/aarch64-cores.def             |  43 ++++--
>>   gcc/config/aarch64/aarch64-fusion-pairs.def      |   1 +
>>   gcc/config/aarch64/aarch64-option-extensions.def |   2 +
>>   gcc/config/aarch64/aarch64-opts.h                |   4 +-
>>   gcc/config/aarch64/aarch64-protos.h              |   4 -
>>   gcc/config/aarch64/aarch64-tune.md               |   2 +-
>>   gcc/config/aarch64/aarch64-tuning-flags.def      |   1 +
>>   gcc/config/aarch64/aarch64.c                     |   7 +-
>>   gcc/config/aarch64/aarch64.h                     |   3 +-
>>   gcc/config/aarch64/driver-aarch64.c              | 169
>> +++++++++++++----------
>>   12 files changed, 136 insertions(+), 106 deletions(-)
>>
>

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

* Re: [PATCH 2/5] [AARCH64] Change IMP and PART over to integers from strings.
  2015-11-17 22:10 ` [PATCH 2/5] [AARCH64] Change IMP and PART over to integers from strings Andrew Pinski
@ 2015-11-20 11:07   ` Kyrill Tkachov
  2015-11-25 10:35   ` James Greenhalgh
  1 sibling, 0 replies; 19+ messages in thread
From: Kyrill Tkachov @ 2015-11-20 11:07 UTC (permalink / raw)
  To: Andrew Pinski, gcc-patches

Hi Andrew,

On 17/11/15 22:10, Andrew Pinski wrote:
> Because the imp and parts are really integer rather than strings, this patch
> moves the comparisons to be integer.  Also allows saving around integers are
> easier than doing string comparisons.  This allows for the next change.
>
> The way I store BIG.little is (big<<12)|little as each part num is only 12bits
> long.  So it would be nice if someone could test -mpu=native on a big.little
> system to make sure it works still.
>
> OK?  Bootstrapped and tested on aarch64-linux-gnu with no regressions.
>
> Thanks,
> Andrew Pinski
>
>
>
> * config/aarch64/aarch64-cores.def: Rewrite so IMP and PART are integer constants.
> * config/aarch64/driver-aarch64.c (struct aarch64_core_data): Change implementer_id to unsigned char.
> Change part_no to unsigned int.
> (AARCH64_BIG_LITTLE): New define.
> (INVALID_IMP): New define.
> (INVALID_CORE): New define.
> (cpu_data): Change the last element's implementer_id and part_no to integers.
> (valid_bL_string_p): Rewrite to ..
> (valid_bL_core_p): this for integers instead of strings.
> (parse_field): New function.
> (contains_string_p): Rewrite to ...
> (contains_core_p): this for integers and only for the part_no.
> (host_detect_local_cpu): Rewrite handling of implementation and part num to be integers;
> simplifying the code.
> ---
>   gcc/config/aarch64/aarch64-cores.def | 25 +++++-----
>   gcc/config/aarch64/driver-aarch64.c  | 90 ++++++++++++++++++++----------------
>   2 files changed, 62 insertions(+), 53 deletions(-)
>
> diff --git a/gcc/config/aarch64/aarch64-cores.def b/gcc/config/aarch64/aarch64-cores.def
> index 0b456f7..798f3e3 100644
> --- a/gcc/config/aarch64/aarch64-cores.def
> +++ b/gcc/config/aarch64/aarch64-cores.def
> @@ -33,25 +33,26 @@
>      This need not include flags implied by the architecture.
>      COSTS is the name of the rtx_costs routine to use.
>      IMP is the implementer ID of the CPU vendor.  On a GNU/Linux system it can
> -   be found in /proc/cpuinfo.
> +   be found in /proc/cpuinfo.  There is a list in the ARM ARM.
>      PART is the part number of the CPU.  On a GNU/Linux system it can be found
> -   in /proc/cpuinfo.  For big.LITTLE systems this should have the form at of
> -   "<big core part number>.<LITTLE core part number>".  */
> +   in /proc/cpuinfo.  For big.LITTLE systems this should use the macro AARCH64_BIG_LITTLE
> +   where the big part number comes as the first arugment to the macro and little is the
> +   second.  */
>   
>   /* V8 Architecture Processors.  */
>   
> -AARCH64_CORE("cortex-a53",  cortexa53, cortexa53, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC, cortexa53, "0x41", "0xd03")
> -AARCH64_CORE("cortex-a57",  cortexa57, cortexa57, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC, cortexa57, "0x41", "0xd07")
> -AARCH64_CORE("cortex-a72",  cortexa72, cortexa57, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC, cortexa72, "0x41", "0xd08")
> -AARCH64_CORE("exynos-m1",   exynosm1,  cortexa57, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC | AARCH64_FL_CRYPTO, cortexa72, "0x53", "0x001")
> -AARCH64_CORE("qdf24xx",     qdf24xx,   cortexa57, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC | AARCH64_FL_CRYPTO, cortexa57, "0x51", "0x800")
> -AARCH64_CORE("thunderx",    thunderx,  thunderx,  8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC | AARCH64_FL_CRYPTO, thunderx,  "0x43", "0x0a1")
> -AARCH64_CORE("xgene1",      xgene1,    xgene1,    8A,  AARCH64_FL_FOR_ARCH8, xgene1, "0x50", "0x000")
> +AARCH64_CORE("cortex-a53",  cortexa53, cortexa53, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC, cortexa53, 0x41, 0xd03)
> +AARCH64_CORE("cortex-a57",  cortexa57, cortexa57, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC, cortexa57, 0x41, 0xd07)
> +AARCH64_CORE("cortex-a72",  cortexa72, cortexa57, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC, cortexa72, 0x41, 0xd08)
> +AARCH64_CORE("exynos-m1",   exynosm1,  cortexa57, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC | AARCH64_FL_CRYPTO, cortexa72, 0x53, 0x001)
> +AARCH64_CORE("qdf24xx",     qdf24xx,   cortexa57, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC | AARCH64_FL_CRYPTO, cortexa57, 0x51, 0x800)
> +AARCH64_CORE("thunderx",    thunderx,  thunderx,  8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC | AARCH64_FL_CRYPTO, thunderx,  0x43, 0x0a1)
> +AARCH64_CORE("xgene1",      xgene1,    xgene1,    8A,  AARCH64_FL_FOR_ARCH8, xgene1, 0x50, 0x000)
>   
>   /* V8 big.LITTLE implementations.  */
>   
> -AARCH64_CORE("cortex-a57.cortex-a53",  cortexa57cortexa53, cortexa53, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC, cortexa57, "0x41", "0xd07.0xd03")
> -AARCH64_CORE("cortex-a72.cortex-a53",  cortexa72cortexa53, cortexa53, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC, cortexa72, "0x41", "0xd08.0xd03")
> +AARCH64_CORE("cortex-a57.cortex-a53",  cortexa57cortexa53, cortexa53, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC, cortexa57, 0x41, AARCH64_BIG_LITTLE(0xd07, 0xd03))
> +AARCH64_CORE("cortex-a72.cortex-a53",  cortexa72cortexa53, cortexa53, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC, cortexa72, 0x41, AARCH64_BIG_LITTLE(0xd08, 0xd03))
>   
>   
>   #undef AARCH64_CORE
> diff --git a/gcc/config/aarch64/driver-aarch64.c b/gcc/config/aarch64/driver-aarch64.c
> index d03654c..92388a9 100644
> --- a/gcc/config/aarch64/driver-aarch64.c
> +++ b/gcc/config/aarch64/driver-aarch64.c
> @@ -37,18 +37,23 @@ static struct arch_extension ext_to_feat_string[] =
>   struct aarch64_core_data
>   {
>     const char* name;
> -  const char* arch;
> -  const char* implementer_id;
> -  const char* part_no;
> +  const char *arch;
> +  unsigned char implementer_id; /* Exactly 8 bits */
> +  unsigned int part_no; /* 12 bits + 12 bits */
>   };
>   
> +#define AARCH64_BIG_LITTLE(BIG, LITTLE) \
> +  (((BIG)&0xFFFu) << 12 | ((LITTLE) & 0xFFFu))
> +#define INVALID_IMP ((unsigned char) -1)
> +#define INVALID_CORE ((unsigned)-1)
> +
>   #define AARCH64_CORE(CORE_NAME, CORE_IDENT, SCHED, ARCH, FLAGS, COSTS, IMP, PART) \
>     { CORE_NAME, #ARCH, IMP, PART },
>   
>   static struct aarch64_core_data cpu_data [] =
>   {
>   #include "aarch64-cores.def"
> -  { NULL, NULL, NULL, NULL }
> +  { NULL, NULL, INVALID_IMP, INVALID_CORE }
>   };
>   
>   
> @@ -91,27 +96,35 @@ get_arch_name_from_id (const char* id)
>      should return true.  */
>   
>   static bool
> -valid_bL_string_p (const char** core, const char* bL_string)
> +valid_bL_core_p (unsigned int *core, unsigned int bL_core)
>   {
> -  return strstr (bL_string, core[0]) != NULL
> -         && strstr (bL_string, core[1]) != NULL;
> +  return AARCH64_BIG_LITTLE (core[0], core[1]) == bL_core;
>   }

This needs the change you suggested in https://gcc.gnu.org/ml/gcc-patches/2015-11/msg02436.html
I've checked that it works now.
Also, please update the comment for this function since the arguments are no longer strings.

Thanks,
Kyrill

>   
> -/*  Return true iff ARR contains STR in one of its two elements.  */
>   
> -static bool
> -contains_string_p (const char** arr, const char* str)
> +/* Returns the integer that is after ':' for the field. */
> +static unsigned parse_field (const char *field)
>   {
> -  bool res = false;
> +  const char *rest = strchr (field, ':');
> +  char *after;
> +  unsigned fint = strtol (rest+1, &after, 16);
> +  if (after == rest+1)
> +    return -1;
> +  return fint;
> +}
> +
> +/*  Return true iff ARR contains CORE, in either of the two elements. */
>   
> -  if (arr[0] != NULL)
> +static bool
> +contains_core_p (unsigned *arr, unsigned core)
> +{
> +  if (arr[0] != INVALID_CORE)
>       {
> -      res = strstr (arr[0], str) != NULL;
> -      if (res)
> -        return res;
> +      if (arr[0] == core)
> +        return true;
>   
> -      if (arr[1] != NULL)
> -        return strstr (arr[1], str) != NULL;
> +      if (arr[1] != INVALID_CORE)
> +        return arr[1] == core;
>       }
>   
>     return false;
> @@ -146,10 +159,9 @@ host_detect_local_cpu (int argc, const char **argv)
>     bool cpu = false;
>     unsigned int i = 0;
>     unsigned int core_idx = 0;
> -  const char* imps[2] = { NULL, NULL };
> -  const char* cores[2] = { NULL, NULL };
> +  unsigned char imp = INVALID_IMP;
> +  unsigned int cores[2] = { INVALID_CORE, INVALID_CORE };
>     unsigned int n_cores = 0;
> -  unsigned int n_imps = 0;
>     bool processed_exts = false;
>     const char *ext_string = "";
>   
> @@ -180,30 +192,28 @@ host_detect_local_cpu (int argc, const char **argv)
>       {
>         if (strstr (buf, "implementer") != NULL)
>   	{
> -	  for (i = 0; cpu_data[i].name != NULL; i++)
> -	    if (strstr (buf, cpu_data[i].implementer_id) != NULL
> -                && !contains_string_p (imps, cpu_data[i].implementer_id))
> -	      {
> -                if (n_imps == 2)
> -                  goto not_found;
> -
> -                imps[n_imps++] = cpu_data[i].implementer_id;
> -
> -                break;
> -	      }
> -          continue;
> +	  unsigned cimp = parse_field (buf);
> +	  if (cimp == INVALID_IMP)
> +	    goto not_found;
> +
> +	  if (imp == INVALID_IMP)
> +	    imp = cimp;
> +	  /* BIG.little implementers are always equal. */
> +	  else if (imp != cimp)
> +	    goto not_found;
>   	}
>   
>         if (strstr (buf, "part") != NULL)
>   	{
> +	  unsigned ccore = parse_field (buf);
>   	  for (i = 0; cpu_data[i].name != NULL; i++)
> -	    if (strstr (buf, cpu_data[i].part_no) != NULL
> -                && !contains_string_p (cores, cpu_data[i].part_no))
> +	    if (ccore == cpu_data[i].part_no
> +                && !contains_core_p (cores, ccore))
>   	      {
>                   if (n_cores == 2)
>                     goto not_found;
>   
> -                cores[n_cores++] = cpu_data[i].part_no;
> +                cores[n_cores++] = ccore;
>   	        core_idx = i;
>   	        arch_id = cpu_data[i].arch;
>   	        break;
> @@ -240,7 +250,7 @@ host_detect_local_cpu (int argc, const char **argv)
>     f = NULL;
>   
>     /* Weird cpuinfo format that we don't know how to handle.  */
> -  if (n_cores == 0 || n_cores > 2 || n_imps != 1)
> +  if (n_cores == 0 || n_cores > 2 || imp == INVALID_IMP)
>       goto not_found;
>   
>     if (arch && !arch_id)
> @@ -261,9 +271,8 @@ host_detect_local_cpu (int argc, const char **argv)
>       {
>         for (i = 0; cpu_data[i].name != NULL; i++)
>           {
> -          if (strchr (cpu_data[i].part_no, '.') != NULL
> -              && strncmp (cpu_data[i].implementer_id, imps[0], strlen (imps[0]) - 1) == 0
> -              && valid_bL_string_p (cores, cpu_data[i].part_no))
> +          if (cpu_data[i].implementer_id == imp
> +              && valid_bL_core_p (cores, cpu_data[i].part_no))
>               {
>                 res = concat ("-m", cpu ? "cpu" : "tune", "=", cpu_data[i].name, NULL);
>                 break;
> @@ -275,8 +284,7 @@ host_detect_local_cpu (int argc, const char **argv)
>     /* The simple, non-big.LITTLE case.  */
>     else
>       {
> -      if (strncmp (cpu_data[core_idx].implementer_id, imps[0],
> -                   strlen (imps[0]) - 1) != 0)
> +      if (cpu_data[core_idx].implementer_id != imp)
>           goto not_found;
>   
>         res = concat ("-m", cpu ? "cpu" : "tune", "=",

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

* Re: [PATCH 2/5] [AARCH64] Change IMP and PART over to integers from strings.
  2015-11-17 22:10 ` [PATCH 2/5] [AARCH64] Change IMP and PART over to integers from strings Andrew Pinski
  2015-11-20 11:07   ` Kyrill Tkachov
@ 2015-11-25 10:35   ` James Greenhalgh
  2015-11-25 20:12     ` Andrew Pinski
  1 sibling, 1 reply; 19+ messages in thread
From: James Greenhalgh @ 2015-11-25 10:35 UTC (permalink / raw)
  To: Andrew Pinski; +Cc: gcc-patches

On Tue, Nov 17, 2015 at 02:10:35PM -0800, Andrew Pinski wrote:
> 
> Because the imp and parts are really integer rather than strings, this patch
> moves the comparisons to be integer.  Also allows saving around integers are
> easier than doing string comparisons.  This allows for the next change.
> 
> The way I store BIG.little is (big<<12)|little as each part num is only 12bits
> long.  So it would be nice if someone could test -mpu=native on a big.little
> system to make sure it works still.
> 
> OK?  Bootstrapped and tested on aarch64-linux-gnu with no regressions.
> 
> Thanks,
> Andrew Pinski
> 
> 
> 
> * config/aarch64/aarch64-cores.def: Rewrite so IMP and PART are integer constants.
> * config/aarch64/driver-aarch64.c (struct aarch64_core_data): Change implementer_id to unsigned char.
> Change part_no to unsigned int.
> (AARCH64_BIG_LITTLE): New define.
> (INVALID_IMP): New define.
> (INVALID_CORE): New define.
> (cpu_data): Change the last element's implementer_id and part_no to integers.
> (valid_bL_string_p): Rewrite to ..
> (valid_bL_core_p): this for integers instead of strings.
> (parse_field): New function.
> (contains_string_p): Rewrite to ...
> (contains_core_p): this for integers and only for the part_no.
> (host_detect_local_cpu): Rewrite handling of implementation and part num to be integers;
> simplifying the code.
> ---
>  gcc/config/aarch64/aarch64-cores.def | 25 +++++-----
>  gcc/config/aarch64/driver-aarch64.c  | 90 ++++++++++++++++++++----------------
>  2 files changed, 62 insertions(+), 53 deletions(-)
> 
> diff --git a/gcc/config/aarch64/aarch64-cores.def b/gcc/config/aarch64/aarch64-cores.def
> index 0b456f7..798f3e3 100644
> --- a/gcc/config/aarch64/aarch64-cores.def
> +++ b/gcc/config/aarch64/aarch64-cores.def
> @@ -33,25 +33,26 @@
>     This need not include flags implied by the architecture.
>     COSTS is the name of the rtx_costs routine to use.
>     IMP is the implementer ID of the CPU vendor.  On a GNU/Linux system it can
> -   be found in /proc/cpuinfo.
> +   be found in /proc/cpuinfo.  There is a list in the ARM ARM.

Let's be precise with this comment.

  "A partial list of implementer IDs is given in the ARM Architecture
   Reference Manual ARMv8, for ARMv8-A architecture profile"

>     PART is the part number of the CPU.  On a GNU/Linux system it can be found
> -   in /proc/cpuinfo.  For big.LITTLE systems this should have the form at of
> -   "<big core part number>.<LITTLE core part number>".  */
> +   in /proc/cpuinfo.  For big.LITTLE systems this should use the macro AARCH64_BIG_LITTLE
> +   where the big part number comes as the first arugment to the macro and little is the

s/arugment/argument/.

> +   second.  */
>  
>  /* V8 Architecture Processors.  */
>  
> -AARCH64_CORE("cortex-a53",  cortexa53, cortexa53, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC, cortexa53, "0x41", "0xd03")
> -AARCH64_CORE("cortex-a57",  cortexa57, cortexa57, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC, cortexa57, "0x41", "0xd07")
> -AARCH64_CORE("cortex-a72",  cortexa72, cortexa57, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC, cortexa72, "0x41", "0xd08")
> -AARCH64_CORE("exynos-m1",   exynosm1,  cortexa57, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC | AARCH64_FL_CRYPTO, cortexa72, "0x53", "0x001")
> -AARCH64_CORE("qdf24xx",     qdf24xx,   cortexa57, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC | AARCH64_FL_CRYPTO, cortexa57, "0x51", "0x800")
> -AARCH64_CORE("thunderx",    thunderx,  thunderx,  8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC | AARCH64_FL_CRYPTO, thunderx,  "0x43", "0x0a1")
> -AARCH64_CORE("xgene1",      xgene1,    xgene1,    8A,  AARCH64_FL_FOR_ARCH8, xgene1, "0x50", "0x000")
> +AARCH64_CORE("cortex-a53",  cortexa53, cortexa53, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC, cortexa53, 0x41, 0xd03)
> +AARCH64_CORE("cortex-a57",  cortexa57, cortexa57, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC, cortexa57, 0x41, 0xd07)
> +AARCH64_CORE("cortex-a72",  cortexa72, cortexa57, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC, cortexa72, 0x41, 0xd08)
> +AARCH64_CORE("exynos-m1",   exynosm1,  cortexa57, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC | AARCH64_FL_CRYPTO, cortexa72, 0x53, 0x001)
> +AARCH64_CORE("qdf24xx",     qdf24xx,   cortexa57, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC | AARCH64_FL_CRYPTO, cortexa57, 0x51, 0x800)
> +AARCH64_CORE("thunderx",    thunderx,  thunderx,  8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC | AARCH64_FL_CRYPTO, thunderx,  0x43, 0x0a1)
> +AARCH64_CORE("xgene1",      xgene1,    xgene1,    8A,  AARCH64_FL_FOR_ARCH8, xgene1, 0x50, 0x000)
>  
>  /* V8 big.LITTLE implementations.  */
>  
> -AARCH64_CORE("cortex-a57.cortex-a53",  cortexa57cortexa53, cortexa53, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC, cortexa57, "0x41", "0xd07.0xd03")
> -AARCH64_CORE("cortex-a72.cortex-a53",  cortexa72cortexa53, cortexa53, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC, cortexa72, "0x41", "0xd08.0xd03")
> +AARCH64_CORE("cortex-a57.cortex-a53",  cortexa57cortexa53, cortexa53, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC, cortexa57, 0x41, AARCH64_BIG_LITTLE(0xd07, 0xd03))
> +AARCH64_CORE("cortex-a72.cortex-a53",  cortexa72cortexa53, cortexa53, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC, cortexa72, 0x41, AARCH64_BIG_LITTLE(0xd08, 0xd03))
>  
>  
>  #undef AARCH64_CORE
> diff --git a/gcc/config/aarch64/driver-aarch64.c b/gcc/config/aarch64/driver-aarch64.c
> index d03654c..92388a9 100644
> --- a/gcc/config/aarch64/driver-aarch64.c
> +++ b/gcc/config/aarch64/driver-aarch64.c
> @@ -37,18 +37,23 @@ static struct arch_extension ext_to_feat_string[] =
>  struct aarch64_core_data
>  {
>    const char* name;
> -  const char* arch;
> -  const char* implementer_id;
> -  const char* part_no;
> +  const char *arch;
> +  unsigned char implementer_id; /* Exactly 8 bits */

Can we redesign this around the most general case - big.LITTLE cores with
different implementer IDs? There is nothing in the architecture that would
forbid this, and Samsung recently announced that their Exynos 8 Octa 8890
which would support a combination of four of their custom cores with four ARM
Cortex-A53 cores.

 ( http://www.samsung.com/semiconductor/minisite/Exynos/w/mediacenter.html#?v=news_Samsung_Unveils_a_New_Exynos_8_Octa_Processor_based_on_advanced_14-Nanometer_FinFET_Process_Technology )

> +  unsigned int part_no; /* 12 bits + 12 bits */
>  };
>  
> +#define AARCH64_BIG_LITTLE(BIG, LITTLE) \
> +  (((BIG)&0xFFFu) << 12 | ((LITTLE) & 0xFFFu))
> +#define INVALID_IMP ((unsigned char) -1)
> +#define INVALID_CORE ((unsigned)-1)
> +
>  #define AARCH64_CORE(CORE_NAME, CORE_IDENT, SCHED, ARCH, FLAGS, COSTS, IMP, PART) \
>    { CORE_NAME, #ARCH, IMP, PART },
>  
>  static struct aarch64_core_data cpu_data [] =
>  {
>  #include "aarch64-cores.def"
> -  { NULL, NULL, NULL, NULL }
> +  { NULL, NULL, INVALID_IMP, INVALID_CORE }
>  };
>  
>  
> @@ -91,27 +96,35 @@ get_arch_name_from_id (const char* id)
>     should return true.  */
>  
>  static bool
> -valid_bL_string_p (const char** core, const char* bL_string)
> +valid_bL_core_p (unsigned int *core, unsigned int bL_core)
>  {
> -  return strstr (bL_string, core[0]) != NULL
> -         && strstr (bL_string, core[1]) != NULL;
> +  return AARCH64_BIG_LITTLE (core[0], core[1]) == bL_core;
>  }
>  
> -/*  Return true iff ARR contains STR in one of its two elements.  */
>  
> -static bool
> -contains_string_p (const char** arr, const char* str)
> +/* Returns the integer that is after ':' for the field. */
> +static unsigned parse_field (const char *field)
>  {
> -  bool res = false;
> +  const char *rest = strchr (field, ':');
> +  char *after;
> +  unsigned fint = strtol (rest+1, &after, 16);
> +  if (after == rest+1)

"rest + 1" in both cases.

> +    return -1;
> +  return fint;
> +}
> +
> +/*  Return true iff ARR contains CORE, in either of the two elements. */
>  
> -  if (arr[0] != NULL)
> +static bool
> +contains_core_p (unsigned *arr, unsigned core)
> +{
> +  if (arr[0] != INVALID_CORE)
>      {
> -      res = strstr (arr[0], str) != NULL;
> -      if (res)
> -        return res;
> +      if (arr[0] == core)
> +        return true;
>  
> -      if (arr[1] != NULL)
> -        return strstr (arr[1], str) != NULL;
> +      if (arr[1] != INVALID_CORE)
> +        return arr[1] == core;
>      }
>  
>    return false;
> @@ -146,10 +159,9 @@ host_detect_local_cpu (int argc, const char **argv)
>    bool cpu = false;
>    unsigned int i = 0;
>    unsigned int core_idx = 0;
> -  const char* imps[2] = { NULL, NULL };
> -  const char* cores[2] = { NULL, NULL };
> +  unsigned char imp = INVALID_IMP;
> +  unsigned int cores[2] = { INVALID_CORE, INVALID_CORE };
>    unsigned int n_cores = 0;
> -  unsigned int n_imps = 0;
>    bool processed_exts = false;
>    const char *ext_string = "";
>  
> @@ -180,30 +192,28 @@ host_detect_local_cpu (int argc, const char **argv)
>      {
>        if (strstr (buf, "implementer") != NULL)
>  	{
> -	  for (i = 0; cpu_data[i].name != NULL; i++)
> -	    if (strstr (buf, cpu_data[i].implementer_id) != NULL
> -                && !contains_string_p (imps, cpu_data[i].implementer_id))
> -	      {
> -                if (n_imps == 2)
> -                  goto not_found;
> -
> -                imps[n_imps++] = cpu_data[i].implementer_id;
> -
> -                break;
> -	      }
> -          continue;
> +	  unsigned cimp = parse_field (buf);
> +	  if (cimp == INVALID_IMP)
> +	    goto not_found;
> +
> +	  if (imp == INVALID_IMP)
> +	    imp = cimp;
> +	  /* BIG.little implementers are always equal. */

See my comment above. This comment does not neccessarily hold.

In addition, this needs updating for Kyrill's comments.

Thanks,
James
 

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

* Re: [PATCH 2/5] [AARCH64] Change IMP and PART over to integers from strings.
  2015-11-25 10:35   ` James Greenhalgh
@ 2015-11-25 20:12     ` Andrew Pinski
  2016-10-16  2:38       ` Andrew Pinski
  0 siblings, 1 reply; 19+ messages in thread
From: Andrew Pinski @ 2015-11-25 20:12 UTC (permalink / raw)
  To: James Greenhalgh; +Cc: Andrew Pinski, GCC Patches

On Wed, Nov 25, 2015 at 2:31 AM, James Greenhalgh
<james.greenhalgh@arm.com> wrote:
> On Tue, Nov 17, 2015 at 02:10:35PM -0800, Andrew Pinski wrote:
>>
>> Because the imp and parts are really integer rather than strings, this patch
>> moves the comparisons to be integer.  Also allows saving around integers are
>> easier than doing string comparisons.  This allows for the next change.
>>
>> The way I store BIG.little is (big<<12)|little as each part num is only 12bits
>> long.  So it would be nice if someone could test -mpu=native on a big.little
>> system to make sure it works still.
>>
>> OK?  Bootstrapped and tested on aarch64-linux-gnu with no regressions.
>>
>> Thanks,
>> Andrew Pinski
>>
>>
>>
>> * config/aarch64/aarch64-cores.def: Rewrite so IMP and PART are integer constants.
>> * config/aarch64/driver-aarch64.c (struct aarch64_core_data): Change implementer_id to unsigned char.
>> Change part_no to unsigned int.
>> (AARCH64_BIG_LITTLE): New define.
>> (INVALID_IMP): New define.
>> (INVALID_CORE): New define.
>> (cpu_data): Change the last element's implementer_id and part_no to integers.
>> (valid_bL_string_p): Rewrite to ..
>> (valid_bL_core_p): this for integers instead of strings.
>> (parse_field): New function.
>> (contains_string_p): Rewrite to ...
>> (contains_core_p): this for integers and only for the part_no.
>> (host_detect_local_cpu): Rewrite handling of implementation and part num to be integers;
>> simplifying the code.
>> ---
>>  gcc/config/aarch64/aarch64-cores.def | 25 +++++-----
>>  gcc/config/aarch64/driver-aarch64.c  | 90 ++++++++++++++++++++----------------
>>  2 files changed, 62 insertions(+), 53 deletions(-)
>>
>> diff --git a/gcc/config/aarch64/aarch64-cores.def b/gcc/config/aarch64/aarch64-cores.def
>> index 0b456f7..798f3e3 100644
>> --- a/gcc/config/aarch64/aarch64-cores.def
>> +++ b/gcc/config/aarch64/aarch64-cores.def
>> @@ -33,25 +33,26 @@
>>     This need not include flags implied by the architecture.
>>     COSTS is the name of the rtx_costs routine to use.
>>     IMP is the implementer ID of the CPU vendor.  On a GNU/Linux system it can
>> -   be found in /proc/cpuinfo.
>> +   be found in /proc/cpuinfo.  There is a list in the ARM ARM.
>
> Let's be precise with this comment.
>
>   "A partial list of implementer IDs is given in the ARM Architecture
>    Reference Manual ARMv8, for ARMv8-A architecture profile"
>
>>     PART is the part number of the CPU.  On a GNU/Linux system it can be found
>> -   in /proc/cpuinfo.  For big.LITTLE systems this should have the form at of
>> -   "<big core part number>.<LITTLE core part number>".  */
>> +   in /proc/cpuinfo.  For big.LITTLE systems this should use the macro AARCH64_BIG_LITTLE
>> +   where the big part number comes as the first arugment to the macro and little is the
>
> s/arugment/argument/.
>
>> +   second.  */
>>
>>  /* V8 Architecture Processors.  */
>>
>> -AARCH64_CORE("cortex-a53",  cortexa53, cortexa53, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC, cortexa53, "0x41", "0xd03")
>> -AARCH64_CORE("cortex-a57",  cortexa57, cortexa57, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC, cortexa57, "0x41", "0xd07")
>> -AARCH64_CORE("cortex-a72",  cortexa72, cortexa57, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC, cortexa72, "0x41", "0xd08")
>> -AARCH64_CORE("exynos-m1",   exynosm1,  cortexa57, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC | AARCH64_FL_CRYPTO, cortexa72, "0x53", "0x001")
>> -AARCH64_CORE("qdf24xx",     qdf24xx,   cortexa57, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC | AARCH64_FL_CRYPTO, cortexa57, "0x51", "0x800")
>> -AARCH64_CORE("thunderx",    thunderx,  thunderx,  8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC | AARCH64_FL_CRYPTO, thunderx,  "0x43", "0x0a1")
>> -AARCH64_CORE("xgene1",      xgene1,    xgene1,    8A,  AARCH64_FL_FOR_ARCH8, xgene1, "0x50", "0x000")
>> +AARCH64_CORE("cortex-a53",  cortexa53, cortexa53, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC, cortexa53, 0x41, 0xd03)
>> +AARCH64_CORE("cortex-a57",  cortexa57, cortexa57, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC, cortexa57, 0x41, 0xd07)
>> +AARCH64_CORE("cortex-a72",  cortexa72, cortexa57, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC, cortexa72, 0x41, 0xd08)
>> +AARCH64_CORE("exynos-m1",   exynosm1,  cortexa57, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC | AARCH64_FL_CRYPTO, cortexa72, 0x53, 0x001)
>> +AARCH64_CORE("qdf24xx",     qdf24xx,   cortexa57, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC | AARCH64_FL_CRYPTO, cortexa57, 0x51, 0x800)
>> +AARCH64_CORE("thunderx",    thunderx,  thunderx,  8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC | AARCH64_FL_CRYPTO, thunderx,  0x43, 0x0a1)
>> +AARCH64_CORE("xgene1",      xgene1,    xgene1,    8A,  AARCH64_FL_FOR_ARCH8, xgene1, 0x50, 0x000)
>>
>>  /* V8 big.LITTLE implementations.  */
>>
>> -AARCH64_CORE("cortex-a57.cortex-a53",  cortexa57cortexa53, cortexa53, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC, cortexa57, "0x41", "0xd07.0xd03")
>> -AARCH64_CORE("cortex-a72.cortex-a53",  cortexa72cortexa53, cortexa53, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC, cortexa72, "0x41", "0xd08.0xd03")
>> +AARCH64_CORE("cortex-a57.cortex-a53",  cortexa57cortexa53, cortexa53, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC, cortexa57, 0x41, AARCH64_BIG_LITTLE(0xd07, 0xd03))
>> +AARCH64_CORE("cortex-a72.cortex-a53",  cortexa72cortexa53, cortexa53, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC, cortexa72, 0x41, AARCH64_BIG_LITTLE(0xd08, 0xd03))
>>
>>
>>  #undef AARCH64_CORE
>> diff --git a/gcc/config/aarch64/driver-aarch64.c b/gcc/config/aarch64/driver-aarch64.c
>> index d03654c..92388a9 100644
>> --- a/gcc/config/aarch64/driver-aarch64.c
>> +++ b/gcc/config/aarch64/driver-aarch64.c
>> @@ -37,18 +37,23 @@ static struct arch_extension ext_to_feat_string[] =
>>  struct aarch64_core_data
>>  {
>>    const char* name;
>> -  const char* arch;
>> -  const char* implementer_id;
>> -  const char* part_no;
>> +  const char *arch;
>> +  unsigned char implementer_id; /* Exactly 8 bits */
>
> Can we redesign this around the most general case - big.LITTLE cores with
> different implementer IDs? There is nothing in the architecture that would
> forbid this, and Samsung recently announced that their Exynos 8 Octa 8890
> which would support a combination of four of their custom cores with four ARM
> Cortex-A53 cores.
>
>  ( http://www.samsung.com/semiconductor/minisite/Exynos/w/mediacenter.html#?v=news_Samsung_Unveils_a_New_Exynos_8_Octa_Processor_based_on_advanced_14-Nanometer_FinFET_Process_Technology )

This should be done a separate patch as even the current
infrastructure does not support it at all.
It would be a simple one though.  Change implementer_id to int and
then encode the two implementer_id in cores_data.  Though the parsing
part becomes more complex as the current parsing does not understand
if a new core has been started or not.  This is why it should be a
separate patch instead.

Maybe to support that one it is not about parsing /proc/cpuinfo but
rather /sys/cpu/* instead which is supported for Linux 4.4 (or did not
make it into 4.4 so 4.5).

Thanks,
Andrew

>
>> +  unsigned int part_no; /* 12 bits + 12 bits */
>>  };
>>
>> +#define AARCH64_BIG_LITTLE(BIG, LITTLE) \
>> +  (((BIG)&0xFFFu) << 12 | ((LITTLE) & 0xFFFu))
>> +#define INVALID_IMP ((unsigned char) -1)
>> +#define INVALID_CORE ((unsigned)-1)
>> +
>>  #define AARCH64_CORE(CORE_NAME, CORE_IDENT, SCHED, ARCH, FLAGS, COSTS, IMP, PART) \
>>    { CORE_NAME, #ARCH, IMP, PART },
>>
>>  static struct aarch64_core_data cpu_data [] =
>>  {
>>  #include "aarch64-cores.def"
>> -  { NULL, NULL, NULL, NULL }
>> +  { NULL, NULL, INVALID_IMP, INVALID_CORE }
>>  };
>>
>>
>> @@ -91,27 +96,35 @@ get_arch_name_from_id (const char* id)
>>     should return true.  */
>>
>>  static bool
>> -valid_bL_string_p (const char** core, const char* bL_string)
>> +valid_bL_core_p (unsigned int *core, unsigned int bL_core)
>>  {
>> -  return strstr (bL_string, core[0]) != NULL
>> -         && strstr (bL_string, core[1]) != NULL;
>> +  return AARCH64_BIG_LITTLE (core[0], core[1]) == bL_core;
>>  }
>>
>> -/*  Return true iff ARR contains STR in one of its two elements.  */
>>
>> -static bool
>> -contains_string_p (const char** arr, const char* str)
>> +/* Returns the integer that is after ':' for the field. */
>> +static unsigned parse_field (const char *field)
>>  {
>> -  bool res = false;
>> +  const char *rest = strchr (field, ':');
>> +  char *after;
>> +  unsigned fint = strtol (rest+1, &after, 16);
>> +  if (after == rest+1)
>
> "rest + 1" in both cases.
>
>> +    return -1;
>> +  return fint;
>> +}
>> +
>> +/*  Return true iff ARR contains CORE, in either of the two elements. */
>>
>> -  if (arr[0] != NULL)
>> +static bool
>> +contains_core_p (unsigned *arr, unsigned core)
>> +{
>> +  if (arr[0] != INVALID_CORE)
>>      {
>> -      res = strstr (arr[0], str) != NULL;
>> -      if (res)
>> -        return res;
>> +      if (arr[0] == core)
>> +        return true;
>>
>> -      if (arr[1] != NULL)
>> -        return strstr (arr[1], str) != NULL;
>> +      if (arr[1] != INVALID_CORE)
>> +        return arr[1] == core;
>>      }
>>
>>    return false;
>> @@ -146,10 +159,9 @@ host_detect_local_cpu (int argc, const char **argv)
>>    bool cpu = false;
>>    unsigned int i = 0;
>>    unsigned int core_idx = 0;
>> -  const char* imps[2] = { NULL, NULL };
>> -  const char* cores[2] = { NULL, NULL };
>> +  unsigned char imp = INVALID_IMP;
>> +  unsigned int cores[2] = { INVALID_CORE, INVALID_CORE };
>>    unsigned int n_cores = 0;
>> -  unsigned int n_imps = 0;
>>    bool processed_exts = false;
>>    const char *ext_string = "";
>>
>> @@ -180,30 +192,28 @@ host_detect_local_cpu (int argc, const char **argv)
>>      {
>>        if (strstr (buf, "implementer") != NULL)
>>       {
>> -       for (i = 0; cpu_data[i].name != NULL; i++)
>> -         if (strstr (buf, cpu_data[i].implementer_id) != NULL
>> -                && !contains_string_p (imps, cpu_data[i].implementer_id))
>> -           {
>> -                if (n_imps == 2)
>> -                  goto not_found;
>> -
>> -                imps[n_imps++] = cpu_data[i].implementer_id;
>> -
>> -                break;
>> -           }
>> -          continue;
>> +       unsigned cimp = parse_field (buf);
>> +       if (cimp == INVALID_IMP)
>> +         goto not_found;
>> +
>> +       if (imp == INVALID_IMP)
>> +         imp = cimp;
>> +       /* BIG.little implementers are always equal. */
>
> See my comment above. This comment does not neccessarily hold.
>
> In addition, this needs updating for Kyrill's comments.
>
> Thanks,
> James
>

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

* Re: [PATCH 1/5] [AARCH64]: Move #undef into .def files.
  2015-11-18 12:29   ` Marcus Shawcroft
@ 2016-10-07 21:36     ` Andrew Pinski
  0 siblings, 0 replies; 19+ messages in thread
From: Andrew Pinski @ 2016-10-07 21:36 UTC (permalink / raw)
  To: Marcus Shawcroft; +Cc: Andrew Pinski, gcc-patches

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

On Wed, Nov 18, 2015 at 4:29 AM, Marcus Shawcroft
<marcus.shawcroft@gmail.com> wrote:
> On 17 November 2015 at 22:10, Andrew Pinski <apinski@cavium.com> wrote:
>>
>> This moves the #undef from the header files to the .def files like was done
>> for builtins.def (https://gcc.gnu.org/ml/gcc-patches/2015-11/msg00662.html).
>>
>> OK?   Bootstrapped and tested on aarch64-linux-gnu with no regressions.
>>
>> Thanks,
>> Andrew Pinski
>>
>> * config/aarch64/aarch64-arches.def (AARCH64_ARCH): #undef at the end.
>> * config/aarch64/aarch64-cores.def (AARCH64_CORE): Likewise.
>> * config/aarch64/aarch64-fusion-pairs.def (AARCH64_FUSION_PAIR): Likewise.
>> * config/aarch64/aarch64-tuning-flags.def (AARCH64_EXTRA_TUNING_OPTION): Likewise.
>> * config/aarch64/aarch64-opts.h (AARCH64_CORE): Don't #undef here.
>> (AARCH64_ARCH): Likewise.
>> * config/aarch64/aarch64-protos.h (AARCH64_FUSION_PAIR): Likewise.
>> (AARCH64_EXTRA_TUNING_OPTION): Likewise.
>> * config/aarch64/aarch64.c (AARCH64_FUION_PAIR): Likewise.
>> (AARCH64_EXTRA_TUNING_OPTION): Likewise.
>> (AARCH64_ARCH): Likewise.
>> (AARCH64_CORE): Likewise.
>> (AARCH64_OPT_EXTENSION): Likewise.
>> * config/aarch64/aarch64.h (AARCH64_CORE): Likewise.
>> * config/aarch64/driver-aarch64.c (AARCH64_OPT_EXTENSION): Likewise.
>> (AARCH64_CORE): Likewise.
>> (AARCH64_ARCH): Likewise.
>> * common/config/aarch64/aarch64-common.c: Likewise.
>
> OK Thanks /Marcus


This is what I applied finally after a new bootstrap/test.

By the way there was even a typo in the code originally for
AARCH64_FUSION_PAIR; there was an undef for AARCH64_FUION_PAIR in
config/aarch64/aarch64.c.  So this patch alone fixes that issue.

Thanks,
Andrew Pinski

2016-10-07  Andrew Pinski  <apinski@cavium.com>

        * config/aarch64/aarch64-arches.def (AARCH64_ARCH): #undef at the end.
        * config/aarch64/aarch64-cores.def (AARCH64_CORE): Likewise.
        * config/aarch64/aarch64-fusion-pairs.def
(AARCH64_FUSION_PAIR): Likewise.
        * config/aarch64/aarch64-option-extensions.def
(AARCH64_OPT_EXTENSION): Likewise.
        * config/aarch64/aarch64-tuning-flags.def
(AARCH64_EXTRA_TUNING_OPTION): Likewise.
        * config/aarch64/aarch64-opts.h (AARCH64_CORE): Don't #undef here.
        (AARCH64_ARCH): Likewise.
        * common/config/aarch64/aarch64-common.c
(AARCH64_OPT_EXTENSION): Likewise.
        (AARCH64_CORE): Likewise.
        (AARCH64_ARCH): Likewise.
        * config/aarch64/aarch64-protos.h (AARCH64_FUSION_PAIR): Likewise.
        (AARCH64_EXTRA_TUNING_OPTION): Likewise.
        * config/aarch64/aarch64.c (AARCH64_FUION_PAIR): Likewise.
        (AARCH64_EXTRA_TUNING_OPTION): Likewise.
        (AARCH64_ARCH): Likewise.
        (AARCH64_CORE): Likewise.
        * config/aarch64/aarch64.h (AARCH64_CORE): Likewise.
        * config/aarch64/driver-aarch64.c (AARCH64_OPT_EXTENSION): Likewise.
        (AARCH64_CORE): Likewise.
        (AARCH64_ARCH): Likewise.

[-- Attachment #2: undef.diff.txt --]
[-- Type: text/plain, Size: 8316 bytes --]

Index: common/config/aarch64/aarch64-common.c
===================================================================
--- common/config/aarch64/aarch64-common.c	(revision 240845)
+++ common/config/aarch64/aarch64-common.c	(working copy)
@@ -123,7 +123,6 @@ static const struct aarch64_option_exten
 #define AARCH64_OPT_EXTENSION(NAME, FLAG_CANONICAL, FLAGS_ON, FLAGS_OFF, Z) \
   {NAME, FLAG_CANONICAL, FLAGS_ON, FLAGS_OFF},
 #include "config/aarch64/aarch64-option-extensions.def"
-#undef AARCH64_OPT_EXTENSION
   {NULL, 0, 0, 0}
 };
 
@@ -148,7 +147,6 @@ static const struct processor_name_to_ar
 #define AARCH64_CORE(NAME, X, IDENT, ARCH_IDENT, FLAGS, COSTS, IMP, PART) \
   {NAME, AARCH64_ARCH_##ARCH_IDENT, FLAGS},
 #include "config/aarch64/aarch64-cores.def"
-#undef AARCH64_CORE
   {"generic", AARCH64_ARCH_8A, AARCH64_FL_FOR_ARCH8},
   {"", aarch64_no_arch, 0}
 };
@@ -159,7 +157,6 @@ static const struct arch_to_arch_name al
 #define AARCH64_ARCH(NAME, CORE, ARCH_IDENT, ARCH, FLAGS) \
   {AARCH64_ARCH_##ARCH_IDENT, NAME, FLAGS},
 #include "config/aarch64/aarch64-arches.def"
-#undef AARCH64_ARCH
   {aarch64_no_arch, "", 0}
 };
 
Index: config/aarch64/aarch64-arches.def
===================================================================
--- config/aarch64/aarch64-arches.def	(revision 240845)
+++ config/aarch64/aarch64-arches.def	(working copy)
@@ -34,3 +34,4 @@ AARCH64_ARCH("armv8-a",	      generic,
 AARCH64_ARCH("armv8.1-a",     generic,	     8_1A,	8,  AARCH64_FL_FOR_ARCH8_1)
 AARCH64_ARCH("armv8.2-a",     generic,	     8_2A,	8,  AARCH64_FL_FOR_ARCH8_2)
 
+#undef AARCH64_ARCH
Index: config/aarch64/aarch64-cores.def
===================================================================
--- config/aarch64/aarch64-cores.def	(revision 240846)
+++ config/aarch64/aarch64-cores.def	(working copy)
@@ -70,3 +70,5 @@ AARCH64_CORE("cortex-a57.cortex-a53",  c
 AARCH64_CORE("cortex-a72.cortex-a53",  cortexa72cortexa53, cortexa53, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC, cortexa72, "0x41", "0xd08.0xd03")
 AARCH64_CORE("cortex-a73.cortex-a35",  cortexa73cortexa35, cortexa53, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC, cortexa73, "0x41", "0xd09.0xd04")
 AARCH64_CORE("cortex-a73.cortex-a53",  cortexa73cortexa53, cortexa53, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC, cortexa73, "0x41", "0xd09.0xd03")
+
+#undef AARCH64_CORE
Index: config/aarch64/aarch64-fusion-pairs.def
===================================================================
--- config/aarch64/aarch64-fusion-pairs.def	(revision 240845)
+++ config/aarch64/aarch64-fusion-pairs.def	(working copy)
@@ -35,3 +35,4 @@ AARCH64_FUSION_PAIR ("adrp+ldr", ADRP_LD
 AARCH64_FUSION_PAIR ("cmp+branch", CMP_BRANCH)
 AARCH64_FUSION_PAIR ("aes+aesmc", AES_AESMC)
 
+#undef AARCH64_FUSION_PAIR
Index: config/aarch64/aarch64-option-extensions.def
===================================================================
--- config/aarch64/aarch64-option-extensions.def	(revision 240845)
+++ config/aarch64/aarch64-option-extensions.def	(working copy)
@@ -59,3 +59,5 @@ AARCH64_OPT_EXTENSION("lse", AARCH64_FL_
 /* Enabling "fp16" also enables "fp".
    Disabling "fp16" just disables "fp16".  */
 AARCH64_OPT_EXTENSION("fp16", AARCH64_FL_F16, AARCH64_FL_FP, 0, "fp16")
+
+#undef AARCH64_OPT_EXTENSION
Index: config/aarch64/aarch64-opts.h
===================================================================
--- config/aarch64/aarch64-opts.h	(revision 240845)
+++ config/aarch64/aarch64-opts.h	(working copy)
@@ -28,7 +28,6 @@ enum aarch64_processor
 #define AARCH64_CORE(NAME, INTERNAL_IDENT, SCHED, ARCH, FLAGS, COSTS, IMP, PART) \
   INTERNAL_IDENT,
 #include "aarch64-cores.def"
-#undef AARCH64_CORE
   /* Used to indicate that no processor has been specified.  */
   generic,
   /* Used to mark the end of the processor table.  */
@@ -40,7 +39,6 @@ enum aarch64_arch
 #define AARCH64_ARCH(NAME, CORE, ARCH_IDENT, ARCH_REV, FLAGS) \
   AARCH64_ARCH_##ARCH_IDENT,
 #include "aarch64-arches.def"
-#undef AARCH64_ARCH
   aarch64_no_arch
 };
 
Index: config/aarch64/aarch64-protos.h
===================================================================
--- config/aarch64/aarch64-protos.h	(revision 240845)
+++ config/aarch64/aarch64-protos.h	(working copy)
@@ -248,7 +248,6 @@ enum aarch64_fusion_pairs_index
 #include "aarch64-fusion-pairs.def"
   AARCH64_FUSE_index_END
 };
-#undef AARCH64_FUSION_PAIR
 
 #define AARCH64_FUSION_PAIR(x, name) \
   AARCH64_FUSE_##name = (1u << AARCH64_FUSE_##name##_index),
@@ -259,7 +258,6 @@ enum aarch64_fusion_pairs
 #include "aarch64-fusion-pairs.def"
   AARCH64_FUSE_ALL = (1u << AARCH64_FUSE_index_END) - 1
 };
-#undef AARCH64_FUSION_PAIR
 
 #define AARCH64_EXTRA_TUNING_OPTION(x, name) \
   AARCH64_EXTRA_TUNE_##name##_index,
@@ -269,7 +267,6 @@ enum aarch64_extra_tuning_flags_index
 #include "aarch64-tuning-flags.def"
   AARCH64_EXTRA_TUNE_index_END
 };
-#undef AARCH64_EXTRA_TUNING_OPTION
 
 
 #define AARCH64_EXTRA_TUNING_OPTION(x, name) \
@@ -281,7 +278,6 @@ enum aarch64_extra_tuning_flags
 #include "aarch64-tuning-flags.def"
   AARCH64_EXTRA_TUNE_ALL = (1u << AARCH64_EXTRA_TUNE_index_END) - 1
 };
-#undef AARCH64_EXTRA_TUNING_OPTION
 
 /* Enum describing the various ways that the
    aarch64_parse_{arch,tune,cpu,extension} functions can fail.
Index: config/aarch64/aarch64-tuning-flags.def
===================================================================
--- config/aarch64/aarch64-tuning-flags.def	(revision 240845)
+++ config/aarch64/aarch64-tuning-flags.def	(working copy)
@@ -34,3 +34,5 @@ AARCH64_EXTRA_TUNING_OPTION ("rename_fma
 two load/stores are not at least 8 byte aligned don't create load/store
 pairs.   */
 AARCH64_EXTRA_TUNING_OPTION ("slow_unaligned_ldpw", SLOW_UNALIGNED_LDPW)
+
+#undef AARCH64_EXTRA_TUNING_OPTION
Index: config/aarch64/aarch64.c
===================================================================
--- config/aarch64/aarch64.c	(revision 240845)
+++ config/aarch64/aarch64.c	(working copy)
@@ -172,7 +172,6 @@ static const struct aarch64_flag_desc aa
   { "all", AARCH64_FUSE_ALL },
   { NULL, AARCH64_FUSE_NOTHING }
 };
-#undef AARCH64_FUION_PAIR
 
 #define AARCH64_EXTRA_TUNING_OPTION(name, internal_name) \
   { name, AARCH64_EXTRA_TUNE_##internal_name },
@@ -183,7 +182,6 @@ static const struct aarch64_flag_desc aa
   { "all", AARCH64_EXTRA_TUNE_ALL },
   { NULL, AARCH64_EXTRA_TUNE_NONE }
 };
-#undef AARCH64_EXTRA_TUNING_OPTION
 
 /* Tuning parameters.  */
 
@@ -828,7 +826,6 @@ static const struct processor all_archit
 #define AARCH64_ARCH(NAME, CORE, ARCH_IDENT, ARCH_REV, FLAGS) \
   {NAME, CORE, CORE, AARCH64_ARCH_##ARCH_IDENT, ARCH_REV, FLAGS, NULL},
 #include "aarch64-arches.def"
-#undef AARCH64_ARCH
   {NULL, aarch64_none, aarch64_none, aarch64_no_arch, 0, 0, NULL}
 };
 
@@ -840,7 +837,6 @@ static const struct processor all_cores[
   all_architectures[AARCH64_ARCH_##ARCH].architecture_version,	\
   FLAGS, &COSTS##_tunings},
 #include "aarch64-cores.def"
-#undef AARCH64_CORE
   {"generic", generic, cortexa53, AARCH64_ARCH_8A, 8,
     AARCH64_FL_FOR_ARCH8, &generic_tunings},
   {NULL, aarch64_none, aarch64_none, aarch64_no_arch, 0, 0, NULL}
Index: config/aarch64/aarch64.h
===================================================================
--- config/aarch64/aarch64.h	(revision 240845)
+++ config/aarch64/aarch64.h	(working copy)
@@ -493,7 +493,6 @@ enum target_cpus
 #define AARCH64_CORE(NAME, INTERNAL_IDENT, SCHED, ARCH, FLAGS, COSTS, IMP, PART) \
   TARGET_CPU_##INTERNAL_IDENT,
 #include "aarch64-cores.def"
-#undef AARCH64_CORE
   TARGET_CPU_generic
 };
 
Index: config/aarch64/driver-aarch64.c
===================================================================
--- config/aarch64/driver-aarch64.c	(revision 240845)
+++ config/aarch64/driver-aarch64.c	(working copy)
@@ -40,7 +40,6 @@ static struct aarch64_arch_extension aar
 {
 #include "aarch64-option-extensions.def"
 };
-#undef AARCH64_OPT_EXTENSION
 
 
 struct aarch64_core_data
@@ -61,7 +60,6 @@ static struct aarch64_core_data aarch64_
   { NULL, NULL, NULL, NULL, 0 }
 };
 
-#undef AARCH64_CORE
 
 struct aarch64_arch_driver_info
 {
@@ -79,7 +77,6 @@ static struct aarch64_arch_driver_info a
   {NULL, NULL, 0}
 };
 
-#undef AARCH64_ARCH
 
 /* Return an aarch64_arch_driver_info for the architecture described
    by ID, or NULL if ID describes something we don't know about.  */

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

* Re: [PATCH 2/5] [AARCH64] Change IMP and PART over to integers from strings.
  2015-11-25 20:12     ` Andrew Pinski
@ 2016-10-16  2:38       ` Andrew Pinski
  2016-10-21 13:59         ` James Greenhalgh
  0 siblings, 1 reply; 19+ messages in thread
From: Andrew Pinski @ 2016-10-16  2:38 UTC (permalink / raw)
  To: James Greenhalgh; +Cc: Andrew Pinski, GCC Patches

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

On Wed, Nov 25, 2015 at 11:59 AM, Andrew Pinski <pinskia@gmail.com> wrote:
> On Wed, Nov 25, 2015 at 2:31 AM, James Greenhalgh
> <james.greenhalgh@arm.com> wrote:
>> On Tue, Nov 17, 2015 at 02:10:35PM -0800, Andrew Pinski wrote:
>>>
>>> Because the imp and parts are really integer rather than strings, this patch
>>> moves the comparisons to be integer.  Also allows saving around integers are
>>> easier than doing string comparisons.  This allows for the next change.
>>>
>>> The way I store BIG.little is (big<<12)|little as each part num is only 12bits
>>> long.  So it would be nice if someone could test -mpu=native on a big.little
>>> system to make sure it works still.
>>>
>>> OK?  Bootstrapped and tested on aarch64-linux-gnu with no regressions.
>>>
>>> Thanks,
>>> Andrew Pinski
>>>
>>>
>>>
>>> * config/aarch64/aarch64-cores.def: Rewrite so IMP and PART are integer constants.
>>> * config/aarch64/driver-aarch64.c (struct aarch64_core_data): Change implementer_id to unsigned char.
>>> Change part_no to unsigned int.
>>> (AARCH64_BIG_LITTLE): New define.
>>> (INVALID_IMP): New define.
>>> (INVALID_CORE): New define.
>>> (cpu_data): Change the last element's implementer_id and part_no to integers.
>>> (valid_bL_string_p): Rewrite to ..
>>> (valid_bL_core_p): this for integers instead of strings.
>>> (parse_field): New function.
>>> (contains_string_p): Rewrite to ...
>>> (contains_core_p): this for integers and only for the part_no.
>>> (host_detect_local_cpu): Rewrite handling of implementation and part num to be integers;
>>> simplifying the code.
>>> ---
>>>  gcc/config/aarch64/aarch64-cores.def | 25 +++++-----
>>>  gcc/config/aarch64/driver-aarch64.c  | 90 ++++++++++++++++++++----------------
>>>  2 files changed, 62 insertions(+), 53 deletions(-)
>>>
>>> diff --git a/gcc/config/aarch64/aarch64-cores.def b/gcc/config/aarch64/aarch64-cores.def
>>> index 0b456f7..798f3e3 100644
>>> --- a/gcc/config/aarch64/aarch64-cores.def
>>> +++ b/gcc/config/aarch64/aarch64-cores.def
>>> @@ -33,25 +33,26 @@
>>>     This need not include flags implied by the architecture.
>>>     COSTS is the name of the rtx_costs routine to use.
>>>     IMP is the implementer ID of the CPU vendor.  On a GNU/Linux system it can
>>> -   be found in /proc/cpuinfo.
>>> +   be found in /proc/cpuinfo.  There is a list in the ARM ARM.
>>
>> Let's be precise with this comment.
>>
>>   "A partial list of implementer IDs is given in the ARM Architecture
>>    Reference Manual ARMv8, for ARMv8-A architecture profile"
>>
>>>     PART is the part number of the CPU.  On a GNU/Linux system it can be found
>>> -   in /proc/cpuinfo.  For big.LITTLE systems this should have the form at of
>>> -   "<big core part number>.<LITTLE core part number>".  */
>>> +   in /proc/cpuinfo.  For big.LITTLE systems this should use the macro AARCH64_BIG_LITTLE
>>> +   where the big part number comes as the first arugment to the macro and little is the
>>
>> s/arugment/argument/.
>>
>>> +   second.  */
>>>
>>>  /* V8 Architecture Processors.  */
>>>
>>> -AARCH64_CORE("cortex-a53",  cortexa53, cortexa53, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC, cortexa53, "0x41", "0xd03")
>>> -AARCH64_CORE("cortex-a57",  cortexa57, cortexa57, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC, cortexa57, "0x41", "0xd07")
>>> -AARCH64_CORE("cortex-a72",  cortexa72, cortexa57, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC, cortexa72, "0x41", "0xd08")
>>> -AARCH64_CORE("exynos-m1",   exynosm1,  cortexa57, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC | AARCH64_FL_CRYPTO, cortexa72, "0x53", "0x001")
>>> -AARCH64_CORE("qdf24xx",     qdf24xx,   cortexa57, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC | AARCH64_FL_CRYPTO, cortexa57, "0x51", "0x800")
>>> -AARCH64_CORE("thunderx",    thunderx,  thunderx,  8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC | AARCH64_FL_CRYPTO, thunderx,  "0x43", "0x0a1")
>>> -AARCH64_CORE("xgene1",      xgene1,    xgene1,    8A,  AARCH64_FL_FOR_ARCH8, xgene1, "0x50", "0x000")
>>> +AARCH64_CORE("cortex-a53",  cortexa53, cortexa53, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC, cortexa53, 0x41, 0xd03)
>>> +AARCH64_CORE("cortex-a57",  cortexa57, cortexa57, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC, cortexa57, 0x41, 0xd07)
>>> +AARCH64_CORE("cortex-a72",  cortexa72, cortexa57, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC, cortexa72, 0x41, 0xd08)
>>> +AARCH64_CORE("exynos-m1",   exynosm1,  cortexa57, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC | AARCH64_FL_CRYPTO, cortexa72, 0x53, 0x001)
>>> +AARCH64_CORE("qdf24xx",     qdf24xx,   cortexa57, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC | AARCH64_FL_CRYPTO, cortexa57, 0x51, 0x800)
>>> +AARCH64_CORE("thunderx",    thunderx,  thunderx,  8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC | AARCH64_FL_CRYPTO, thunderx,  0x43, 0x0a1)
>>> +AARCH64_CORE("xgene1",      xgene1,    xgene1,    8A,  AARCH64_FL_FOR_ARCH8, xgene1, 0x50, 0x000)
>>>
>>>  /* V8 big.LITTLE implementations.  */
>>>
>>> -AARCH64_CORE("cortex-a57.cortex-a53",  cortexa57cortexa53, cortexa53, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC, cortexa57, "0x41", "0xd07.0xd03")
>>> -AARCH64_CORE("cortex-a72.cortex-a53",  cortexa72cortexa53, cortexa53, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC, cortexa72, "0x41", "0xd08.0xd03")
>>> +AARCH64_CORE("cortex-a57.cortex-a53",  cortexa57cortexa53, cortexa53, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC, cortexa57, 0x41, AARCH64_BIG_LITTLE(0xd07, 0xd03))
>>> +AARCH64_CORE("cortex-a72.cortex-a53",  cortexa72cortexa53, cortexa53, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC, cortexa72, 0x41, AARCH64_BIG_LITTLE(0xd08, 0xd03))
>>>
>>>
>>>  #undef AARCH64_CORE
>>> diff --git a/gcc/config/aarch64/driver-aarch64.c b/gcc/config/aarch64/driver-aarch64.c
>>> index d03654c..92388a9 100644
>>> --- a/gcc/config/aarch64/driver-aarch64.c
>>> +++ b/gcc/config/aarch64/driver-aarch64.c
>>> @@ -37,18 +37,23 @@ static struct arch_extension ext_to_feat_string[] =
>>>  struct aarch64_core_data
>>>  {
>>>    const char* name;
>>> -  const char* arch;
>>> -  const char* implementer_id;
>>> -  const char* part_no;
>>> +  const char *arch;
>>> +  unsigned char implementer_id; /* Exactly 8 bits */
>>
>> Can we redesign this around the most general case - big.LITTLE cores with
>> different implementer IDs? There is nothing in the architecture that would
>> forbid this, and Samsung recently announced that their Exynos 8 Octa 8890
>> which would support a combination of four of their custom cores with four ARM
>> Cortex-A53 cores.
>>
>>  ( http://www.samsung.com/semiconductor/minisite/Exynos/w/mediacenter.html#?v=news_Samsung_Unveils_a_New_Exynos_8_Octa_Processor_based_on_advanced_14-Nanometer_FinFET_Process_Technology )
>
> This should be done a separate patch as even the current
> infrastructure does not support it at all.
> It would be a simple one though.  Change implementer_id to int and
> then encode the two implementer_id in cores_data.  Though the parsing
> part becomes more complex as the current parsing does not understand
> if a new core has been started or not.  This is why it should be a
> separate patch instead.
>
> Maybe to support that one it is not about parsing /proc/cpuinfo but
> rather /sys/cpu/* instead which is supported for Linux 4.4 (or did not
> make it into 4.4 so 4.5).
>
> Thanks,
> Andrew
>
>>
>>> +  unsigned int part_no; /* 12 bits + 12 bits */
>>>  };
>>>
>>> +#define AARCH64_BIG_LITTLE(BIG, LITTLE) \
>>> +  (((BIG)&0xFFFu) << 12 | ((LITTLE) & 0xFFFu))
>>> +#define INVALID_IMP ((unsigned char) -1)
>>> +#define INVALID_CORE ((unsigned)-1)
>>> +
>>>  #define AARCH64_CORE(CORE_NAME, CORE_IDENT, SCHED, ARCH, FLAGS, COSTS, IMP, PART) \
>>>    { CORE_NAME, #ARCH, IMP, PART },
>>>
>>>  static struct aarch64_core_data cpu_data [] =
>>>  {
>>>  #include "aarch64-cores.def"
>>> -  { NULL, NULL, NULL, NULL }
>>> +  { NULL, NULL, INVALID_IMP, INVALID_CORE }
>>>  };
>>>
>>>
>>> @@ -91,27 +96,35 @@ get_arch_name_from_id (const char* id)
>>>     should return true.  */
>>>
>>>  static bool
>>> -valid_bL_string_p (const char** core, const char* bL_string)
>>> +valid_bL_core_p (unsigned int *core, unsigned int bL_core)
>>>  {
>>> -  return strstr (bL_string, core[0]) != NULL
>>> -         && strstr (bL_string, core[1]) != NULL;
>>> +  return AARCH64_BIG_LITTLE (core[0], core[1]) == bL_core;
>>>  }
>>>
>>> -/*  Return true iff ARR contains STR in one of its two elements.  */
>>>
>>> -static bool
>>> -contains_string_p (const char** arr, const char* str)
>>> +/* Returns the integer that is after ':' for the field. */
>>> +static unsigned parse_field (const char *field)
>>>  {
>>> -  bool res = false;
>>> +  const char *rest = strchr (field, ':');
>>> +  char *after;
>>> +  unsigned fint = strtol (rest+1, &after, 16);
>>> +  if (after == rest+1)
>>
>> "rest + 1" in both cases.
>>
>>> +    return -1;
>>> +  return fint;
>>> +}
>>> +
>>> +/*  Return true iff ARR contains CORE, in either of the two elements. */
>>>
>>> -  if (arr[0] != NULL)
>>> +static bool
>>> +contains_core_p (unsigned *arr, unsigned core)
>>> +{
>>> +  if (arr[0] != INVALID_CORE)
>>>      {
>>> -      res = strstr (arr[0], str) != NULL;
>>> -      if (res)
>>> -        return res;
>>> +      if (arr[0] == core)
>>> +        return true;
>>>
>>> -      if (arr[1] != NULL)
>>> -        return strstr (arr[1], str) != NULL;
>>> +      if (arr[1] != INVALID_CORE)
>>> +        return arr[1] == core;
>>>      }
>>>
>>>    return false;
>>> @@ -146,10 +159,9 @@ host_detect_local_cpu (int argc, const char **argv)
>>>    bool cpu = false;
>>>    unsigned int i = 0;
>>>    unsigned int core_idx = 0;
>>> -  const char* imps[2] = { NULL, NULL };
>>> -  const char* cores[2] = { NULL, NULL };
>>> +  unsigned char imp = INVALID_IMP;
>>> +  unsigned int cores[2] = { INVALID_CORE, INVALID_CORE };
>>>    unsigned int n_cores = 0;
>>> -  unsigned int n_imps = 0;
>>>    bool processed_exts = false;
>>>    const char *ext_string = "";
>>>
>>> @@ -180,30 +192,28 @@ host_detect_local_cpu (int argc, const char **argv)
>>>      {
>>>        if (strstr (buf, "implementer") != NULL)
>>>       {
>>> -       for (i = 0; cpu_data[i].name != NULL; i++)
>>> -         if (strstr (buf, cpu_data[i].implementer_id) != NULL
>>> -                && !contains_string_p (imps, cpu_data[i].implementer_id))
>>> -           {
>>> -                if (n_imps == 2)
>>> -                  goto not_found;
>>> -
>>> -                imps[n_imps++] = cpu_data[i].implementer_id;
>>> -
>>> -                break;
>>> -           }
>>> -          continue;
>>> +       unsigned cimp = parse_field (buf);
>>> +       if (cimp == INVALID_IMP)
>>> +         goto not_found;
>>> +
>>> +       if (imp == INVALID_IMP)
>>> +         imp = cimp;
>>> +       /* BIG.little implementers are always equal. */
>>
>> See my comment above. This comment does not neccessarily hold.
>>
>> In addition, this needs updating for Kyrill's comments.


Here is finally an updated (fixed) patch (I did not implement the two
implementer big.LITTLE support yet, that will be for a different patch
since I also fixed the part no not being unique as a separate patch.
Once I get a new enough kernel, I will also look into doing the
/sys/cpu/* style detection first.

OK? Bootstrapped and tested on aarch64-linux-gnu with no regressions
(and tested hacking the location of the read in file to see if it
works with big.LITTLE and other formats of /proc/cpuinfo).

Thanks,
Andrew Pinski

* config/aarch64/aarch64-cores.def: Rewrite so IMP and PART are
integer constants.
* config/aarch64/driver-aarch64.c (struct aarch64_core_data): Change
implementer_id to unsigned char.
Change part_no to unsigned int.
(AARCH64_BIG_LITTLE): New define.
(INVALID_IMP): New define.
(INVALID_CORE): New define.
(cpu_data): Change the last element's implementer_id and part_no to integers.
(valid_bL_string_p): Rewrite to ..
(valid_bL_core_p): this for integers instead of strings.
(parse_field): New function.
(contains_string_p): Rewrite to ...
(contains_core_p): this for integers and only for the part_no.
(host_detect_local_cpu): Rewrite handling of implementation and part
num to be integers;
simplifying the code.

>>
>> Thanks,
>> James
>>

[-- Attachment #2: changetointfromstring.diff.txt --]
[-- Type: text/plain, Size: 10919 bytes --]

Index: config/aarch64/aarch64-cores.def
===================================================================
--- config/aarch64/aarch64-cores.def	(revision 241200)
+++ config/aarch64/aarch64-cores.def	(working copy)
@@ -32,43 +32,46 @@
    FLAGS are the bitwise-or of the traits that apply to that core.
    This need not include flags implied by the architecture.
    COSTS is the name of the rtx_costs routine to use.
-   IMP is the implementer ID of the CPU vendor.  On a GNU/Linux system it can
-   be found in /proc/cpuinfo.
+   IMP is the implementer ID of the CPU vendor.  On a GNU/Linux system it
+   can be found in /proc/cpuinfo. A partial list of implementer IDs is
+   given in the ARM Architecture Reference Manual ARMv8, for
+   ARMv8-A architecture profile.
    PART is the part number of the CPU.  On a GNU/Linux system it can be found
-   in /proc/cpuinfo.  For big.LITTLE systems this should have the form at of
-   "<big core part number>.<LITTLE core part number>".  */
+   in /proc/cpuinfo.  For big.LITTLE systems this should use the macro AARCH64_BIG_LITTLE
+   where the big part number comes as the first arugment to the macro and little is the
+   second.  */
 
 /* V8 Architecture Processors.  */
 
 /* ARM ('A') cores. */
-AARCH64_CORE("cortex-a35",  cortexa35, cortexa53, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC, cortexa35, "0x41", "0xd04")
-AARCH64_CORE("cortex-a53",  cortexa53, cortexa53, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC, cortexa53, "0x41", "0xd03")
-AARCH64_CORE("cortex-a57",  cortexa57, cortexa57, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC, cortexa57, "0x41", "0xd07")
-AARCH64_CORE("cortex-a72",  cortexa72, cortexa57, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC, cortexa72, "0x41", "0xd08")
-AARCH64_CORE("cortex-a73",  cortexa73, cortexa57, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC, cortexa73, "0x41", "0xd09")
+AARCH64_CORE("cortex-a35",  cortexa35, cortexa53, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC, cortexa35, 0x41, 0xd04)
+AARCH64_CORE("cortex-a53",  cortexa53, cortexa53, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC, cortexa53, 0x41, 0xd03)
+AARCH64_CORE("cortex-a57",  cortexa57, cortexa57, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC, cortexa57, 0x41, 0xd07)
+AARCH64_CORE("cortex-a72",  cortexa72, cortexa57, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC, cortexa72, 0x41, 0xd08)
+AARCH64_CORE("cortex-a73",  cortexa73, cortexa57, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC, cortexa73, 0x41, 0xd09)
 
 /* Samsung ('S') cores. */
-AARCH64_CORE("exynos-m1",   exynosm1,  exynosm1,  8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC | AARCH64_FL_CRYPTO, exynosm1,  "0x53", "0x001")
+AARCH64_CORE("exynos-m1",   exynosm1,  exynosm1,  8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC | AARCH64_FL_CRYPTO, exynosm1,  0x53, 0x001)
 
 /* Qualcomm ('Q') cores. */
-AARCH64_CORE("qdf24xx",     qdf24xx,   cortexa57, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC | AARCH64_FL_CRYPTO, qdf24xx,   "0x51", "0x800")
+AARCH64_CORE("qdf24xx",     qdf24xx,   cortexa57, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC | AARCH64_FL_CRYPTO, qdf24xx,   0x51, 0x800)
 
 /* Cavium ('C') cores. */
-AARCH64_CORE("thunderx",    thunderx,  thunderx,  8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC | AARCH64_FL_CRYPTO, thunderx,  "0x43", "0x0a1")
+AARCH64_CORE("thunderx",    thunderx,  thunderx,  8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC | AARCH64_FL_CRYPTO, thunderx,  0x43, 0x0a1)
 
 /* APM ('P') cores. */
-AARCH64_CORE("xgene1",      xgene1,    xgene1,    8A,  AARCH64_FL_FOR_ARCH8, xgene1, "0x50", "0x000")
+AARCH64_CORE("xgene1",      xgene1,    xgene1,    8A,  AARCH64_FL_FOR_ARCH8, xgene1, 0x50, 0x000)
 
 /* V8.1 Architecture Processors.  */
 
 /* Broadcom ('B') cores. */
-AARCH64_CORE("vulcan",  vulcan, cortexa57, 8_1A,  AARCH64_FL_FOR_ARCH8_1 | AARCH64_FL_CRYPTO, vulcan, "0x42", "0x516")
+AARCH64_CORE("vulcan",  vulcan, cortexa57, 8_1A,  AARCH64_FL_FOR_ARCH8_1 | AARCH64_FL_CRYPTO, vulcan, 0x42, 0x516)
 
 /* V8 big.LITTLE implementations.  */
 
-AARCH64_CORE("cortex-a57.cortex-a53",  cortexa57cortexa53, cortexa53, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC, cortexa57, "0x41", "0xd07.0xd03")
-AARCH64_CORE("cortex-a72.cortex-a53",  cortexa72cortexa53, cortexa53, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC, cortexa72, "0x41", "0xd08.0xd03")
-AARCH64_CORE("cortex-a73.cortex-a35",  cortexa73cortexa35, cortexa53, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC, cortexa73, "0x41", "0xd09.0xd04")
-AARCH64_CORE("cortex-a73.cortex-a53",  cortexa73cortexa53, cortexa53, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC, cortexa73, "0x41", "0xd09.0xd03")
+AARCH64_CORE("cortex-a57.cortex-a53",  cortexa57cortexa53, cortexa53, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC, cortexa57, 0x41, AARCH64_BIG_LITTLE (0xd07, 0xd03))
+AARCH64_CORE("cortex-a72.cortex-a53",  cortexa72cortexa53, cortexa53, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC, cortexa72, 0x41, AARCH64_BIG_LITTLE (0xd08, 0xd03))
+AARCH64_CORE("cortex-a73.cortex-a35",  cortexa73cortexa35, cortexa53, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC, cortexa73, 0x41, AARCH64_BIG_LITTLE (0xd09, 0xd04))
+AARCH64_CORE("cortex-a73.cortex-a53",  cortexa73cortexa53, cortexa53, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC, cortexa73, 0x41, AARCH64_BIG_LITTLE (0xd09, 0xd03))
 
 #undef AARCH64_CORE
Index: config/aarch64/driver-aarch64.c
===================================================================
--- config/aarch64/driver-aarch64.c	(revision 241200)
+++ config/aarch64/driver-aarch64.c	(working copy)
@@ -46,18 +46,23 @@ struct aarch64_core_data
 {
   const char* name;
   const char* arch;
-  const char* implementer_id;
-  const char* part_no;
+  unsigned char implementer_id; /* Exactly 8 bits */
+  unsigned int part_no; /* 12 bits + 12 bits */
   const unsigned long flags;
 };
 
+#define AARCH64_BIG_LITTLE(BIG, LITTLE) \
+  (((BIG)&0xFFFu) << 12 | ((LITTLE) & 0xFFFu))
+#define INVALID_IMP ((unsigned char) -1)
+#define INVALID_CORE ((unsigned)-1)
+
 #define AARCH64_CORE(CORE_NAME, CORE_IDENT, SCHED, ARCH, FLAGS, COSTS, IMP, PART) \
   { CORE_NAME, #ARCH, IMP, PART, FLAGS },
 
 static struct aarch64_core_data aarch64_cpu_data[] =
 {
 #include "aarch64-cores.def"
-  { NULL, NULL, NULL, NULL, 0 }
+  { NULL, NULL, INVALID_IMP, INVALID_CORE, 0 }
 };
 
 
@@ -95,32 +100,40 @@ get_arch_from_id (const char* id)
   return NULL;
 }
 
-/* Check wether the string CORE contains the same CPU part numbers
-   as BL_STRING.  For example CORE="{0xd03, 0xd07}" and BL_STRING="0xd07.0xd03"
-   should return true.  */
+/* Check wether the CORE array is the same as the big.LITTLE BL_CORE.
+   For an example CORE={0xd08, 0xd03} and
+   BL_CORE=AARCH64_BIG_LITTLE (0xd08, 0xd03) will return true.  */
 
-static bool
-valid_bL_string_p (const char** core, const char* bL_string)
+ static bool
+valid_bL_core_p (unsigned int *core, unsigned int bL_core)
 {
-  return strstr (bL_string, core[0]) != NULL
-    && strstr (bL_string, core[1]) != NULL;
+  return AARCH64_BIG_LITTLE (core[0], core[1]) == bL_core
+         || AARCH64_BIG_LITTLE (core[1], core[0]) == bL_core;
+}
+
+/* Returns the integer that is after ':' for the field. */
+static unsigned parse_field (const char *field)
+ {
+  const char *rest = strchr (field, ':');
+  char *after;
+  unsigned fint = strtol (rest + 1, &after, 16);
+  if (after == rest + 1)
+    return -1;
+  return fint;
 }
 
-/*  Return true iff ARR contains STR in one of its two elements.  */
+/*  Return true iff ARR contains CORE, in either of the two elements. */
 
 static bool
-contains_string_p (const char** arr, const char* str)
+contains_core_p (unsigned *arr, unsigned core)
 {
-  bool res = false;
-
-  if (arr[0] != NULL)
+  if (arr[0] != INVALID_CORE)
     {
-      res = strstr (arr[0], str) != NULL;
-      if (res)
-        return res;
+      if (arr[0] == core)
+        return true;
 
-      if (arr[1] != NULL)
-        return strstr (arr[1], str) != NULL;
+      if (arr[1] != INVALID_CORE)
+        return arr[1] == core;
     }
 
   return false;
@@ -155,10 +168,9 @@ host_detect_local_cpu (int argc, const c
   bool cpu = false;
   unsigned int i = 0;
   unsigned int core_idx = 0;
-  const char* imps[2] = { NULL, NULL };
-  const char* cores[2] = { NULL, NULL };
+  unsigned char imp = INVALID_IMP;
+  unsigned int cores[2] = { INVALID_CORE, INVALID_CORE };
   unsigned int n_cores = 0;
-  unsigned int n_imps = 0;
   bool processed_exts = false;
   const char *ext_string = "";
   unsigned long extension_flags = 0;
@@ -191,31 +203,28 @@ host_detect_local_cpu (int argc, const c
     {
       if (strstr (buf, "implementer") != NULL)
 	{
-	  for (i = 0; aarch64_cpu_data[i].name != NULL; i++)
-	    if (strstr (buf, aarch64_cpu_data[i].implementer_id) != NULL
-		&& !contains_string_p (imps,
-				       aarch64_cpu_data[i].implementer_id))
-	      {
-		if (n_imps == 2)
-		  goto not_found;
-
-		imps[n_imps++] = aarch64_cpu_data[i].implementer_id;
-
-		break;
-	      }
-	  continue;
+	  unsigned cimp = parse_field (buf);
+	  if (cimp == INVALID_IMP)
+	    goto not_found;
+
+	  if (imp == INVALID_IMP)
+	    imp = cimp;
+	  /* FIXME: BIG.little implementers are always equal. */
+	  else if (imp != cimp)
+	    goto not_found;
 	}
 
       if (strstr (buf, "part") != NULL)
 	{
+	  unsigned ccore = parse_field (buf);
 	  for (i = 0; aarch64_cpu_data[i].name != NULL; i++)
-	    if (strstr (buf, aarch64_cpu_data[i].part_no) != NULL
-		&& !contains_string_p (cores, aarch64_cpu_data[i].part_no))
+	    if (ccore == aarch64_cpu_data[i].part_no
+		&& !contains_core_p (cores, ccore))
 	      {
 		if (n_cores == 2)
 		  goto not_found;
 
-		cores[n_cores++] = aarch64_cpu_data[i].part_no;
+		cores[n_cores++] = ccore;
 		core_idx = i;
 		arch_id = aarch64_cpu_data[i].arch;
 		break;
@@ -262,7 +271,7 @@ host_detect_local_cpu (int argc, const c
   f = NULL;
 
   /* Weird cpuinfo format that we don't know how to handle.  */
-  if (n_cores == 0 || n_cores > 2 || n_imps != 1)
+  if (n_cores == 0 || n_cores > 2 || imp == INVALID_IMP)
     goto not_found;
 
   if (arch && !arch_id)
@@ -284,11 +293,8 @@ host_detect_local_cpu (int argc, const c
     {
       for (i = 0; aarch64_cpu_data[i].name != NULL; i++)
 	{
-	  if (strchr (aarch64_cpu_data[i].part_no, '.') != NULL
-	      && strncmp (aarch64_cpu_data[i].implementer_id,
-			  imps[0],
-			  strlen (imps[0]) - 1) == 0
-	      && valid_bL_string_p (cores, aarch64_cpu_data[i].part_no))
+	  if (aarch64_cpu_data[i].implementer_id == imp
+	      && valid_bL_core_p (cores, aarch64_cpu_data[i].part_no))
 	    {
 	      res = concat ("-m",
 			    cpu ? "cpu" : "tune", "=",
@@ -304,8 +310,7 @@ host_detect_local_cpu (int argc, const c
   /* The simple, non-big.LITTLE case.  */
   else
     {
-      if (strncmp (aarch64_cpu_data[core_idx].implementer_id, imps[0],
-		   strlen (imps[0]) - 1) != 0)
+      if (aarch64_cpu_data[core_idx].implementer_id != imp)
 	goto not_found;
 
       res = concat ("-m", cpu ? "cpu" : "tune", "=",

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

* Re: [PATCH 2/5] [AARCH64] Change IMP and PART over to integers from strings.
  2016-10-16  2:38       ` Andrew Pinski
@ 2016-10-21 13:59         ` James Greenhalgh
  2016-10-21 15:57           ` Richard Earnshaw (lists)
  0 siblings, 1 reply; 19+ messages in thread
From: James Greenhalgh @ 2016-10-21 13:59 UTC (permalink / raw)
  To: Andrew Pinski; +Cc: Andrew Pinski, GCC Patches, nd

On Sat, Oct 15, 2016 at 07:38:40PM -0700, Andrew Pinski wrote:
> On Wed, Nov 25, 2015 at 11:59 AM, Andrew Pinski <pinskia@gmail.com> wrote:
> Here is finally an updated (fixed) patch (I did not implement the two
> implementer big.LITTLE support yet, that will be for a different patch
> since I also fixed the part no not being unique as a separate patch.
> Once I get a new enough kernel, I will also look into doing the
> /sys/cpu/* style detection first.
> 
> OK? Bootstrapped and tested on aarch64-linux-gnu with no regressions
> (and tested hacking the location of the read in file to see if it
> works with big.LITTLE and other formats of /proc/cpuinfo).

I'm OK with this in principle, but it needs some polish for pedantic
style comments...

> * config/aarch64/aarch64-cores.def: Rewrite so IMP and PART are
> integer constants.
> * config/aarch64/driver-aarch64.c (struct aarch64_core_data): Change
> implementer_id to unsigned char.
> Change part_no to unsigned int.
> (AARCH64_BIG_LITTLE): New define.
> (INVALID_IMP): New define.
> (INVALID_CORE): New define.
> (cpu_data): Change the last element's implementer_id and part_no to integers.
> (valid_bL_string_p): Rewrite to ..
> (valid_bL_core_p): this for integers instead of strings.
> (parse_field): New function.
> (contains_string_p): Rewrite to ...
> (contains_core_p): this for integers and only for the part_no.
> (host_detect_local_cpu): Rewrite handling of implementation and part
> num to be integers;
> simplifying the code.

> Index: config/aarch64/aarch64-cores.def
> ===================================================================
> --- config/aarch64/aarch64-cores.def	(revision 241200)
> +++ config/aarch64/aarch64-cores.def	(working copy)
> @@ -32,43 +32,46 @@
>     FLAGS are the bitwise-or of the traits that apply to that core.
>     This need not include flags implied by the architecture.
>     COSTS is the name of the rtx_costs routine to use.
> -   IMP is the implementer ID of the CPU vendor.  On a GNU/Linux system it can
> -   be found in /proc/cpuinfo.
> +   IMP is the implementer ID of the CPU vendor.  On a GNU/Linux system it
> +   can be found in /proc/cpuinfo. A partial list of implementer IDs is
> +   given in the ARM Architecture Reference Manual ARMv8, for
> -   in /proc/cpuinfo.  For big.LITTLE systems this should have the form at of
> -   "<big core part number>.<LITTLE core part number>".  */
> +   in /proc/cpuinfo.  For big.LITTLE systems this should use the macro AARCH64_BIG_LITTLE
> +   where the big part number comes as the first arugment to the macro and little is the
> +   second.  */

Needs rewrapped for 80 char width.

>  
> -static bool
> -valid_bL_string_p (const char** core, const char* bL_string)
> + static bool
> +valid_bL_core_p (unsigned int *core, unsigned int bL_core)

Stray space before static.

>  {
> -  return strstr (bL_string, core[0]) != NULL
> -    && strstr (bL_string, core[1]) != NULL;
> +  return AARCH64_BIG_LITTLE (core[0], core[1]) == bL_core
> +         || AARCH64_BIG_LITTLE (core[1], core[0]) == bL_core;
> +}
> +
> +/* Returns the integer that is after ':' for the field. */
> +static unsigned parse_field (const char *field)

parse_field should be on a new line, FIELD should be capitalised in the
explanatory comment.

OK with the appropriate changes to rectify these points.

Thanks,
James

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

* Re: [PATCH 2/5] [AARCH64] Change IMP and PART over to integers from strings.
  2016-10-21 13:59         ` James Greenhalgh
@ 2016-10-21 15:57           ` Richard Earnshaw (lists)
  2016-10-21 16:28             ` James Greenhalgh
  0 siblings, 1 reply; 19+ messages in thread
From: Richard Earnshaw (lists) @ 2016-10-21 15:57 UTC (permalink / raw)
  To: James Greenhalgh, Andrew Pinski; +Cc: Andrew Pinski, GCC Patches, nd

On 21/10/16 14:59, James Greenhalgh wrote:
> On Sat, Oct 15, 2016 at 07:38:40PM -0700, Andrew Pinski wrote:
>> On Wed, Nov 25, 2015 at 11:59 AM, Andrew Pinski <pinskia@gmail.com> wrote:
>> Here is finally an updated (fixed) patch (I did not implement the two
>> implementer big.LITTLE support yet, that will be for a different patch
>> since I also fixed the part no not being unique as a separate patch.
>> Once I get a new enough kernel, I will also look into doing the
>> /sys/cpu/* style detection first.
>>
>> OK? Bootstrapped and tested on aarch64-linux-gnu with no regressions
>> (and tested hacking the location of the read in file to see if it
>> works with big.LITTLE and other formats of /proc/cpuinfo).
> 
> I'm OK with this in principle, but it needs some polish for pedantic
> style comments...
> 
>> * config/aarch64/aarch64-cores.def: Rewrite so IMP and PART are
>> integer constants.
>> * config/aarch64/driver-aarch64.c (struct aarch64_core_data): Change
>> implementer_id to unsigned char.
>> Change part_no to unsigned int.
>> (AARCH64_BIG_LITTLE): New define.
>> (INVALID_IMP): New define.
>> (INVALID_CORE): New define.
>> (cpu_data): Change the last element's implementer_id and part_no to integers.
>> (valid_bL_string_p): Rewrite to ..
>> (valid_bL_core_p): this for integers instead of strings.
>> (parse_field): New function.
>> (contains_string_p): Rewrite to ...
>> (contains_core_p): this for integers and only for the part_no.
>> (host_detect_local_cpu): Rewrite handling of implementation and part
>> num to be integers;
>> simplifying the code.
> 
>> Index: config/aarch64/aarch64-cores.def
>> ===================================================================
>> --- config/aarch64/aarch64-cores.def	(revision 241200)
>> +++ config/aarch64/aarch64-cores.def	(working copy)
>> @@ -32,43 +32,46 @@
>>     FLAGS are the bitwise-or of the traits that apply to that core.
>>     This need not include flags implied by the architecture.
>>     COSTS is the name of the rtx_costs routine to use.
>> -   IMP is the implementer ID of the CPU vendor.  On a GNU/Linux system it can
>> -   be found in /proc/cpuinfo.
>> +   IMP is the implementer ID of the CPU vendor.  On a GNU/Linux system it
>> +   can be found in /proc/cpuinfo. A partial list of implementer IDs is
>> +   given in the ARM Architecture Reference Manual ARMv8, for
>> -   in /proc/cpuinfo.  For big.LITTLE systems this should have the form at of
>> -   "<big core part number>.<LITTLE core part number>".  */
>> +   in /proc/cpuinfo.  For big.LITTLE systems this should use the macro AARCH64_BIG_LITTLE
>> +   where the big part number comes as the first arugment to the macro and little is the
>> +   second.  */
> 
> Needs rewrapped for 80 char width.
> 

I don't think it's a good idea to line wrap the def files, some of them
are processed with AWK during configure and having a complete entry per
line avoids potential matching problems.

R.

>>  
>> -static bool
>> -valid_bL_string_p (const char** core, const char* bL_string)
>> + static bool
>> +valid_bL_core_p (unsigned int *core, unsigned int bL_core)
> 
> Stray space before static.
> 
>>  {
>> -  return strstr (bL_string, core[0]) != NULL
>> -    && strstr (bL_string, core[1]) != NULL;
>> +  return AARCH64_BIG_LITTLE (core[0], core[1]) == bL_core
>> +         || AARCH64_BIG_LITTLE (core[1], core[0]) == bL_core;
>> +}
>> +
>> +/* Returns the integer that is after ':' for the field. */
>> +static unsigned parse_field (const char *field)
> 
> parse_field should be on a new line, FIELD should be capitalised in the
> explanatory comment.
> 
> OK with the appropriate changes to rectify these points.
> 
> Thanks,
> James
> 

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

* Re: [PATCH 2/5] [AARCH64] Change IMP and PART over to integers from strings.
  2016-10-21 15:57           ` Richard Earnshaw (lists)
@ 2016-10-21 16:28             ` James Greenhalgh
  2016-10-22  0:48               ` Andrew Pinski
  0 siblings, 1 reply; 19+ messages in thread
From: James Greenhalgh @ 2016-10-21 16:28 UTC (permalink / raw)
  To: Richard Earnshaw (lists); +Cc: Andrew Pinski, Andrew Pinski, GCC Patches, nd

On Fri, Oct 21, 2016 at 04:57:22PM +0100, Richard Earnshaw (lists) wrote:
> On 21/10/16 14:59, James Greenhalgh wrote:
> > On Sat, Oct 15, 2016 at 07:38:40PM -0700, Andrew Pinski wrote:
> >> On Wed, Nov 25, 2015 at 11:59 AM, Andrew Pinski <pinskia@gmail.com> wrote:
> >> Here is finally an updated (fixed) patch (I did not implement the two
> >> implementer big.LITTLE support yet, that will be for a different patch
> >> since I also fixed the part no not being unique as a separate patch.
> >> Once I get a new enough kernel, I will also look into doing the
> >> /sys/cpu/* style detection first.
> >>
> >> OK? Bootstrapped and tested on aarch64-linux-gnu with no regressions
> >> (and tested hacking the location of the read in file to see if it
> >> works with big.LITTLE and other formats of /proc/cpuinfo).
> > 
> > I'm OK with this in principle, but it needs some polish for pedantic
> > style comments...
> > 
> >> * config/aarch64/aarch64-cores.def: Rewrite so IMP and PART are
> >> integer constants.
> >> * config/aarch64/driver-aarch64.c (struct aarch64_core_data): Change
> >> implementer_id to unsigned char.
> >> Change part_no to unsigned int.
> >> (AARCH64_BIG_LITTLE): New define.
> >> (INVALID_IMP): New define.
> >> (INVALID_CORE): New define.
> >> (cpu_data): Change the last element's implementer_id and part_no to integers.
> >> (valid_bL_string_p): Rewrite to ..
> >> (valid_bL_core_p): this for integers instead of strings.
> >> (parse_field): New function.
> >> (contains_string_p): Rewrite to ...
> >> (contains_core_p): this for integers and only for the part_no.
> >> (host_detect_local_cpu): Rewrite handling of implementation and part
> >> num to be integers;
> >> simplifying the code.
> > 
> >> Index: config/aarch64/aarch64-cores.def
> >> ===================================================================
> >> --- config/aarch64/aarch64-cores.def	(revision 241200)
> >> +++ config/aarch64/aarch64-cores.def	(working copy)
> >> @@ -32,43 +32,46 @@
> >>     FLAGS are the bitwise-or of the traits that apply to that core.
> >>     This need not include flags implied by the architecture.
> >>     COSTS is the name of the rtx_costs routine to use.
> >> -   IMP is the implementer ID of the CPU vendor.  On a GNU/Linux system it can
> >> -   be found in /proc/cpuinfo.
> >> +   IMP is the implementer ID of the CPU vendor.  On a GNU/Linux system it
> >> +   can be found in /proc/cpuinfo. A partial list of implementer IDs is
> >> +   given in the ARM Architecture Reference Manual ARMv8, for
> >> -   in /proc/cpuinfo.  For big.LITTLE systems this should have the form at of
> >> -   "<big core part number>.<LITTLE core part number>".  */
> >> +   in /proc/cpuinfo.  For big.LITTLE systems this should use the macro AARCH64_BIG_LITTLE
> >> +   where the big part number comes as the first arugment to the macro and little is the
> >> +   second.  */
> > 
> > Needs rewrapped for 80 char width.
> > 
> 
> I don't think it's a good idea to line wrap the def files, some of them
> are processed with AWK during configure and having a complete entry per
> line avoids potential matching problems.

Agreed (and essential) for the entries themselves. This is just a comment
that hangs over the end and should be fixed.

While I'm here...

> >> +   where the big part number comes as the first arugment to the macro and little is the

s/arugment/argument.

Cheers,
James

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

* Re: [PATCH 2/5] [AARCH64] Change IMP and PART over to integers from strings.
  2016-10-21 16:28             ` James Greenhalgh
@ 2016-10-22  0:48               ` Andrew Pinski
  0 siblings, 0 replies; 19+ messages in thread
From: Andrew Pinski @ 2016-10-22  0:48 UTC (permalink / raw)
  To: James Greenhalgh; +Cc: Richard Earnshaw (lists), Andrew Pinski, GCC Patches, nd

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

On Fri, Oct 21, 2016 at 9:28 AM, James Greenhalgh
<james.greenhalgh@arm.com> wrote:
> On Fri, Oct 21, 2016 at 04:57:22PM +0100, Richard Earnshaw (lists) wrote:
>> On 21/10/16 14:59, James Greenhalgh wrote:
>> > On Sat, Oct 15, 2016 at 07:38:40PM -0700, Andrew Pinski wrote:
>> >> On Wed, Nov 25, 2015 at 11:59 AM, Andrew Pinski <pinskia@gmail.com> wrote:
>> >> Here is finally an updated (fixed) patch (I did not implement the two
>> >> implementer big.LITTLE support yet, that will be for a different patch
>> >> since I also fixed the part no not being unique as a separate patch.
>> >> Once I get a new enough kernel, I will also look into doing the
>> >> /sys/cpu/* style detection first.
>> >>
>> >> OK? Bootstrapped and tested on aarch64-linux-gnu with no regressions
>> >> (and tested hacking the location of the read in file to see if it
>> >> works with big.LITTLE and other formats of /proc/cpuinfo).
>> >
>> > I'm OK with this in principle, but it needs some polish for pedantic
>> > style comments...
>> >
>> >> * config/aarch64/aarch64-cores.def: Rewrite so IMP and PART are
>> >> integer constants.
>> >> * config/aarch64/driver-aarch64.c (struct aarch64_core_data): Change
>> >> implementer_id to unsigned char.
>> >> Change part_no to unsigned int.
>> >> (AARCH64_BIG_LITTLE): New define.
>> >> (INVALID_IMP): New define.
>> >> (INVALID_CORE): New define.
>> >> (cpu_data): Change the last element's implementer_id and part_no to integers.
>> >> (valid_bL_string_p): Rewrite to ..
>> >> (valid_bL_core_p): this for integers instead of strings.
>> >> (parse_field): New function.
>> >> (contains_string_p): Rewrite to ...
>> >> (contains_core_p): this for integers and only for the part_no.
>> >> (host_detect_local_cpu): Rewrite handling of implementation and part
>> >> num to be integers;
>> >> simplifying the code.
>> >
>> >> Index: config/aarch64/aarch64-cores.def
>> >> ===================================================================
>> >> --- config/aarch64/aarch64-cores.def       (revision 241200)
>> >> +++ config/aarch64/aarch64-cores.def       (working copy)
>> >> @@ -32,43 +32,46 @@
>> >>     FLAGS are the bitwise-or of the traits that apply to that core.
>> >>     This need not include flags implied by the architecture.
>> >>     COSTS is the name of the rtx_costs routine to use.
>> >> -   IMP is the implementer ID of the CPU vendor.  On a GNU/Linux system it can
>> >> -   be found in /proc/cpuinfo.
>> >> +   IMP is the implementer ID of the CPU vendor.  On a GNU/Linux system it
>> >> +   can be found in /proc/cpuinfo. A partial list of implementer IDs is
>> >> +   given in the ARM Architecture Reference Manual ARMv8, for
>> >> -   in /proc/cpuinfo.  For big.LITTLE systems this should have the form at of
>> >> -   "<big core part number>.<LITTLE core part number>".  */
>> >> +   in /proc/cpuinfo.  For big.LITTLE systems this should use the macro AARCH64_BIG_LITTLE
>> >> +   where the big part number comes as the first arugment to the macro and little is the
>> >> +   second.  */
>> >
>> > Needs rewrapped for 80 char width.
>> >
>>
>> I don't think it's a good idea to line wrap the def files, some of them
>> are processed with AWK during configure and having a complete entry per
>> line avoids potential matching problems.
>
> Agreed (and essential) for the entries themselves. This is just a comment
> that hangs over the end and should be fixed.

Yes I agree too.  I think with the entries over 80 char width made me
miss the comment being over 80.  Anyways I fixed it.

>
> While I'm here...
>
>> >> +   where the big part number comes as the first arugment to the macro and little is the
>
> s/arugment/argument.

Fixed.

I also changed the comment for parse_field to include why it would
return -1 and that it parses hex rather than any
base (I noticed this due to request to fix the style of the function itself :)).

Attached is what I am applying after another bootstrap/test on
aarch64-linux-gnu.

Thanks,
Andrew

* config/aarch64/aarch64-cores.def: Rewrite so IMP and PART are
integer constants.
* config/aarch64/driver-aarch64.c (struct aarch64_core_data): Change
implementer_id to unsigned char.
Change part_no to unsigned int.
(AARCH64_BIG_LITTLE): New define.
(INVALID_IMP): New define.
(INVALID_CORE): New define.
(cpu_data): Change the last element's implementer_id and part_no to integers.
(valid_bL_string_p): Rewrite to ..
(valid_bL_core_p): this for integers instead of strings.
(parse_field): New function.
(contains_string_p): Rewrite to ...
(contains_core_p): this for integers and only for the part_no.
(host_detect_local_cpu): Rewrite handling of implementation and part
num to be integers;
simplifying the code.

>
> Cheers,
> James
>

[-- Attachment #2: changestringtoint.diff.txt --]
[-- Type: text/plain, Size: 11057 bytes --]

Index: config/aarch64/aarch64-cores.def
===================================================================
--- config/aarch64/aarch64-cores.def	(revision 241432)
+++ config/aarch64/aarch64-cores.def	(working copy)
@@ -32,43 +32,46 @@
    FLAGS are the bitwise-or of the traits that apply to that core.
    This need not include flags implied by the architecture.
    COSTS is the name of the rtx_costs routine to use.
-   IMP is the implementer ID of the CPU vendor.  On a GNU/Linux system it can
-   be found in /proc/cpuinfo.
-   PART is the part number of the CPU.  On a GNU/Linux system it can be found
-   in /proc/cpuinfo.  For big.LITTLE systems this should have the form at of
-   "<big core part number>.<LITTLE core part number>".  */
+   IMP is the implementer ID of the CPU vendor.  On a GNU/Linux system it
+   can be found in /proc/cpuinfo. A partial list of implementer IDs is
+   given in the ARM Architecture Reference Manual ARMv8, for
+   ARMv8-A architecture profile.
+   PART is the part number of the CPU.  On a GNU/Linux system it can be
+   found in /proc/cpuinfo.  For big.LITTLE systems this should use the
+   macro AARCH64_BIG_LITTLE where the big part number comes as the first
+   argument to the macro and little is the second.  */
 
 /* V8 Architecture Processors.  */
 
 /* ARM ('A') cores. */
-AARCH64_CORE("cortex-a35",  cortexa35, cortexa53, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC, cortexa35, "0x41", "0xd04")
-AARCH64_CORE("cortex-a53",  cortexa53, cortexa53, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC, cortexa53, "0x41", "0xd03")
-AARCH64_CORE("cortex-a57",  cortexa57, cortexa57, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC, cortexa57, "0x41", "0xd07")
-AARCH64_CORE("cortex-a72",  cortexa72, cortexa57, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC, cortexa72, "0x41", "0xd08")
-AARCH64_CORE("cortex-a73",  cortexa73, cortexa57, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC, cortexa73, "0x41", "0xd09")
+AARCH64_CORE("cortex-a35",  cortexa35, cortexa53, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC, cortexa35, 0x41, 0xd04)
+AARCH64_CORE("cortex-a53",  cortexa53, cortexa53, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC, cortexa53, 0x41, 0xd03)
+AARCH64_CORE("cortex-a57",  cortexa57, cortexa57, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC, cortexa57, 0x41, 0xd07)
+AARCH64_CORE("cortex-a72",  cortexa72, cortexa57, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC, cortexa72, 0x41, 0xd08)
+AARCH64_CORE("cortex-a73",  cortexa73, cortexa57, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC, cortexa73, 0x41, 0xd09)
 
 /* Samsung ('S') cores. */
-AARCH64_CORE("exynos-m1",   exynosm1,  exynosm1,  8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC | AARCH64_FL_CRYPTO, exynosm1,  "0x53", "0x001")
+AARCH64_CORE("exynos-m1",   exynosm1,  exynosm1,  8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC | AARCH64_FL_CRYPTO, exynosm1,  0x53, 0x001)
 
 /* Qualcomm ('Q') cores. */
-AARCH64_CORE("qdf24xx",     qdf24xx,   cortexa57, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC | AARCH64_FL_CRYPTO, qdf24xx,   "0x51", "0x800")
+AARCH64_CORE("qdf24xx",     qdf24xx,   cortexa57, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC | AARCH64_FL_CRYPTO, qdf24xx,   0x51, 0x800)
 
 /* Cavium ('C') cores. */
-AARCH64_CORE("thunderx",    thunderx,  thunderx,  8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC | AARCH64_FL_CRYPTO, thunderx,  "0x43", "0x0a1")
+AARCH64_CORE("thunderx",    thunderx,  thunderx,  8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC | AARCH64_FL_CRYPTO, thunderx,  0x43, 0x0a1)
 
 /* APM ('P') cores. */
-AARCH64_CORE("xgene1",      xgene1,    xgene1,    8A,  AARCH64_FL_FOR_ARCH8, xgene1, "0x50", "0x000")
+AARCH64_CORE("xgene1",      xgene1,    xgene1,    8A,  AARCH64_FL_FOR_ARCH8, xgene1, 0x50, 0x000)
 
 /* V8.1 Architecture Processors.  */
 
 /* Broadcom ('B') cores. */
-AARCH64_CORE("vulcan",  vulcan, cortexa57, 8_1A,  AARCH64_FL_FOR_ARCH8_1 | AARCH64_FL_CRYPTO, vulcan, "0x42", "0x516")
+AARCH64_CORE("vulcan",  vulcan, cortexa57, 8_1A,  AARCH64_FL_FOR_ARCH8_1 | AARCH64_FL_CRYPTO, vulcan, 0x42, 0x516)
 
 /* V8 big.LITTLE implementations.  */
 
-AARCH64_CORE("cortex-a57.cortex-a53",  cortexa57cortexa53, cortexa53, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC, cortexa57, "0x41", "0xd07.0xd03")
-AARCH64_CORE("cortex-a72.cortex-a53",  cortexa72cortexa53, cortexa53, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC, cortexa72, "0x41", "0xd08.0xd03")
-AARCH64_CORE("cortex-a73.cortex-a35",  cortexa73cortexa35, cortexa53, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC, cortexa73, "0x41", "0xd09.0xd04")
-AARCH64_CORE("cortex-a73.cortex-a53",  cortexa73cortexa53, cortexa53, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC, cortexa73, "0x41", "0xd09.0xd03")
+AARCH64_CORE("cortex-a57.cortex-a53",  cortexa57cortexa53, cortexa53, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC, cortexa57, 0x41, AARCH64_BIG_LITTLE (0xd07, 0xd03))
+AARCH64_CORE("cortex-a72.cortex-a53",  cortexa72cortexa53, cortexa53, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC, cortexa72, 0x41, AARCH64_BIG_LITTLE (0xd08, 0xd03))
+AARCH64_CORE("cortex-a73.cortex-a35",  cortexa73cortexa35, cortexa53, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC, cortexa73, 0x41, AARCH64_BIG_LITTLE (0xd09, 0xd04))
+AARCH64_CORE("cortex-a73.cortex-a53",  cortexa73cortexa53, cortexa53, 8A,  AARCH64_FL_FOR_ARCH8 | AARCH64_FL_CRC, cortexa73, 0x41, AARCH64_BIG_LITTLE (0xd09, 0xd03))
 
 #undef AARCH64_CORE
Index: config/aarch64/driver-aarch64.c
===================================================================
--- config/aarch64/driver-aarch64.c	(revision 241432)
+++ config/aarch64/driver-aarch64.c	(working copy)
@@ -46,18 +46,23 @@ struct aarch64_core_data
 {
   const char* name;
   const char* arch;
-  const char* implementer_id;
-  const char* part_no;
+  unsigned char implementer_id; /* Exactly 8 bits */
+  unsigned int part_no; /* 12 bits + 12 bits */
   const unsigned long flags;
 };
 
+#define AARCH64_BIG_LITTLE(BIG, LITTLE) \
+  (((BIG)&0xFFFu) << 12 | ((LITTLE) & 0xFFFu))
+#define INVALID_IMP ((unsigned char) -1)
+#define INVALID_CORE ((unsigned)-1)
+
 #define AARCH64_CORE(CORE_NAME, CORE_IDENT, SCHED, ARCH, FLAGS, COSTS, IMP, PART) \
   { CORE_NAME, #ARCH, IMP, PART, FLAGS },
 
 static struct aarch64_core_data aarch64_cpu_data[] =
 {
 #include "aarch64-cores.def"
-  { NULL, NULL, NULL, NULL, 0 }
+  { NULL, NULL, INVALID_IMP, INVALID_CORE, 0 }
 };
 
 
@@ -95,32 +100,42 @@ get_arch_from_id (const char* id)
   return NULL;
 }
 
-/* Check wether the string CORE contains the same CPU part numbers
-   as BL_STRING.  For example CORE="{0xd03, 0xd07}" and BL_STRING="0xd07.0xd03"
-   should return true.  */
+/* Check wether the CORE array is the same as the big.LITTLE BL_CORE.
+   For an example CORE={0xd08, 0xd03} and
+   BL_CORE=AARCH64_BIG_LITTLE (0xd08, 0xd03) will return true.  */
 
 static bool
-valid_bL_string_p (const char** core, const char* bL_string)
+valid_bL_core_p (unsigned int *core, unsigned int bL_core)
+{
+  return AARCH64_BIG_LITTLE (core[0], core[1]) == bL_core
+         || AARCH64_BIG_LITTLE (core[1], core[0]) == bL_core;
+}
+
+/* Returns the hex integer that is after ':' for the FIELD.
+   Returns -1 is returned if there was problem parsing the integer. */
+static unsigned
+parse_field (const char *field)
 {
-  return strstr (bL_string, core[0]) != NULL
-    && strstr (bL_string, core[1]) != NULL;
+  const char *rest = strchr (field, ':');
+  char *after;
+  unsigned fint = strtol (rest + 1, &after, 16);
+  if (after == rest + 1)
+    return -1;
+  return fint;
 }
 
-/*  Return true iff ARR contains STR in one of its two elements.  */
+/*  Return true iff ARR contains CORE, in either of the two elements. */
 
 static bool
-contains_string_p (const char** arr, const char* str)
+contains_core_p (unsigned *arr, unsigned core)
 {
-  bool res = false;
-
-  if (arr[0] != NULL)
+  if (arr[0] != INVALID_CORE)
     {
-      res = strstr (arr[0], str) != NULL;
-      if (res)
-        return res;
+      if (arr[0] == core)
+        return true;
 
-      if (arr[1] != NULL)
-        return strstr (arr[1], str) != NULL;
+      if (arr[1] != INVALID_CORE)
+        return arr[1] == core;
     }
 
   return false;
@@ -155,10 +170,9 @@ host_detect_local_cpu (int argc, const c
   bool cpu = false;
   unsigned int i = 0;
   unsigned int core_idx = 0;
-  const char* imps[2] = { NULL, NULL };
-  const char* cores[2] = { NULL, NULL };
+  unsigned char imp = INVALID_IMP;
+  unsigned int cores[2] = { INVALID_CORE, INVALID_CORE };
   unsigned int n_cores = 0;
-  unsigned int n_imps = 0;
   bool processed_exts = false;
   const char *ext_string = "";
   unsigned long extension_flags = 0;
@@ -191,31 +205,28 @@ host_detect_local_cpu (int argc, const c
     {
       if (strstr (buf, "implementer") != NULL)
 	{
-	  for (i = 0; aarch64_cpu_data[i].name != NULL; i++)
-	    if (strstr (buf, aarch64_cpu_data[i].implementer_id) != NULL
-		&& !contains_string_p (imps,
-				       aarch64_cpu_data[i].implementer_id))
-	      {
-		if (n_imps == 2)
-		  goto not_found;
-
-		imps[n_imps++] = aarch64_cpu_data[i].implementer_id;
-
-		break;
-	      }
-	  continue;
+	  unsigned cimp = parse_field (buf);
+	  if (cimp == INVALID_IMP)
+	    goto not_found;
+
+	  if (imp == INVALID_IMP)
+	    imp = cimp;
+	  /* FIXME: BIG.little implementers are always equal. */
+	  else if (imp != cimp)
+	    goto not_found;
 	}
 
       if (strstr (buf, "part") != NULL)
 	{
+	  unsigned ccore = parse_field (buf);
 	  for (i = 0; aarch64_cpu_data[i].name != NULL; i++)
-	    if (strstr (buf, aarch64_cpu_data[i].part_no) != NULL
-		&& !contains_string_p (cores, aarch64_cpu_data[i].part_no))
+	    if (ccore == aarch64_cpu_data[i].part_no
+		&& !contains_core_p (cores, ccore))
 	      {
 		if (n_cores == 2)
 		  goto not_found;
 
-		cores[n_cores++] = aarch64_cpu_data[i].part_no;
+		cores[n_cores++] = ccore;
 		core_idx = i;
 		arch_id = aarch64_cpu_data[i].arch;
 		break;
@@ -262,7 +273,7 @@ host_detect_local_cpu (int argc, const c
   f = NULL;
 
   /* Weird cpuinfo format that we don't know how to handle.  */
-  if (n_cores == 0 || n_cores > 2 || n_imps != 1)
+  if (n_cores == 0 || n_cores > 2 || imp == INVALID_IMP)
     goto not_found;
 
   if (arch && !arch_id)
@@ -284,11 +295,8 @@ host_detect_local_cpu (int argc, const c
     {
       for (i = 0; aarch64_cpu_data[i].name != NULL; i++)
 	{
-	  if (strchr (aarch64_cpu_data[i].part_no, '.') != NULL
-	      && strncmp (aarch64_cpu_data[i].implementer_id,
-			  imps[0],
-			  strlen (imps[0]) - 1) == 0
-	      && valid_bL_string_p (cores, aarch64_cpu_data[i].part_no))
+	  if (aarch64_cpu_data[i].implementer_id == imp
+	      && valid_bL_core_p (cores, aarch64_cpu_data[i].part_no))
 	    {
 	      res = concat ("-m",
 			    cpu ? "cpu" : "tune", "=",
@@ -304,8 +312,7 @@ host_detect_local_cpu (int argc, const c
   /* The simple, non-big.LITTLE case.  */
   else
     {
-      if (strncmp (aarch64_cpu_data[core_idx].implementer_id, imps[0],
-		   strlen (imps[0]) - 1) != 0)
+      if (aarch64_cpu_data[core_idx].implementer_id != imp)
 	goto not_found;
 
       res = concat ("-m", cpu ? "cpu" : "tune", "=",

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

end of thread, other threads:[~2016-10-22  0:48 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-11-17 22:10 [PATCH 0/5] Add support -mcpu=thunderxt88pass1 and needed changes to support that Andrew Pinski
2015-11-17 22:10 ` [PATCH 2/5] [AARCH64] Change IMP and PART over to integers from strings Andrew Pinski
2015-11-20 11:07   ` Kyrill Tkachov
2015-11-25 10:35   ` James Greenhalgh
2015-11-25 20:12     ` Andrew Pinski
2016-10-16  2:38       ` Andrew Pinski
2016-10-21 13:59         ` James Greenhalgh
2016-10-21 15:57           ` Richard Earnshaw (lists)
2016-10-21 16:28             ` James Greenhalgh
2016-10-22  0:48               ` Andrew Pinski
2015-11-17 22:10 ` [PATCH 4/5] {AARCH64] Add comment for the company's cores Andrew Pinski
2015-11-17 22:10 ` [PATCH 3/5] [AARCH64] Fix part num and implement indendent Andrew Pinski
2015-11-17 22:11 ` [PATCH 1/5] [AARCH64]: Move #undef into .def files Andrew Pinski
2015-11-18 12:29   ` Marcus Shawcroft
2016-10-07 21:36     ` Andrew Pinski
2015-11-17 22:11 ` [PATCH 5/5] [AARCH64] Add variant support to -m*=native and add thunderxt88pass1 Andrew Pinski
2015-11-17 22:55   ` Joseph Myers
2015-11-19 12:08 ` [PATCH 0/5] Add support -mcpu=thunderxt88pass1 and needed changes to support that Kyrill Tkachov
2015-11-20  0:04   ` Andrew Pinski

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).