Re: [PATCH] MIPS: microMIPS ASE support

["Maciej W. Rozycki" <macro at codesourcery dot com>]

Hi Richard,

> Thanks for the updates.  I'll try to go through the patches in the next
> couple of weeks (maybe later this weekend).

 I've been distracted for a while, but got back to this effort again.  
Here's another update, combining your two replies.  Thanks for your 
ongoing review.

> >> > +/* Return true if the given CPU supports microMIPS.  */
> >> > +#define CPU_HAS_MICROMIPS(cpu)	0
> >> 
> >> out.  I think the CPU_HAS_MIPS16 stuff is derived from the original LSI
> >> TinyRisc support and wouldn't be used for ASEs.
> >
> >  The microMIPS ASE provides for processors that do not support the 
> > standard MIPS instruction set.  These I think should default to the 
> > microMIPS mode.  I suspect someone will eventually implement such a 
> > processor as since we've got this code implemented here already I'd like 
> > to leave it as a placeholder.  I think it's not much of a burden, is it?
> 
> Oh no, it wasn't any sense of burden that bothered me.  It was more
> the talk of "the microMIPS processor".  "The MIPS16 processor" made
> sense when the support was first added, but plain "microMIPS" makes
> more sense here.  (Or, reading further on, I suppose you won't agree.
> Something other than "processor" though, if you want to treat
> "microMIPS" as an adjective.)
> 
> I just thought that, if this was dead code, we might as well just
> remove it rather than quibble about wording.  Given what you say about
> microMIPS-only processors being possible though, please just change the
> comment instead.

 "Return true if the given CPU supports the microMIPS ASE." it is then.

> >> > +#define RELAX_MICROMIPS_ENCODE(type, is_16bit, uncond, link, toofar)	\
> >> > +  (0x40000000							\
> >> > +   | ((type) & 0xff)						\
> >> > +   | ((is_16bit) ? 0x100 : 0)					\
> >> > +   | ((uncond) ? 0x200 : 0)					\
> >> > +   | ((link) ? 0x400 : 0)					\
> >> > +   | ((toofar) ? 0x800 : 0))
> >> > +#define RELAX_MICROMIPS_P(i) (((i) & 0xc0000000) == 0x40000000)
> >> > +#define RELAX_MICROMIPS_TYPE(i) ((i) & 0xff)
> >> > +#define RELAX_MICROMIPS_USER_16BIT(i) (((i) & 0x100) != 0)
> >> > +#define RELAX_MICROMIPS_UNCOND(i) (((i) & 0x200) != 0)
> >> > +#define RELAX_MICROMIPS_LINK(i) (((i) & 0x400) != 0)
> >> > +#define RELAX_MICROMIPS_TOOFAR(i) (((i) & 0x800) != 0)
> >> > +#define RELAX_MICROMIPS_MARK_TOOFAR(i) ((i) | 0x800)
> >> > +#define RELAX_MICROMIPS_CLEAR_TOOFAR(i) ((i) & ~0x800)
> >> 
> >> Is there a need to create variant frags when the user has explicitly
> >> specified the instruction size?  I wouldn't have expected any relaxation
> >> to be necessary in that case, and it looks like the relaxation code does
> >> indeed return 2 whenever USER_16BIT is true.
> >
> >  I suspect this has been copied over from MIPS16 code.  
> > RELAX_MIPS16_USER_SMALL seems to be used in a similar fashion.  Do you 
> > happen to know for sure why it has been implemented this way for MIPS16 
> > assembly?
> 
> Nope :-)

 With a sudden insight I realised this is a workaround for the lack of 
MIPS16 branch relocations.  As no actual relocation can be encoded in 
offset_expr, relaxation is used unconditionally instead and the definition 
of the relocatable field carried through as a transformed argument code.

> > My suspicion is we want to keep the relocation until the final 
> > relaxation so that if the final value turns out to fit afterwards (but not 
> > until then) in the forced-truncated immediate field of the instruction 
> > nothing is lost.
> 
> But that's true of all fixups, and should already be handled correctly.
> 
> E.g. if you had microMIPS code embedded in a larger MIPS function, you
> might have normal MIPS branches that cross relaxable microMIPS instructions.
> The same consideration would apply then, even if branch relaxation
> wasn't enabled.

 Given the MIPS16 justification above, I have removed this extra bit from 
type encoding and modified code to emit forced 16-bit branches straight 
away.  No regressions in the test suite.

> >> > +/* These are the bitmasks and shift counts used for the different
> >> > +   fields in the instruction formats.  Other than OP, no masks are
> >> > +   provided for the fixed portions of an instruction, since they are
> >> > +   not needed.  */
> >> 
> >> Seems like too much cut-&-paste: there isn't an OP field here.
> >> "Other than TARGET", perhaps, unless there are other opcode masks here.
> >
> >  This looks like copied verbatim from the MIPS16 part.  The two parts are 
> > functionally equivalent and my understanding of the comment is no masks 
> > are provided for the non-operand parts of instruction.  I've left the 
> > comment as is; I'm not sure what TARGET might mean in this context, please 
> > elaborate.
> 
> As you say, the MIPS16 comment is:
> 
> /* These are the bitmasks and shift counts used for the different
>    fields in the instruction formats.  Other than OP, no masks are
>    provided for the fixed portions of an instruction, since they are
>    not needed.
> 
> OP in this case refers to the first field definition:
> 
> #define MIPS16OP_MASK_OP	0x1f
> #define MIPS16OP_SH_OP		11
> 
> which is the opcode ("fixed portion").  There didn't seem to be
> a corresponding MASK_OP and SH_OP for microMIPS.

 I see what you mean now, I have merely truncated the comment.  I think 
the mask combinations used for the various instructions are too diverse to 
have them all listed here.  Also I've dropped MICROMIPSOP_{MASK,SH}_MAJOR 
that were unused (and were what OP would be referring to otherwise).

> >> > +/* Return 1 if a symbol associated with the location being disassembled
> >> > +   indicates a compressed mode, either MIPS16 or microMIPS one.  Otherwise,
> >> > +   return 0.  */
> >> 
> >> Reads more naturally to me without "one".
> >
> >  Both MIPS16 and microMIPS are adjectives; they need a noun or a pronoun.  
> > I realise this requirement is not met everywhere, but that doesn't mean we 
> > should add new such places IMO.
> 
> Really?  I'd have thought they were nouns.  If you want them to be
> adjectives though, it should be "either the ...".

 Well, that comes from US trademark law I'm told -- apparently you can't 
defend a trademark that's grammatically not an adjective.  Don't ask me 
for further details (that's certainly language-specific too, for example 
I've heard of no such restriction in Polish trademark law and Polish 
trademarks are generally nouns, because the way the Polish grammar defines 
how adjectives are made from other words, typically nouns, renders them 
mostly useless for this purpose).

> >> > +  for (i = 0; i < info->num_symbols; i++)
> >> > +    {
> >> > +      pos = info->symtab_pos + i;
> >> > +
> >> > +      if (bfd_asymbol_flavour (info->symtab[pos]) != bfd_target_elf_flavour)
> >> > +	continue;
> >> > +
> >> > +      symbol = (elf_symbol_type *) info->symtab[pos];
> >> > +      if ((!micromips_ase
> >> > +	   && ELF_ST_IS_MIPS16 (symbol->internal_elf_sym.st_other))
> >> > +	  || (micromips_ase
> >> > +	      && ELF_ST_IS_MICROMIPS (symbol->internal_elf_sym.st_other)))
> >> > +	    return 1;
> >> > +    }
> >> 
> >> Why is a search necessary here, when the previous code was happy to
> >> look only at the first symbol?  I'm not saying the code is wrong,
> >> but a bit of commentary would be good.
> >
> >  My feeling is previous code was not "happy", but simply untested (or to 
> > be more accurate, not tested satisfactorily).
> >
> >  Symbols sharing the same address are sorted alphabetically here which 
> > becomes a problem when they include both objects and functions (or symbols 
> > derived from standard MIPS functions defined elsewhere).  Disassembly 
> > shouldn't yield different results based merely on the names of symbols 
> > chosen and given the semantics of the compressed annotation (it is only 
> > added to a function symbol if a genuine instruction has been emitted 
> > following immediately in the source code) I think it should take 
> > precedence, so we check if any symbol has one.
> 
> Agreed, and like I say, I was willing to believe the new code was right.
> Please put in a comment along these lines.

 I have updated the comment; please see if the new version is good enough.

> >> What problem is the ld-lib.exp change fixing?
> >
> >  Currently you can't build the same source file multiple times with 
> > different flags.  See ld/testsuite/ld-mips-elf/mips16-and-micromips.d for 
> > a use case (and try it with the ld-lib.exp piece reverted).  I think the 
> > framework shouldn't be limiting the developer like this and making a copy 
> > of the source to work around the limitation sounds to me like the wrong 
> > direction to go.
> 
> OK, thanks, makes sense.  The ld-lib.exp change is independently OK.
> Please commit it separately.

 Thanks, split off now.

> >  As it has turned out in the course of sorting out some earlier concerns 
> > the microMIPS change needs a couple of updates.  For your reference I'm 
> > sending the current version of the original patch as it had to be 
> > regenerated.  On top of this I'm sending the following updates:
> 
> Everything except binutils-gas-umips-swap.diff is OK (as one commit,
> like you say), with the changes below.  If you don't agree with some
> of the requested changes, let me know.
> 
> I thinkb inutils-gas-umips-swap.diff should go in as a separate commit,
> and I'll review it separately.

 It certainly makes sense to me.

> > - binutils-umips-opcode-trap.diff -- a complementing microMIPS change to 
> >   the trap/no-delay slot annotation made to standard MIPS/MIPS16 code,
> 
> Nit:
> 
> -      target_is_micromips_code_p = (htab->splt != sec)
> -				    && ELF_ST_IS_MICROMIPS (h->root.other);
> +      target_is_micromips_code_p = ((htab->splt != sec)
> +				    && ELF_ST_IS_MICROMIPS (h->root.other));
> 
> should be:
> 
>       target_is_micromips_code_p = (htab->splt != sec
> 				    && ELF_ST_IS_MICROMIPS (h->root.other));

 Indeed, fixed.

> -      if (isym->st_shndx == sec_shndx
> -	  && isym->st_value > addr
> -	  && isym->st_value < toaddr)
> +      bfd_vma value;
> +
> +      if (isym->st_shndx != sec_shndx)
> +	continue;
> +
> +      value = isym->st_value;
> +      if (ELF_ST_IS_MICROMIPS (isym->st_other))
> +	value &= MINUS_TWO;
> +      if (value > addr)
>  	isym->st_value -= count;
> 
> I still don't understand why we need to mask the low bit here.
> As per the original review, aren't these symbols already even?
> Only those entered into the hash table are odd.  OK as:
> 
>       if (isym->st_shndx == sec_shndx
> 	  && isym->st_value > addr)
> 	isym->st_value -= count;
> 
> if that's correct, but please let me know if it isn't.

 I think you're right.  I hope GCC is smart enough not to read the memory 
behind the pointer twice.

> @@ -11936,12 +11933,17 @@ mips_elf_relax_delete_bytes (bfd *abfd,
>    for (; sym_hashes < end_hashes; sym_hashes++)
>      {
>        struct elf_link_hash_entry *sym_hash = *sym_hashes;
> +      bfd_vma value;
>  
> -      if ((sym_hash->root.type == bfd_link_hash_defined
> -	   || sym_hash->root.type == bfd_link_hash_defweak)
> -	  && sym_hash->root.u.def.section == sec
> -	  && sym_hash->root.u.def.value > addr
> -	  && sym_hash->root.u.def.value < toaddr)
> +      if ((sym_hash->root.type != bfd_link_hash_defined
> +	   && sym_hash->root.type != bfd_link_hash_defweak)
> +	  || sym_hash->root.u.def.section != sec)
> +	continue;
> +
> +      value = sym_hash->root.u.def.value;
> +      if (ELF_ST_IS_MICROMIPS (sym_hash->other))
> +	value &= MINUS_TWO;
> +      if (value > addr)
>  	sym_hash->root.u.def.value -= count;
>      }
> 
> Very much nit stage, but "continue" seems overkill here.  I preferred
> the original style, which doesn't have the combination of positive
> and negative tests.  OK as:
> 
>       if ((sym_hash->root.type == bfd_link_hash_defined
> 	   || sym_hash->root.type == bfd_link_hash_defweak)
> 	  && sym_hash->root.u.def.section == sec)
> 	{
> 	  bfd_vma value;
> 
> 	  value = sym_hash->root.u.def.value;
> 	  if (ELF_ST_IS_MICROMIPS (sym_hash->other))
> 	    value &= MINUS_TWO;
> 	  if (value > addr)
> 	    sym_hash->root.u.def.value -= count;
> 	}

 I don't like having too much indentation, but I won't insist.

> +	  /* See if there is a jump or a branch reloc preceding the
> +	     LUI instruction immediately.  */
> +	  for (ibrrel = internal_relocs; ibrrel < irelend; ibrrel++)
> +	    {
> +	      offset = irel->r_offset - ibrrel->r_offset;
> +	      if (offset != 2 && offset != 4)
> +		continue;
> +
> +	      br_r_type = ELF32_R_TYPE (ibrrel->r_info);
> +	      if (offset == 2
> +		  && (br_r_type == R_MICROMIPS_PC7_S1
> +		      || br_r_type == R_MICROMIPS_PC10_S1
> +		      || br_r_type == R_MICROMIPS_JALR))
> +		break;
> +	      if (offset == 4
> +		  && (br_r_type == R_MICROMIPS_26_S1
> +		      || br_r_type == R_MICROMIPS_PC16_S1
> +		      || br_r_type == R_MICROMIPS_JALR))
> +		{
> +		  bfd_byte *ptr = contents + ibrrel->r_offset;
> +		  unsigned long bropc;
> +
> +		  bropc   = bfd_get_16 (abfd, ptr);
> +		  bropc <<= 16;
> +		  bropc  |= bfd_get_16 (abfd, ptr + 2);
> +		  /* Compact branches are OK.  */
> +		  if (find_match (opcode, bzc_insns_32) >= 0)
> +		    brc = TRUE;
> +		  break;
> +		}
> +	    }
> +	  /* A delay slot was found, give up, sigh...  */
> +	  if (!brc && ibrrel < irelend)
> +	    continue;
> +
> +	  /* Otherwise see if the LUI instruction *might* be in a
> +	     branch delay slot.  */
> +	  if (!brc)
> +	    {
> +	      bfd_byte *ptr = contents + irel->r_offset;
> +
> +	      if (irel->r_offset >= 2)
> +		bdsize = check_br16_dslot (abfd, ptr - 2);
> +	      /* A branch possibly found, give up, sigh...  */
> +	      if (bdsize > 0)
> +		continue;
> +	      if (irel->r_offset >= 4)
> +		bdsize = check_br32_dslot (abfd, ptr - 4);
> +	      /* A branch possibly found, give up, sigh...  */
> +	      if (bdsize > 0)
> +		continue;
> +	    }
> 
> ISTR discussing this before, but with the new approach, it ought not to
> be necessary to check the relocations for this get-out:
> 
> 	  /* A delay slot was found, give up, sigh...  */
> 	  if (!brc && ibrrel < irelend)
> 	    continue;
> 
> because the following code ought to detect the same cases (and do
> so much more cheaply).  If you want to avoid this:
> 
> 	      if (irel->r_offset >= 2)
> 		bdsize = check_br16_dslot (abfd, ptr - 2);
> 	      /* A branch possibly found, give up, sigh...  */
> 	      if (bdsize > 0)
> 		continue;
> 
> triggering for cases where the relocs tell us that the instruction
> is actually a BRC, then I think it would be better to split the
> search out into a separate function and only use it when we would
> otherwise continue.  OK as:
> 
> /* See if relocations [INTERNAL_RELOCS, IRELEND) confirm that there
>    is a 4-byte branch at offset OFFSET.  */
> 
> static boolean
> check_4byte_branch (Elf_Internal_Rela *internal_relocs,
> 		    Elf_Internal_Rela *irelend, bfd_vma offset)
> {
>   Elf_Internal_Rela *irel;
>   unsigned long r_type;
> 
>   for (irel = internal_relocs; irel < irelend; irel++)
>     if (irel->r_offset == offset)
>       {
> 	r_type = ELF32_R_TYPE (ibrrel->r_info);
> 	if (br_r_type == R_MICROMIPS_26_S1
> 	    || br_r_type == R_MICROMIPS_PC16_S1
> 	    || br_r_type == R_MICROMIPS_JALR)
> 	  return TRUE;
>       }
>   return FALSE;
> }
> ...
> 
> 	  /* See if the LUI instruction *might* be in a branch delay slot.  */
> 	  if (irel->r_offset >= 2
> 	      && check_br16_dslot (abfd, ptr - 2) > 0
> 	      && !(irel->r_offset >= 4
> 		   /* If the instruction is actually a 4-byte branch,
> 		      the value of check_br16_dslot doesn't matter.
> 		      We should use check_br32_dslot to check whether
> 		      the branch has a delay slot.  */
> 		   && check_4byte_branch (internal_relocs, irelend,
> 					  irel->r_offset - 4)))
> 	    continue;
> 	  if (irel->r_offset >= 4
> 	      && check_br32_dslot (abfd, ptr - 4) > 0)
> 	    continue;
> 
> if that's correct (with trivial fixes to make it compile :-)),
> otherwise please let me know.

 You are right, in principle, but actually we can check for compact branch 
encodings first and only if that has succeded, then scan the relocations.  
Which is what I did in the end.

 Perhaps that's too much hassle for handling a corner case, but that's 
just a couple of lines of code.  Let me know if you'd rather I removed it 
altogether.

> Of course, this could be generalised so that if the relocations say
> we have any type of 4-byte instruction, check_br16_dslot doesn't matter,
> and vice versa.  But even if you'd like to do that, it's follow-on material.
> Let's get the current code in first.

 Yeah, let's skip it for now, and if anyone ever gets back to it, then 
they may rethink the compact branch special case too. :)

> bdsize should be dead after the changes above.

 Yes, and a couple of others.

> -	  /* Give up if not the same register used with both relocations.  */
> +	  /* Give up unless the same register used with both relocations.  */
> 
> Should be:
> 
> 	  /* Give up unless the same register is used with both relocations.  */

 OK.

> +/* Check if S points at a valid register list according to TYPES.
> +   If so, then return 1, advance S to consume the list and store
> +   the registers present on the list as a bitmask of ones in REGLISTP,
> +   otherwise return 0.  A valid list comprises a comma-separated
> +   enumeration of valid single registers and/or dash-separated
> +   contiguous register ranges as determined by their numbers.
> +
> +   As a special exception if one of s0-s7 registers is specified as
> +   the range's lower delimiter and s8 (fp) is its upper one, then no
> +   registers whose numbers place them between s7 and s8 (i.e. $24-$29)
> +   are selected; they have to be named separately if needed.  */
> +
> +static int
> +reglist_lookup (char **s, unsigned int types, unsigned int *reglistp)
> +{
> +  unsigned int reglist = 0;
> +  unsigned int lastregno;
> +  bfd_boolean ok = TRUE;
> +  unsigned int regmask;
> +  unsigned int regno;
> +  char *s_reset = *s;
> +  char *s_comma = *s;
> +
> +  while (reg_lookup (s, types, &regno))
> +    {
> +      lastregno = regno;
> +      if (**s == '-')
> +	{
> +	  (*s)++;
> +	  ok = reg_lookup (s, types, &lastregno);
> +	  if (ok && lastregno < regno)
> +	    ok = FALSE;
> +	  if (!ok)
> +	    break;
> +	}
> +
> +      if (lastregno == FP && regno >= S0 && regno <= S7)
> +	{
> +	  lastregno = S7;
> +	  reglist |= 1 << FP;
> +	}
> +      regmask = 1 << lastregno;
> +      regmask = (regmask << 1) - 1;
> +      regmask ^= (1 << regno) - 1;
> +      reglist |= regmask;
> +
> +      s_comma = *s;
> +      if (**s != ',')
> +	break;
> +      (*s)++;
> +    }
> +
> +  if (ok)
> +    *s = s_comma;
> +  else
> +    *s = s_reset;
> +  if (reglistp)
> +    *reglistp = reglist;
> +  return ok && reglist != 0;
> +}
> 
> I found s_comma a confusing name for something that often doesn't
> point to a comma.  OK as "s_end_of_reglist", otherwise let me know.

 Well, "s_comma" points to anything other than a comma for invalid syntax 
only, but I think "s_endlist" sounds better indeed.  Your proposal seems a 
bit legthy and the number of letters we have available is at most 
countable, so a bit of thrift won't hurt.

> +      /* If the previous instruction has an incorrect size for a fixed
> +         branch delay slot in the microMIPS mode, we cannot swap.  */
> 
> OK as "in microMIPS mode".
> 
> As discussed later, from the original patch:
> 
> +/* These are the bitmasks and shift counts used for the different
> +   fields in the instruction formats.  Other than OP, no masks are
> +   provided for the fixed portions of an instruction, since they are
> +   not needed.  */
> 
> Just drop the "Other than OP, ".

 Yes, especially with the removal of MICROMIPSOP_{MASK,SH}_MAJOR.

> > - binutils-gas-mips-fix-adjust-reloc.diff -- a fix for relocation handling 
> >   problems discovered while fixing the issue with PC-relative relocations 
> >   discussed earlier; a summary of the changes:
> >
> >   * BFD_RELOC_MICROMIPS_JALR relocs are now explicitly excluded like their
> >     standard MIPS counterpart; this bug was covered by all microMIPS being 
> >     converted to section-relative ones,
> >
> >   * unlike MIPS16 code we don't have call stubs in the microMIPS mode and 
> >     therefore of the remaing relocs affecting microMIPS code only jump 
> >     relocations against microMIPS text symbols on REL targets are 
> >     converted to section-relative ones as the in-place relocatable field 
> >     strips out the ISA bit,
> >
> >   * therefore we don't have to tag symbols that have a microMIPS jump 
> >     relocation against them, because they're going to be handled just fine 
> >     as long as the symbol is not a microMIPS text one,
> 
> Makes sense.  OK as long as you split this:
> 
> @@ -17189,7 +17187,9 @@ mips_fix_adjustable (fixS *fixp)
> +	  || fixp->fx_r_type == BFD_RELOC_MIPS_JALR
> +	  || fixp->fx_r_type == BFD_RELOC_MICROMIPS_JALR))
> 
> into a jalr_reloc_p, which should be defined alongside the existing
> *_reloc_p functions.  Likewise:
> 
> +	      && (fixp->fx_r_type == BFD_RELOC_MIPS_JMP
> +		  || fixp->fx_r_type == BFD_RELOC_MICROMIPS_JMP))))
> 
> and jmp_reloc_p.  (I think you already use this condition elsewhere,
> so please change those too.)

 Both are used once each only, but I'm fine with such a change.  Some 
other relocs might be handled like this too.

> > - binutils-umips-relax16.diff -- the original 16-bit->32-bit->out-of-range 
> >   branch relaxation change, regenerated,
> 
> +      fragp->fr_var= length;
> 
> Missing space.
> 
> +      /* Handle 32-bit branches that fit or forced to fit.  */
> 
> "are forced to fit"

 Fixed.

>       /* Check the short-delay-slot bit.  */
>       if (al && (insn & 0x02000000) != 0)
>  	{
> 	  jal = 0x74000000;				/* jals  */
> 	  jalr = 0x45e0;				/* jalrs  */
> 	}
> 
> This is now quite far (and IMO confusingly far) from the code that sets
> the default insns.  OK if you replace:
> 
> +      unsigned long jal = 0xf4000000;			/* jal  */
> +      unsigned long jalr = 0x45c0;			/* jalr  */
> 
> with:
> 
> +      unsigned long jal, jalr;
> 
> and add:
> 
>       else
> 	{
> 	  jal = 0xf4000000;				/* jal  */
> 	  jalr = 0x45c0;				/* jalr  */
> 	}
> 
> to the condition above.
> 
> I think it'd be more consistent to set "jr" here too, but it's OK
> either way.

 So I've moved them close to their points of use instead.

> > - binutils-umips-fix-reloc.diff -- the original microMIPS relocation 
> >   handling divergence reduction change, regenerated,
> 
> +	      {
> +		bfd_reloc_code_real_type reloc;
> +		int shift;
> +
> +		reloc = micromips_map_reloc (orig_reloc);
> +		shift = reloc == BFD_RELOC_MICROMIPS_JMP ? 1 : 2;
> +		if ((address_expr->X_add_number & ((1 << shift) - 1)) != 0)
> +		  as_bad (_("jump to misaligned address (0x%lx)"),
> +			  (unsigned long) address_expr->X_add_number);
> +		ip->insn_opcode |= ((address_expr->X_add_number >> shift)
> +				    & 0x3ffffff);
> +	      }
> 
> OK if you replace:
> 
> +		reloc = micromips_map_reloc (orig_reloc);
> +		shift = reloc == BFD_RELOC_MICROMIPS_JMP ? 1 : 2;
> 
> with:
> 
> +		shift = mips_opts.micromips ? 1 : 2;
> 
> Same for BFD_RELOC_16_PCREL_S2.

 Adjusted.

> +	  reloc = micromips_map_reloc (reloc_type[i - 1]);
> +	  howto = bfd_reloc_type_lookup (stdoutput, reloc);
>  	  if (howto == NULL)
>  	    {
>  	      /* To reproduce this failure try assembling gas/testsuites/
>  		 gas/mips/mips16-intermix.s with a mips-ecoff targeted
>  		 assembler.  */
> -	      as_bad (_("Unsupported MIPS relocation number %d"), reloc_type[i -
> 1]);
> +	      as_bad (_("Unsupported MIPS relocation number %d"), reloc);
>  	      howto = bfd_reloc_type_lookup (stdoutput, BFD_RELOC_16);
>  	    }
> -	  
> +
> +	  reloc = micromips_map_reloc (orig_reloc);
> 
> In the usual case, this calls micromips_map_reloc twice for the same thing.
> Seems better IMO to replace:
> 
> 	  /* In a compound relocation, it is the final (outermost)
> 	     operator that determines the relocated field.  */
> 	  for (i = 1; i < 3; i++)
> 	    if (reloc_type[i] == BFD_RELOC_UNUSED)
> 	      break;
> 
> 	  howto = bfd_reloc_type_lookup (stdoutput, reloc_type[i - 1]);
> 
> with:
> 
> 	  bfd_reloc_code_real_type final_type[3];
> 
> 	  /* Perform any necessary conversion to microMIPS relocations
> 	     and find out how many relocations there actually are.  */
> 	  for (i = 0; i < 3 && reloc_type[i] != BFD_RELOC_UNUSED; i++)
> 	    final_type[i] = micromips_map_reloc (reloc_type[i]);
> 
> 	  /* In a compound relocation, it is the final (outermost)
> 	     operator that determines the relocated field.  */
> 	  howto = bfd_reloc_type_lookup (stdoutput, final_type[i - 1]);
> 
> Then use final_type instead of reloc_type.  OK with that change,
> otherwise please let me know.

 Done.

 I'm waiting for the patches you promised and will work on regenerating
changes described above against current trunk.

  Maciej