This is the mail archive of the binutils@sourceware.org mailing list for the binutils project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH 0/2] [PUSHED/OBV] gas/arc: Add nps400 support to .cpu directive


* Claudiu Zissulescu <Claudiu.Zissulescu@synopsys.com> [2016-04-20 09:07:18 +0000]:

> Hi Andrew,
> 
> > +/* Maximum number of CPU aliases in the cpu_type table.  */
> > +#define MAX_NUMBER_OF_ALIASES 2
> > +
> >  /* A table of CPU names and opcode sets.  */
> >  static const struct cpu_type
> >  {
> >    const char *name;
> > +  const char *alises [MAX_NUMBER_OF_ALIASES + 1];
> 
> Instead of this list of aliases, I would list all the alternatives
> in cpu_types[]. On the expense of more used memory, we will get rid
> of the MAX_NUMBER _OF_ALIASES and the need to maintain it whenever
> we want to add a new cpu alias.  Also the support routines will look
> much cleaner, we will have a single scanning loop instead of
> two. Please take it as recommendation. Regardless the decision you
> take, the patch seems alright to me.

* Nick Clifton <nickc@redhat.com> [2016-04-20 10:25:13 +0100]:

> 
> > Instead of this list of aliases, I would list all the alternatives
> > in cpu_types[].
> 
> This would be a much better idea.

I disagree and think it's a much worse idea.  My concern was never
about memory usage, my concern is data duplication, how long until we
have a bug where one alias is configured slightly differently to
another alias.  Much better, I still think, to have one list of names
and one set of configuration, and link those together.

The concern about the support routines being "cleaner" is valid, but
then, that's why they are support routines, to hide the grubby
detail.  Still, in deference I have merged the name field and the
aliases into a single list, this makes the support routines slightly
easier on the eyes, but we still fundamentally have two loops one over
table entries and one over aliases within the entry.

I actually preferred the code as it was before really, the first
"name" is special, which is why I originally left it as a separate
field; it's the only name accepted in 'arc_select_cpu' (so -mcpu=TYPE
on the command line does not like aliases) while the handling of .cpu
directive is the only place that aliases are accepted.  Though this
specialness is not something I would choose I originally left it in
just to avoid changing the behaviour.

Still, here's a revised patch with the name / aliases merged into a
single list for consideration, though if this patch can't be merged in
the current form then it's certainly not a blocker for me.

Thanks,
Andrew

---

arc/gas: Unify -mcpu option and .cpu directive handling

This change makes use of the single cpu_types table for handling the
.cpu directive and the -mcpu command line option.  At the same time the
help text for -mcpu is extended to include the list of possible cpu
types, generated from the table.

gas/ChangeLog:

	* config/tc-arc.c (MAX_CPU_NAMES): Define.
	(struct cpu_type): Change names field to a list.
	(cpu_types): Add all aliases to the names list.
	(NUMBER_OF_CPU_TYPES): Define.
	(arc_select_cpu): Updates to reflect changes in cpu_types table.
	(arc_handle_cpu_option_string): New function.
	(arc_option): Use arc_handle_cpu_option_string to process the
	option.
	(arc_show_cpu_list): New function.
	(md_show_usage): Call arc_show_cpu_list.  Convert help text to
	lower case to match all other help text.  Remove use of tabs,
	inline with global help text.
---
 gas/ChangeLog       |  15 ++++++
 gas/config/tc-arc.c | 128 +++++++++++++++++++++++++++++++++-------------------
 2 files changed, 97 insertions(+), 46 deletions(-)

diff --git a/gas/config/tc-arc.c b/gas/config/tc-arc.c
index 169b05c..0d11209 100644
--- a/gas/config/tc-arc.c
+++ b/gas/config/tc-arc.c
@@ -408,10 +408,13 @@ static struct hash_control *arc_reg_hash;
 /* The hash table of aux register symbols.  */
 static struct hash_control *arc_aux_hash;
 
+/* Maximum number of CPU aliases in the cpu_type table.  */
+#define MAX_CPU_NAMES 3
+
 /* A table of CPU names and opcode sets.  */
 static const struct cpu_type
 {
-  const char *name;
+  const char *names [MAX_CPU_NAMES];
   unsigned flags;
   int mach;
   unsigned eflags;
@@ -419,19 +422,21 @@ static const struct cpu_type
 }
   cpu_types[] =
 {
-  { "arc600", ARC_OPCODE_ARC600,  bfd_mach_arc_arc600,
-    E_ARC_MACH_ARC600,  0x00},
-  { "arc700", ARC_OPCODE_ARC700,  bfd_mach_arc_arc700,
-    E_ARC_MACH_ARC700,  0x00},
-  { "nps400", ARC_OPCODE_ARC700 | ARC_OPCODE_NPS400, bfd_mach_arc_nps400,
+  { { "ARC600", "ARC601", "A6" },
+    ARC_OPCODE_ARC600,  bfd_mach_arc_arc600, E_ARC_MACH_ARC600,  0x00},
+  { { "ARC700", "A7" },
+    ARC_OPCODE_ARC700,  bfd_mach_arc_arc700, E_ARC_MACH_ARC700,  0x00},
+  { { "NPS400" },
+    ARC_OPCODE_ARC700 | ARC_OPCODE_NPS400, bfd_mach_arc_nps400,
     E_ARC_MACH_NPS400,  0x00},
-  { "arcem",  ARC_OPCODE_ARCv2EM, bfd_mach_arc_arcv2,
-    EF_ARC_CPU_ARCV2EM, ARC_CD},
-  { "archs",  ARC_OPCODE_ARCv2HS, bfd_mach_arc_arcv2,
-    EF_ARC_CPU_ARCV2HS, ARC_CD},
-  { 0, 0, 0, 0, 0 }
+  { { "ARCEM", "EM" },
+    ARC_OPCODE_ARCv2EM, bfd_mach_arc_arcv2, EF_ARC_CPU_ARCV2EM, ARC_CD},
+  { { "ARCHS", "HS" },
+    ARC_OPCODE_ARCv2HS, bfd_mach_arc_arcv2, EF_ARC_CPU_ARCV2HS, ARC_CD},
 };
 
+#define NUMBER_OF_CPU_TYPES (sizeof (cpu_types) / sizeof (cpu_types[0]))
+
 /* Used by the arc_reloc_op table.  Order is important.  */
 #define O_gotoff  O_md1     /* @gotoff relocation.  */
 #define O_gotpc   O_md2     /* @gotpc relocation.  */
@@ -745,14 +750,14 @@ static void
 arc_select_cpu (const char *arg)
 {
   int cpu_flags = 0;
-  int i;
+  unsigned int i;
 
-  for (i = 0; cpu_types[i].name; ++i)
+  for (i = 0; i < NUMBER_OF_CPU_TYPES; ++i)
     {
-      if (!strcasecmp (cpu_types[i].name, arg))
+      if (!strcasecmp (cpu_types[i].names[0], arg))
         {
           arc_target = cpu_types[i].flags;
-          arc_target_name = cpu_types[i].name;
+          arc_target_name = cpu_types[i].names[0];
           arc_features = cpu_types[i].features;
           arc_mach_type = cpu_types[i].mach;
           cpu_flags = cpu_types[i].eflags;
@@ -760,8 +765,9 @@ arc_select_cpu (const char *arg)
         }
     }
 
-  if (!cpu_types[i].name)
+  if (i == NUMBER_OF_CPU_TYPES)
     as_fatal (_("unknown architecture: %s\n"), arg);
+
   gas_assert (cpu_flags != 0);
   arc_eflag = (arc_eflag & ~EF_ARC_MACH_MSK) | cpu_flags;
 }
@@ -861,6 +867,32 @@ arc_lcomm (int ignore)
     symbol_get_bfdsym (symbolP)->flags |= BSF_OBJECT;
 }
 
+/* Take the string passed to the .cpu directive and configure the assembler
+   as though the -mcpu option was used.  Raises an error if the string is
+   not a valid cpu name.  */
+
+static void
+arc_handle_cpu_option_string (const char *cpu)
+{
+  unsigned int i;
+
+  for (i = 0; i < NUMBER_OF_CPU_TYPES; ++i)
+    {
+      int j;
+
+      for (j = 0; j < MAX_CPU_NAMES && cpu_types[i].names[j] != NULL; ++j)
+        {
+          if (!strcmp (cpu_types[i].names[j], cpu))
+            {
+              md_parse_option (OPTION_MCPU, cpu_types[i].names[0]);
+              return;
+            }
+        }
+    }
+
+  as_fatal (_("could not find the architecture"));
+}
+
 /* Select the cpu we're assembling for.  */
 
 static void
@@ -878,31 +910,7 @@ arc_option (int ignore ATTRIBUTE_UNUSED)
 
   if (!mach_type_specified_p)
     {
-      if ((!strcmp ("ARC600", cpu))
-	  || (!strcmp ("ARC601", cpu))
-	  || (!strcmp ("A6", cpu)))
-	{
-	  md_parse_option (OPTION_MCPU, "arc600");
-	}
-      else if ((!strcmp ("ARC700", cpu))
-	       || (!strcmp ("A7", cpu)))
-	{
-	  md_parse_option (OPTION_MCPU, "arc700");
-	}
-      else if (!strcmp ("EM", cpu))
-	{
-	  md_parse_option (OPTION_MCPU, "arcem");
-	}
-      else if (!strcmp ("HS", cpu))
-	{
-	  md_parse_option (OPTION_MCPU, "archs");
-	}
-      else if (!strcmp ("NPS400", cpu))
-	{
-	  md_parse_option (OPTION_MCPU, "nps400");
-	}
-      else
-	as_fatal (_("could not find the architecture"));
+      arc_handle_cpu_option_string (cpu);
 
       if (!bfd_set_arch_mach (stdoutput, bfd_arch_arc, mach))
 	as_fatal (_("could not set architecture and machine"));
@@ -3238,21 +3246,49 @@ md_parse_option (int c, const char *arg ATTRIBUTE_UNUSED)
   return 1;
 }
 
+/* Display the list of cpu names for use in the help text.  */
+static void
+arc_show_cpu_list (FILE *stream)
+{
+  unsigned int i, offset;
+  static const char *spaces = "                          ";
+
+  fprintf (stream, spaces);
+  offset = strlen (spaces);
+  for (i = 0; i < NUMBER_OF_CPU_TYPES; ++i)
+    {
+      bfd_boolean last = (i + 1 == NUMBER_OF_CPU_TYPES);
+
+      /* If displaying the new cpu name string, and the ', ' (for all but
+         the last one) will take us past a target width of 80 characters,
+         then it's time for a new line.  */
+      if (offset + strlen (cpu_types[i].names[0]) + (last ? 0 : 2) > 80)
+        {
+          fprintf (stream, "\n%s", spaces);
+          offset = strlen (spaces);
+        }
+
+      fprintf (stream, "%s%s", cpu_types[i].names[0], (last ? "\n" : ", "));
+      offset += strlen (cpu_types [i].names[0]) + (last ? 0 : 2);
+    }
+}
+
 void
 md_show_usage (FILE *stream)
 {
   fprintf (stream, _("ARC-specific assembler options:\n"));
 
-  fprintf (stream, "  -mcpu=<cpu name>\t  assemble for CPU <cpu name>\n");
-  fprintf (stream,
-	   "  -mcode-density\t  enable code density option for ARC EM\n");
-
+  fprintf (stream, "\
+  -mcpu=<cpu name>        assemble for CPU <cpu name>, one of:\n");
+  arc_show_cpu_list (stream);
+  fprintf (stream, "\
+  -mcode-density          enable code density option for ARC EM\n");
   fprintf (stream, _("\
   -EB                     assemble code for a big-endian cpu\n"));
   fprintf (stream, _("\
   -EL                     assemble code for a little-endian cpu\n"));
   fprintf (stream, _("\
-  -mrelax                  Enable relaxation\n"));
+  -mrelax                 enable relaxation\n"));
 
 }
 
-- 
2.5.1


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]