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]

[PATCH] gas: Revive MIPS itbl


Hello,

 It looks like we have this itbl feature, lost and forgotten by everybody, 
apparently meant to be generic, but in its current shape usable for MIPS 
only if at all.  Actually the latter is more of the case right now -- a 
recent change to itbl-ops.c which renamed NUMBER_OF_PROCESSORS to 
ITBL_NUMBER_OF_PROCESSORS almost killed the feature as with the default of 
1 coprocessor only any attempt to use "--itbl" with a table accessing, 
say, coprocessor 3 yields a segfault (for MIPS coprocessor 0 typically is 
not the target of instruction set extensions).

 While sorting it out, I fixed a couple of issues:

1. The COPx opcode itself was not marked as a part of the opcode mask -- 
   this would guarantee an "internal: bad mips opcode (bits [...] 
   undefined): [...]" error,

2. The ISA membership was left uninitialised, guaranteeing an "opcode not 
   supported on this processor: [...]" error,

3. I moved a chunk hidden within the LOSING_COMPILER conditional to where 
   it should be, that is the last default for the macro/macro2() switch 
   selection; still disabled as intended,

4. Fixed typos in related comments, etc.

With these changes in place the code works for a case close to one 
outlined within comments.  That is this sequence now works as expected:

$ cat pig.tbl
p3 insn pig 0x1:24-21 creg:20-16 immed:15-0
p3 creg d2 0x2
$ cat pig.s
	pig	d2, 0x100
	pig	$2, 0x100
$ mipsel-linux-as --itbl pig.tbl -o pig.o pig.s
$ mipsel-linux-objdump -d pig.o

pig.o:     file format elf32-tradlittlemips

Disassembly of section .text:

00000000 <.text>:
   0:	4e220100 	c3	0x220100
   4:	4e220100 	c3	0x220100
	...

(`objdump' has yet to be taught about itbl).

 The issue #3 above is an interesting one.  As you may have noticed I have 
substituted "dreg" for "creg" in the "pig" example.  This is because the 
itbl instructions are currently treated as hardwired and md_begin() forces 
a check also seen with #1 above, which makes specific assumptions as to 
where a register field is located within the instruction word, based on 
the operand codes.  The check does not work well for itbl in general as 
the extension was designed to allow arbitrary sizes and positions of 
registers and other fields within.

 Treating itbl instructions as macros, as done with some disabled 
fragments of code could potentially resolve the issue, but then we would 
get rid of all the relocation generation that we get "for free" with 
hardwired instructions.  So I decided to leave the original conditions as 
they are.

2007-10-08  Maciej W. Rozycki  <macro@linux-mips.org>

	* config/itbl-mips.h (NUMBER_OF_PROCESSORS): Rename to...
	(ITBL_NUMBER_OF_PROCESSORS): ... this.
	(MAX_BITPOS): Rename to...
	(ITBL_MAX_BITPOS): ... this
	(MIPS_MASK_COP0): New macro.
	(MIPS_OPCODE_MASK): New macro.
	(MIPS_IS_COP_INSN): Correct opcode masking.
	(ITBL_OPCODE_MASK): New macro.
	* config/tc-mips.c (macro): Move the handling of itbl macros
	to...
	(macro2): ... here.  Keep disabled.
	* itbl-ops.c (build_mask): New function.
	(append_insns_as_macros): Correct recording opcode information
	in ITBL_OPCODE_STRUCT.
	(apply_range): Rename MAX_BITPOS to ITBL_MAX_BITPOS.
	* itbl-ops.h (ITBL_OPCODE_MASK): New macro default.

 What next is up to discussion, but for now I would like to apply this 
change.  No regressions seen on mips64el-linux-gnu.  Unfortunately the 
itbl feature itself is not quite covered -- there is a test case of some 
sort included, but never run.  No surprise years of accumulated breakage 
remained unnoticed.

 There is also the issue of the LOSING_COMPILER chunk, which with the 
patch below is almost safe to be enabled with "CFLAGS=-DLOSING_COMPILER" 
or suchlike.  I write "almost", because "-Werror" is not entirely happy 
with the variable definitions.  Which reaises the question as to whether 
we want to keep it at all.  Given the thing has been broken since the itbl 
feature was added, which is for some ten years, and nobody complained, I 
guess it should be considered safe to drop.  I'll cook a patch unless 
there is strong objection. ;-)

  Maciej

binutils-2.18-mips-itbl.patch
diff -up --recursive --new-file binutils-2.18.macro/gas/config/itbl-mips.h binutils-2.18/gas/config/itbl-mips.h
--- binutils-2.18.macro/gas/config/itbl-mips.h	2007-08-06 19:59:52.000000000 +0000
+++ binutils-2.18/gas/config/itbl-mips.h	2007-10-08 17:40:51.000000000 +0000
@@ -19,28 +19,34 @@
    Software Foundation, 51 Franklin Street - Fifth Floor, Boston, MA
    02110-1301, USA.  */
 
-/* Defines for Mips itbl cop support.  */
+/* Defines for MIPS itbl cop support.  */
 
 #include "opcode/mips.h"
 
-/* Values for processors will be from 0 to NUMBER_OF_PROCESSORS-1 */
-#define NUMBER_OF_PROCESSORS 4
-#define MAX_BITPOS 31
-
-/* Mips specifics */
-#define MIPS_OPCODE_COP0 (0x21)	/* COPz+CO, bits 31-25: 0100zz1 */
-#define MIPS_ENCODE_COP_NUM(z) ((MIPS_OPCODE_COP0|z<<1)<<25)
-#define MIPS_IS_COP_INSN(insn) ((MIPS_OPCODE_COP0&(insn>>25)) \
-	== MIPS_OPCODE_COP0)
-#define MIPS_DECODE_COP_NUM(insn) ((~MIPS_OPCODE_COP0&(insn>>25))>>1)
-#define MIPS_DECODE_COP_COFUN(insn) ((~MIPS_ENCODE_COP_NUM(3))&(insn))
-
-/* definitions required by generic code */
-#define ITBL_IS_INSN(insn) MIPS_IS_COP_INSN(insn)
-#define ITBL_DECODE_PNUM(insn) MIPS_DECODE_COP_NUM(insn)
-#define ITBL_ENCODE_PNUM(pnum) MIPS_ENCODE_COP_NUM(pnum)
-
-#define ITBL_OPCODE_STRUCT mips_opcode
-#define ITBL_OPCODES mips_opcodes
-#define ITBL_NUM_OPCODES NUMOPCODES
-#define ITBL_NUM_MACROS M_NUM_MACROS
+/* Values for processors will be from 0 to ITBL_NUMBER_OF_PROCESSORS-1.  */
+#define ITBL_NUMBER_OF_PROCESSORS	4
+#define ITBL_MAX_BITPOS			31
+
+/* MIPS specifics.  */
+					/* COPz+CO, bits 31-25: 0100zz1 */
+#define MIPS_OPCODE_COP0		0x21
+					/* COPz+CO, bits 31-25: 1111zz1 */
+#define MIPS_MASK_COP0			0x79
+#define MIPS_ENCODE_COP_NUM(z)		((MIPS_OPCODE_COP0 | (z) << 1) << 25)
+#define MIPS_OPCODE_MASK		((MIPS_MASK_COP0 | 3 << 1) << 25)
+#define MIPS_IS_COP_INSN(insn)		((MIPS_MASK_COP0 & ((insn) >> 25)) \
+					 == MIPS_OPCODE_COP0)
+#define MIPS_DECODE_COP_NUM(insn)	((~MIPS_OPCODE_COP0 & ((insn) >> 25)) \
+					 >> 1)
+#define MIPS_DECODE_COP_COFUN(insn)	((~MIPS_ENCODE_COP_NUM(3)) & (insn))
+
+/* Definitions required by generic code.  */
+#define ITBL_IS_INSN(insn)		MIPS_IS_COP_INSN(insn)
+#define ITBL_DECODE_PNUM(insn)		MIPS_DECODE_COP_NUM(insn)
+#define ITBL_ENCODE_PNUM(pnum)		MIPS_ENCODE_COP_NUM(pnum)
+#define ITBL_OPCODE_MASK		MIPS_OPCODE_MASK
+
+#define ITBL_OPCODE_STRUCT		mips_opcode
+#define ITBL_OPCODES			mips_opcodes
+#define ITBL_NUM_OPCODES		NUMOPCODES
+#define ITBL_NUM_MACROS			M_NUM_MACROS
diff -up --recursive --new-file binutils-2.18.macro/gas/config/tc-mips.c binutils-2.18/gas/config/tc-mips.c
--- binutils-2.18.macro/gas/config/tc-mips.c	2007-08-06 20:00:02.000000000 +0000
+++ binutils-2.18/gas/config/tc-mips.c	2007-09-30 10:57:10.000000000 +0000
@@ -2892,7 +2892,7 @@ append_insn (struct mips_cl_insn *ip, ex
 	{
 	  /* We don't keep enough information to sort these cases out.
 	     The itbl support does keep this information however, although
-	     we currently don't support itbl fprmats as part of the cop
+	     we currently don't support itbl formats as part of the cop
 	     instruction.  May want to add this support in the future.  */
 	}
       /* Never set the bit for $0, which is always zero.  */
@@ -7078,7 +7078,7 @@ macro (struct mips_cl_insn *ip)
       break;
 
    /* New code added to support COPZ instructions.
-      This code builds table entries out of the macros in mip_opcodes.
+      This code builds table entries out of the macros in mips_opcodes.
       R4000 uses interlocks to handle coproc delays.
       Other chips (like the R3000) require nops to be inserted for delays.
 
@@ -7117,23 +7117,6 @@ macro (struct mips_cl_insn *ip)
 
 #ifdef LOSING_COMPILER
     default:
-      /* Try and see if this is a new itbl instruction.
-         This code builds table entries out of the macros in mip_opcodes.
-         FIXME: For now we just assemble the expression and pass it's
-         value along as a 32-bit immediate.
-         We may want to have the assembler assemble this value,
-         so that we gain the assembler's knowledge of delay slots,
-         symbols, etc.
-         Would it be more efficient to use mask (id) here? */
-      if (itbl_have_entries
-	  && (immed_expr = itbl_assemble (ip->insn_mo->name, "")))
-	{
-	  s = ip->insn_mo->name;
-	  s2 = "cop3";
-	  coproc = ITBL_DECODE_PNUM (immed_expr);;
-	  macro_build (&immed_expr, s, "C");
-	  break;
-	}
       macro2 (ip);
       break;
     }
@@ -7923,8 +7906,25 @@ macro2 (struct mips_cl_insn *ip)
       break;
 
     default:
-      /* FIXME: Check if this is one of the itbl macros, since they
-	 are added dynamically.  */
+#if 0 /* XXX FIXME */
+      /* Try and see if this is a new itbl instruction.
+         This code builds table entries out of the macros in mips_opcodes.
+         FIXME: For now we just assemble the expression and pass its
+         value along as a 32-bit immediate.
+         We may want to have the assembler assemble this value,
+         so that we gain the assembler's knowledge of delay slots,
+         symbols, etc.
+         Would it be more efficient to use mask (id) here? */
+      if (itbl_have_entries
+	  && (immed_expr = itbl_assemble (ip->insn_mo->name, "")))
+	{
+	  s = ip->insn_mo->name;
+	  s2 = "cop3";
+	  coproc = ITBL_DECODE_PNUM (immed_expr);;
+	  macro_build (&immed_expr, s, "C");
+	  break;
+	}
+#endif
       as_bad (_("Macro %s not implemented yet"), ip->insn_mo->name);
       break;
     }
diff -up --recursive --new-file binutils-2.18.macro/gas/itbl-ops.c binutils-2.18/gas/itbl-ops.c
--- binutils-2.18.macro/gas/itbl-ops.c	2007-08-06 19:59:51.000000000 +0000
+++ binutils-2.18/gas/itbl-ops.c	2007-09-30 11:07:12.000000000 +0000
@@ -149,6 +149,7 @@ static struct itbl_entry *entries[e_npro
 
 /* local prototypes */
 static unsigned long build_opcode (struct itbl_entry *e);
+static unsigned long build_mask (struct itbl_entry *e);
 static e_type get_type (int yytype);
 static e_processor get_processor (int yyproc);
 static struct itbl_entry **get_entries (e_processor processor,
@@ -290,8 +291,8 @@ itbl_init (void)
  *   unsigned long pinfo; 	- insn flags, or INSN_MACRO
  * };
  * examples:
- *	{"li",      "t,i",  0x34000000, 0xffe00000, WR_t    },
- *	{"li",      "t,I",  0,    (int) M_LI,   INSN_MACRO  },
+ *	{ "li",     "t,i",  0x34000000, 0xffe00000, WR_t       },
+ *	{ "li",     "t,I",  0,          (int) M_LI, INSN_MACRO },
  */
 
 static char *form_args (struct itbl_entry *e);
@@ -342,24 +343,25 @@ append_insns_as_macros (void)
       es = get_entries (n, e_insn);
       for (e = *es; e; e = e->next)
 	{
-	  /* name,    args,   mask,       match,  pinfo
-		 * {"li",      "t,i",  0x34000000, 0xffe00000, WR_t    },
-		 * {"li",      "t,I",  0,    (int) M_LI,   INSN_MACRO  },
-		 * Construct args from itbl_fields.
-		*/
+	  /*   name,     args,   match,      mask,       pinfo
+	   * { "li",     "t,i",  0x34000000, 0xffe00000, WR_t       },
+	   * { "li",     "t,I",  0,          (int) M_LI, INSN_MACRO },
+	   * Construct args from itbl_fields.
+	   */
 	  o->name = e->name;
 	  o->args = strdup (form_args (e));
-	  o->mask = apply_range (e->value, e->range);
+#ifndef ITBL_USE_MACROS
+	  o->mask = build_mask (e);
 	  /* FIXME how to catch during assembly? */
 	  /* mask to identify this insn */
-	  o->match = apply_range (e->value, e->range);
+	  o->match = build_opcode (e);
 	  o->pinfo = 0;
-
-#ifdef USE_MACROS
+#else
 	  o->mask = id++;	/* FIXME how to catch during assembly? */
 	  o->match = 0;		/* for macros, the insn_isa number */
 	  o->pinfo = INSN_MACRO;
 #endif
+	  o->membership = ~0;
 
 	  /* Don't add instructions which caused an error */
 	  if (o->args)
@@ -634,7 +636,7 @@ itbl_disassemble (char *s, unsigned long
  * for each processor.
  */
 
-/* Calculate instruction's opcode and function values from entry */
+/* Calculate instruction's opcode and opcode mask from entry */
 
 static unsigned long
 build_opcode (struct itbl_entry *e)
@@ -646,6 +648,16 @@ build_opcode (struct itbl_entry *e)
   return opcode;
 }
 
+static unsigned long
+build_mask (struct itbl_entry *e)
+{
+  unsigned long opcode;
+
+  opcode = apply_range (0xffffffff, e->range);
+  opcode |= ITBL_OPCODE_MASK;
+  return opcode;
+}
+
 /* Calculate absolute value given the relative value and bit position range
  * within the instruction.
  * The range is inclusive where 0 is least significant bit.
@@ -662,10 +674,10 @@ apply_range (unsigned long rval, struct 
 {
   unsigned long mask;
   unsigned long aval;
-  int len = MAX_BITPOS - r.sbit;
+  int len = ITBL_MAX_BITPOS - r.sbit;
 
   ASSERT (r.sbit >= r.ebit);
-  ASSERT (MAX_BITPOS >= r.sbit);
+  ASSERT (ITBL_MAX_BITPOS >= r.sbit);
   ASSERT (r.ebit >= 0);
 
   /* create mask by truncating 1s by shifting */
@@ -686,7 +698,7 @@ extract_range (unsigned long aval, struc
 {
   unsigned long mask;
   unsigned long rval;
-  int len = MAX_BITPOS - r.sbit;
+  int len = ITBL_MAX_BITPOS - r.sbit;
 
   /* create mask by truncating 1s by shifting */
   mask = 0xffffffff << len;
diff -up --recursive --new-file binutils-2.18.macro/gas/itbl-ops.h binutils-2.18/gas/itbl-ops.h
--- binutils-2.18.macro/gas/itbl-ops.h	2007-08-06 19:59:51.000000000 +0000
+++ binutils-2.18/gas/itbl-ops.h	2007-09-30 11:04:16.000000000 +0000
@@ -49,6 +49,10 @@
 #define ITBL_ENCODE_PNUM(pnum) 0
 #endif
 
+#ifndef ITBL_OPCODE_MASK
+#define ITBL_OPCODE_MASK 0
+#endif
+
 typedef ITBL_TYPE t_insn;
 
 /* types of entries */


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