Re: [PATCH] MIPS: microMIPS and MCU ASE instruction set support

[Richard Sandiford <rdsandiford at googlemail dot com>]

Big patch, so this review is only for the gas and ld/testsuite bits.
I'll try to look at the rest some

Generally looks good.  It would have been better to submit the
ACE, m14kc and microMIPS support as three separate changes though.
Please can you do that for the follow-up?

>From a usability perspective, it's a shame that:

	.set	micromips
	.ent	foo
foo:
	b	1f
	nop
	.end	foo
	.ent	bar
bar:
1:	nop
	.end	bar

disassembles as:

00000000 <foo>:
   0:   cfff            b       0 <foo>
                        0: R_MICROMIPS_PC10_S1  .L11
   2:   0c00            nop
   4:   0c00            nop

00000006 <bar>:
   6:   0c00            nop

leaving the poor user with no idea what .L11 is.

The following:

	.set	micromips
	.ent	foo
foo:
	ld	$10,0x1000($11)
	.end	foo

generates an assertion failure:

Assertion failure in micromips_macro_build at gas/config/tc-mips.c line 19466.
Please report this bug.

on mipsisa64-elf with "-mips1 -mabi=32".

"Maciej W. Rozycki" <macro@codesourcery.com> writes:
> gas/
> 2010-05-18  Chao-ying Fu  <fu@mips.com>
>             Maciej W. Rozycki  <macro@codesourcery.com>
>             Daniel Jacobowitz  <dan@codesourcery.com>
>
> 	* config/tc-mips.h (mips_segment_info): Add one bit for
> 	microMIPS.
> 	* config/tc-mips.c

How about having something like:

  #define OOD_TEXT_LABELS (mips_opts.mips16 || mips_opts.micromips)

?

> 	(A_BFD_RELOC_HI16_S, A_BFD_RELOC_HI16, A_BFD_RELOC_LO16): New
> 	relocation wrapper macros.
> 	(A_BFD_RELOC_GPREL16): Likewise.
> 	(A_BFD_RELOC_MIPS_GOT16, A_BFD_RELOC_MIPS_GOT_HI16): Likewise.
> 	(A_BFD_RELOC_MIPS_GOT_LO16, A_BFD_RELOC_MIPS_HIGHEST): Likewise.
> 	(A_BFD_RELOC_MIPS_HIGHER, A_BFD_RELOC_MIPS_GOT_DISP): Likewise.
> 	(A_BFD_RELOC_MIPS_GOT_PAGE, A_BFD_RELOC_MIPS_GOT_OFST): Likewise.
> 	(A_BFD_RELOC_MIPS_SUB, A_BFD_RELOC_MIPS_JALR): Likewise.

Did you consider doing the translation from non-microMIPS to
microMIPS in the macro_* functions, rather than in their callers?
I fear it'll be too easy to accidentally forget to use A_BFD_* in future.

> 	(mips_macro_warning): Add delay_slot_16bit_p, delay_slot_32bit_p,
> 	and num_insns.
> 	(micromips_16, micromips_32): New variables.
> 	(is_micromips_16bit_p, is_micromips_32bit_p): New functions.
> 	(insn_length): Return the length of microMIPS instructions.
> 	(mips_record_mips16_mode): Rename to...
> 	(mips_record_mips16_micromips_mode): ... this.  Handle microMIPS.
> 	(install_insn): Handle microMIPS.
> 	(is_opcode_valid): Likewise.
> 	(md_begin): Likewise.

+	      if (micromips_nop16_insn.insn_mo == NULL
+		  && strcmp (name, "nop") == 0)
+		{
+		  create_insn (&micromips_nop16_insn, micromips_opcodes + i);
+		  micromips_nop16_insn.fixed_p = 1;
+		}
+	      else if (micromips_nop32_insn.insn_mo == NULL
+		  && strcmp (name, "nop") == 0)
+		{
+		  create_insn (&micromips_nop32_insn, micromips_opcodes + i);
+		  micromips_nop32_insn.fixed_p = 1;
+		}

You seem to rely on the 16-bit nop being first.  Wouldn't it be more
robust to call is_micromips_16bit_p and is_micromips_32bit_p?

> 	(MICROMIPS_TARGET, MICROMIPS_TARGET_LABEL): New macros.
> 	(micromips_add_number_label): New function.

+/* For microMIPS macros, we need to generate a local number label
+   as the target of branches.  */
+#define MICROMIPS_TARGET	"2147483647f"
+#define MICROMIPS_TARGET_LABEL	2147483647
+
+static void
+micromips_add_number_label (void)
+{
+  symbolS *s;
+  fb_label_instance_inc (MICROMIPS_TARGET_LABEL);
+  s = colon (fb_label_name (MICROMIPS_TARGET_LABEL, 0));
+  S_SET_OTHER (s, ELF_ST_SET_MICROMIPS (S_GET_OTHER (s)));
+}
+

Ugh, this is a bit hackish.  There's nothing stopping a user using
2147483647f themselves.

> 	(append_insn): Handle microMIPS.

+  if (mips_opts.micromips)
+    {
+      if ((prev_pinfo2 & INSN2_BRANCH_DELAY_16BIT)
+	  && !is_micromips_16bit_p (ip->insn_mo))
+	as_warn (_("instruction with wrong size in a branch delay slot that"
+		   " requires a 16-bit instruction"));
+      if ((prev_pinfo2 & INSN2_BRANCH_DELAY_32BIT)
+	  && !is_micromips_32bit_p (ip->insn_mo))
+	as_warn (_("instruction with wrong size in a branch delay slot that"
+		   " requires a 32-bit instruction"));
+    }
+  

Although not enforced as often as it should be, GAS convention is for
errors to start with a capital letter.

+      if (pinfo & INSN_COP)
+	{
+	  /* 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
+	     instruction.  May want to add this support in the future.  */
+	}

Assert?

+	      /* For microMIPS, disable reordering.  */
+	      || (mips_opts.micromips)

Redundant (...)

-	      if (mips_relax.sequence)
-		mips_relax.sizes[mips_relax.sequence - 1] += 4;
+	      /* MicroMIPS nop is 2 bytes.  */
+  	      if (mips_relax.sequence)
+		mips_relax.sizes[mips_relax.sequence - 1] +=
+		  mips_opts.micromips ? micromips_nop_size : 4;

Confusing comment: the microMIPS nop may be 2 or 4 bytes.
Better to say nothing IMO.

> 	(start_noreorder, end_noreorder): Likewise.

+	      frag_grow ((mips_opts.mips16 | mips_opts.micromips) ? nops * 2
+								  : nops * 4);

How about a NOP_INSN_SIZE macro?  (Or similar.)

> 	(macro_start, macro_warning, macro_end): Likewise.

+  else if ((subtype & RELAX_DELAY_SLOT_SIZE_ERROR_FIRST)
+	   || (subtype & RELAX_DELAY_SLOT_SIZE_ERROR_SECOND))
+    return _("Macro instruction of the wrong size in a branch delay slot"
+	     " that requires a 16-bit or 32-bit instruction");

Did you consider adding a flag to distinguish the 32-bit and 16-bit cases?
It'd be nice to be consistent with the non-relaxed error if possible.

+  /* If either one implementation contains one instruction, we need to check
+     the delay slot size requirement.  */

"If either implementation"

+  /* If either one implementation contains one instruction, we need to check
+     the delay slot size requirement.  */
+  if (mips_macro_warning.num_insns[0] == 1
+      || mips_macro_warning.num_insns[1] == 1)
+    {
+      if (mips_macro_warning.num_insns[0] == mips_macro_warning.num_insns[1]
+	  && mips_macro_warning.sizes[0] == mips_macro_warning.sizes[1])
+	{
+	  /* Either the macro has a single implementation or both
+	     implementations are 1 instruction with the same size.
+	     Emit the warning now.  */
+	  if ((mips_macro_warning.delay_slot_16bit_p
+	       && mips_macro_warning.sizes[0] != 2)
+	      || (mips_macro_warning.delay_slot_32bit_p
+		  && mips_macro_warning.sizes[0] != 4))
+	    {
+	      const char *msg;
+	      msg = macro_warning (RELAX_DELAY_SLOT_SIZE_ERROR_FIRST);
+	      if (msg != 0)
+		as_warn (msg);
+	    }
+	}
+      else
+	{
+	  relax_substateT subtype;
+
+	  /* Set up the relaxation warning flags.  */
+	  subtype = 0;
+	  if (mips_macro_warning.delay_slot_16bit_p)
+	    {
+	      if (mips_macro_warning.num_insns[0] != 1
+		  || mips_macro_warning.sizes[0] != 2)
+		subtype |= RELAX_DELAY_SLOT_SIZE_ERROR_FIRST;
+	      if (mips_macro_warning.num_insns[1] != 1
+		  || mips_macro_warning.sizes[1] != 2)
+		subtype |= RELAX_DELAY_SLOT_SIZE_ERROR_SECOND;
+	    }
+	  if (mips_macro_warning.delay_slot_32bit_p)
+	    {
+	      if (mips_macro_warning.num_insns[0] != 1
+		  || mips_macro_warning.sizes[0] != 4)
+		subtype |= RELAX_DELAY_SLOT_SIZE_ERROR_FIRST;
+	      if (mips_macro_warning.num_insns[1] != 1
+		  || mips_macro_warning.sizes[1] != 4)
+		subtype |= RELAX_DELAY_SLOT_SIZE_ERROR_SECOND;
+	    }
+
+	  /* One implementation might need a warning but the other
+	     definitely doesn't.  */
+	  mips_macro_warning.first_frag->fr_subtype |= subtype;
+	}
+    }

Why not work out the subtype, then check whether both ERROR_FIRST and
ERROR_SECOND are set?

+	  if (mips_macro_warning.delay_slot_p)
+	    {
+	      if (mips_macro_warning.num_insns[0] > 1)
+		subtype |= RELAX_DELAY_SLOT_SIZE_ERROR_FIRST;
+	      if (mips_macro_warning.num_insns[1] > 1)
+		subtype |= RELAX_DELAY_SLOT_SIZE_ERROR_SECOND;
+	    }

I don't get why this hunk is needed.  I thought ERROR_FIRST and ERROR_SECOND
controlled cases where a macro has a single-insn expansion that is the
wrong size, which ought to be handled by the block above.  If the code
really is needed, you should add a comment explaining why.

> 	(macro_build): Likewise.

+  if (mips_opts.micromips)
+    {
+      if (strcmp (name, "lui") == 0)
+	micromips_macro_build (ep, name, "s,u", args);
+      else if (strcmp (fmt, "d,w,<") == 0)
+	micromips_macro_build (ep, name, "t,r,<", args);
+      else
+	micromips_macro_build (ep, name, fmt, args);
+      va_end (args);
+      return;
+    }

A bit of commentary might help explain the letter switch here.

> 	(macro_build_jalr): Likewise.

+  if (mips_opts.micromips)
+    {
+      if (HAVE_NEWABI)
+	macro_build (NULL, "jalr", "t,s", RA, PIC_CALL_REG);
+      else
+	macro_build (NULL, "jalr", "mj", PIC_CALL_REG);
+    }
+  else
+    macro_build (NULL, "jalr", "d,s", RA, PIC_CALL_REG);

Why HAVE_NEWABI?  Do you want a 32-bit insn for R_MIPS_JALR?
If so, you should check MIPS_JALR_HINT_P (ep) instead.

> 	(load_register): Likewise.

-	      macro_build (&tmp, "ori", "t,r,i", reg, 0, BFD_RELOC_LO16);
+	      macro_build (&tmp, "ori", "t,r,i", reg, 0, A_BFD_RELOC_LO16);
 	      macro_build (NULL, (shift >= 32) ? "dsll32" : "dsll", "d,w,<",
-			   reg, reg, (shift >= 32) ? shift - 32 : shift);
+				  reg, reg, (shift >= 32) ? shift - 32 : shift);

Last change looks bogus.

> 	(md_apply_fix): Likewise.

-  gas_assert (fixP->fx_size == 4
+  gas_assert (fixP->fx_size == 2
+	  || fixP->fx_size == 4
 	  || fixP->fx_r_type == BFD_RELOC_16
+	  || fixP->fx_r_type == BFD_RELOC_MICROMIPS_16
 	  || fixP->fx_r_type == BFD_RELOC_64
 	  || fixP->fx_r_type == BFD_RELOC_CTOR
 	  || fixP->fx_r_type == BFD_RELOC_MIPS_SUB
+	  || fixP->fx_r_type == BFD_RELOC_MICROMIPS_SUB
 	  || fixP->fx_r_type == BFD_RELOC_VTABLE_INHERIT
 	  || fixP->fx_r_type == BFD_RELOC_VTABLE_ENTRY
 	  || fixP->fx_r_type == BFD_RELOC_MIPS_TLS_DTPREL64);
 
+
   buf = (bfd_byte *) (fixP->fx_frag->fr_literal + fixP->fx_where);

Last change is bogus.

+    case BFD_RELOC_MICROMIPS_7_PCREL_S1:
+    case BFD_RELOC_MICROMIPS_10_PCREL_S1:
+    case BFD_RELOC_MICROMIPS_16_PCREL_S1:
+      /* We adjust the offset back to even.  */
+      if ((*valP & 0x1) != 0)
+	--(*valP);
+
+      if (! fixP->fx_done)
+	break;
+
+      /* Should never visit here, because we keep the relocation.  */
+      abort ();
+      break;

I suppose this silently ignores branches to non-microMIPS code,
but there again, so does the MIPS16 equivalent...

> 	(mips_relax_frag): Handle microMIPS.

+     gas_assert (fixp->fx_r_type == BFD_RELOC_16_PCREL_S2
+	      || fixp->fx_r_type == BFD_RELOC_MICROMIPS_7_PCREL_S1
+	      || fixp->fx_r_type == BFD_RELOC_MICROMIPS_10_PCREL_S1
+	      || fixp->fx_r_type == BFD_RELOC_MICROMIPS_16_PCREL_S1);
+
+      /* For 7/10 PCREL_S1, we just need to use fixp->fx_addnumber.  */
+      if (fixp->fx_r_type == BFD_RELOC_MICROMIPS_7_PCREL_S1
+	  || fixp->fx_r_type == BFD_RELOC_MICROMIPS_10_PCREL_S1)
+	reloc->addend = fixp->fx_addnumber;
+      else
+	/* At this point, fx_addnumber is "symbol offset - pcrel address".
+	   Relocations want only the symbol offset.  */
+	reloc->addend = fixp->fx_addnumber + reloc->address;

A better comment is needed.  _Why_ do you just need fx_addnumber?

> 	(md_convert_frag): Likewise.

-      /* Possibly emit a warning if we've chosen the longer option.  */
-      if (((fragp->fr_subtype & RELAX_USE_SECOND) != 0)
-	  == ((fragp->fr_subtype & RELAX_SECOND_LONGER) != 0))
+      if (!(fragp->fr_subtype & RELAX_USE_SECOND))
+  	{
+	  /* Check if the size in branch delay slot is ok.  */
+	  if (fragp->fr_subtype & RELAX_DELAY_SLOT_SIZE_ERROR_FIRST)
+	    {
+	      const char *msg = macro_warning (fragp->fr_subtype);
+	      if (msg != 0)
+		as_warn_where (fragp->fr_file, fragp->fr_line, msg);
+	    }
+	}
+      else
 	{
-	  const char *msg = macro_warning (fragp->fr_subtype);
-	  if (msg != 0)
-	    as_warn_where (fragp->fr_file, fragp->fr_line, "%s", msg);
+	  /* Check if the size in branch delay slot is ok.
+	     Possibly emit a warning if we've chosen the longer option.  */
+	  if ((fragp->fr_subtype & RELAX_DELAY_SLOT_SIZE_ERROR_SECOND)
+	      || (fragp->fr_subtype & RELAX_SECOND_LONGER))
+	    {
+	      const char *msg = macro_warning (fragp->fr_subtype);
+	      if (msg != 0)
+		as_warn_where (fragp->fr_file, fragp->fr_line, msg);
+	    }
 	}

This doesn't accurately preserve the previous:

      if (((fragp->fr_subtype & RELAX_USE_SECOND) != 0)
	  == ((fragp->fr_subtype & RELAX_SECOND_LONGER) != 0))

behaviour.

> 	(mips_handle_align): Likewise.

       /* If we're not inserting a whole number of instructions,
-	 pad the end of the fixed part of the frag with zeros.  */
-      memset (p, 0, excess);
+         pad the end of the fixed part of the frag with zeros.
+         But try to fit a short microMIPS nop too if applicable.  */
+      if (excess < 2 || *p != 2)
+	memset (p, 0, excess);
+      else
+	{
+	  memset (p, 0, excess - 2);
+	  md_number_to_chars (p + excess - 2,
+			      micromips_nop16_insn.insn_opcode, 2);
+	}

Not how it'd be written from scratch; the comment makes microMIPS sound
like an afterthought.  Please handle the microMIPS case first and make
the comment treat it as a first-class citizen.

> 	(micromips_ip): New function.

+      /* Try to search "16" or "32" in the str.  */
+      if ((t = strstr (str, "16")) != NULL && t < save_s)
+	{
+	  /* Make sure "16" is before the first '.' if '.' exists.  */
+	  if ((s = strchr (str, '.')) != NULL && (t + 2 != s))
+	    {
+	      insn_error = "unrecognized opcode";
+	      return;
+	    }
+
+	  /* Make sure "16" is at the end of insn name, if no '.'.  */
+	  if ((s = strchr (str, '.')) == NULL
+	      && (!ISSPACE (*(t + 2)) && *(t + 2) != '\0'))
+	    {
+	      insn_error = "unrecognized opcode";
+	      return;
+	    }
+
+	  micromips_16 = TRUE;
+	  for (s = t + 2; *s != '\0'; ++s)
+	    *(s - 2) = *s;
+	  *(s - 2) = '\0';
+
+	  for (s = t; *s != '\0' && !ISSPACE (*s); ++s)
+	    continue;
+
+	  if (ISSPACE (*s))
+	    {
+	      save_c = *s;
+	      *s++ = '\0';
+	    }
+
+	  if ((insn = (struct mips_opcode *) hash_find (micromips_op_hash, str))
+	      == NULL)
+	    {
+	      int i;
+	      int length;
+	      micromips_16 = FALSE;
+
+	      /* Restore the character we overwrite above (if any).  */
+	      if (save_c)
+		*(--s) = save_c;
+
+	      length = strlen (str);
+	      for (i = length - 1; &str[i] >= t; i--)
+		{
+		  str[i + 2] = str[i];
+		  if (t == &str[i])
+		    {
+		      str[i + 1] = '6';
+		      str[i] = '1';
+		      str[length + 2] = '\0';
+		      break;
+		    }
+		}
+
+	      insn_error = "unrecognized 16-bit version of microMIPS opcode";
+	      return;
+	    }
+	}
+      else if ((t = strstr (str, "32")) != NULL && t < save_s)
+	{
+	  /* For some instruction names, we already have 32, so we need
+	     to seek the second 32 to process.  Ex: bposge3232, dsra3232.  */
+	  char *new_t;
+	  if ((new_t = strstr (t + 2, "32")) != NULL && new_t < save_s)
+	    t = new_t;
+
+	  /* Make sure "32" is before the first '.' if '.' exists.  */
+	  if ((s = strchr (str, '.')) != NULL && (t + 2 != s))
+	    {
+	      insn_error = "unrecognized opcode";
+	      return;
+	    }
+
+	  /* Make sure "32" is at the end of the name, if no '.'.  */
+	  if ((s = strchr (str, '.')) == NULL
+	      && (!ISSPACE (*(t + 2)) && *(t + 2) != '\0'))
+	    {
+	      insn_error = "unrecognized opcode";
+	      return;
+	    }
+
+	  micromips_32 = TRUE;
+	  for (s = t + 2; *s != '\0'; ++s)
+	    *(s - 2) = *s;
+	  *(s - 2) = '\0';
+
+	  for (s = t; *s != '\0' && !ISSPACE (*s); ++s)
+	    continue;
+
+	  if (ISSPACE (*s))
+	    {
+	      save_c = *s;
+	      *s++ = '\0';
+	    }
+
+	  if ((insn = (struct mips_opcode *) hash_find (micromips_op_hash, str))
+	      == NULL)
+	    {
+	      int i;
+	      int length;
+	      micromips_32 = FALSE;
+
+	      /* Restore the character we overwrite above (if any).  */
+	      if (save_c)
+		*(--s) = save_c;
+
+	      length = strlen (str);
+	      for (i = length - 1; &str[i] >= t; i--)
+		{
+		  str[i + 2] = str[i];
+		  if (t == &str[i])
+		    {
+		      str[i + 1] = '2';
+		      str[i] = '3';
+		      str[length + 2] = '\0';
+		      break;
+		    }
+		}
+
+	      insn_error = "unrecognized 32-bit version of microMIPS opcode";
+	      return;
+	    }

Far too much cut-&-paste between the "16" and "32" cases.  Also:

+      if ((t = strstr (str, "16")) != NULL && t < save_s)

t < save_s must surely be true, since save_s is the null terminator.

+	  /* Make sure "16" is before the first '.' if '.' exists.  */
+	  if ((s = strchr (str, '.')) != NULL && (t + 2 != s))
+	    {
+	      insn_error = "unrecognized opcode";
+	      return;
+	    }
+
+	  /* Make sure "16" is at the end of insn name, if no '.'.  */
+	  if ((s = strchr (str, '.')) == NULL
+	      && (!ISSPACE (*(t + 2)) && *(t + 2) != '\0'))
+	    {
+	      insn_error = "unrecognized opcode";
+	      return;
+	    }

Don't call strchr (str, '.') twice like that.  Better would be:

	s = strchr (str, '.');

followed by the two checks.  Isn't the ISSPACE check redundant though,
given that you've terminated the string at the first space?  I would
have thought:

        if (t + 2 != (s ? s : save_s))

would be enough.  Errors should start with a capital letter.  Missing
internationalisation.

You could use alloca to create an opcode without the "16" or "32",
which would make the error-reporting code simpler.  It's best not
to change the user's source line if we can help it.

+	      if (!insn_error)
+		{
+		  static char buf[100];
+		  sprintf (buf,
+			   _("opcode not supported on this processor: %s (%s)"),
+			   mips_cpu_info_from_arch (mips_opts.arch)->name,
+			   mips_cpu_info_from_isa (mips_opts.isa)->name);
+		  insn_error = buf;
+		}
+	      if (save_c)
+		*(--s) = save_c;
+
+	      if (micromips_16 || micromips_32)
+		{
+		  int i;
+		  int length;
+
+		  length = strlen (str);
+		  for (i = length - 1; i >= 0; i--)
+		    {
+		      str[i + 2] = str[i];
+		      if (t == &str[i])
+			break;
+		    }
+		  if (micromips_16)
+		    {
+		      insn_error =
+			"unrecognized 16-bit version of microMIPS opcode";
+		      str[i + 1] = '6';
+		      str[i] = '1';
+		    }
+		  else
+		    {
+		      insn_error =
+			"unrecognized 32-bit version of microMIPS opcode";
+		      str[i + 1] = '2';
+		      str[i] = '3';
+		    }
+		  str[length + 2] = '\0';
+		}
+	      return;

Why override the insn_error unconditionally like this?  E.g.:

	jar16	$30,$26

    Error: unrecognized 16-bit version of microMIPS opcode `jar16 $30,$26'

implies there's a 32-bit opcode.  I'd also have thought that the
"opcode not supported on this processor" would triumph if it applies.

+do_lsb:

Not properly indented.  A few other instances.

+			}
+		    }
+	/* Now that we have assembled one operand, we use the args string
+	 * to figure out where it goes in the instruction.  */
+		  switch (c)
+		    {

Bogus indentation.

> 	(micromips_macro_build): Likewise.
> 	(micromips_macro): Likewise.

I'll look at micromips_ip and these two in more detail later.

> 	* ld-mips-elf/jalx-2-main.s: New.
> 	* ld-mips-elf/jalx-2.dd: New.
> 	* ld-mips-elf/jalx-2-ex.s: New.
> 	* ld-mips-elf/jalx-2-printf.s: New.
> 	* ld-mips-elf/mips-elf.exp: Run new test.

Please make the .dd output less susceptible to things like the number
of sections, size of program headers, etc.  One way is to use a linker
script to place each section at a nice round address.  Another is to
".*" out the addresses and instruction encodings and just reply on
the symbolic part of the disassembly.  I think the former's better
here.  There are quite a few existing examples.

Richard