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] MIPS: microMIPS ASE support


"Maciej W. Rozycki" <macro@codesourcery.com> writes:
>  Here's what I have come up with as a result of merging your changes into 
> my code base.  There are plenty of small changes, so I have decided not to 
> put too much effort into straightening them up (or not) lest you are 
> unhappy with the outcome anyway.  Instead, I'm giving you (and the others) 
> an opportunity to review my code in its current shape.

Thanks for doing it this way.

> On Tue, 26 Jul 2011, Maciej W. Rozycki wrote:
>>  There's a whole lot of important linker relaxation fixes that I reckon 
>> were not included in the original series plus several bug fixes.
>
>  I'll work on these fixes now -- there are quite a few and at least one 
> still requires some work not to be considered a dirty hack -- and will be 
> back with you shortly.

Thanks.  This is what I asked for in the first place: that anything we
hadn't caught should be dealt with as follow-ups, rather than folded
into the huge initial commit.  (For reference, even the .tar.bz2 was
over 280k.)  I still don't understand why you chose to ignore that.

The whole point of approving most of the submission with a few minor
changes (back in March) was that it would be quick to make those changes
and get the whole thing into CVS.  It would then be much easier for me
to review any further changes you wanted to make.  I also assumed that
it would be easier for you to submit them.

Anyway, on a more positive note, a lot of the hunks are OK to commit,
as follows:

> Index: binutils-fsf-trunk-quilt/gas/config/tc-mips.c
> ===================================================================
> --- binutils-fsf-trunk-quilt.orig/gas/config/tc-mips.c	2011-07-28 23:18:50.000000000 +0100
> +++ binutils-fsf-trunk-quilt/gas/config/tc-mips.c	2011-07-28 23:30:26.000000000 +0100
> @@ -483,7 +483,7 @@ static int mips_32bitmode = 0;
>     (strncmp (TARGET_CPU, "mips16", sizeof ("mips16") - 1) == 0		\
>      || strncmp (TARGET_CANONICAL, "mips-lsi-elf", sizeof ("mips-lsi-elf") - 1) == 0)
>  
> -/* Return true if the given CPU supports microMIPS.  */
> +/* Return true if the given CPU supports the microMIPS ASE.  */
>  #define CPU_HAS_MICROMIPS(cpu)	0
>  
>  /* True if CPU has a dror instruction.  */

OK

> @@ -2141,7 +2141,7 @@ reg_lookup (char **s, unsigned int types
>     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.  */
> +   are selected; they have to be listed separately if needed.  */
>  
>  static int
>  reglist_lookup (char **s, unsigned int types, unsigned int *reglistp)

OK

> @@ -2893,18 +2888,21 @@ relax_end (void)
>    mips_relax.sequence = 0;
>  }
>  
> -/* Return the mask of core registers that instruction IP may
> -   read or write.  */
> +/* Return the mask of core registers that IP reads or writes.  */
>  
>  static unsigned int
>  gpr_mod_mask (const struct mips_cl_insn *ip)
>  {
> -  unsigned long pinfo, pinfo2;
> +  unsigned long pinfo2;
>    unsigned int mask;
>  
>    mask = 0;
> -  pinfo = ip->insn_mo->pinfo;
>    pinfo2 = ip->insn_mo->pinfo2;
> +  if (!mips_opts.mips16)
> +    {
> +      if (pinfo2 & INSN2_MOD_SP)
> +	mask |= 1 << SP;
> +    }
>    if (mips_opts.micromips)
>      {
>        if (pinfo2 & INSN2_MOD_GPR_MB)
and:
> @@ -2934,8 +2932,6 @@ gpr_mod_mask (const struct mips_cl_insn 
>  	mask |= 1 << EXTRACT_OPERAND (1, MP, *ip);
>        if (pinfo2 & INSN2_MOD_GPR_MQ)
>  	mask |= 1 << micromips_to_32_reg_q_map[EXTRACT_OPERAND (1, MQ, *ip)];
> -      if (pinfo2 & INSN2_MOD_SP)
> -	mask |= 1 << SP;
>      }
>    return mask;
>  }

*sigh*  Have I introduced another write to an unused variable? :-(
I really should use a more modern compiler.

The removal of pinfo is OK, thanks.  The rest of it doesn't look like
an improvement.

> @@ -2969,27 +2965,20 @@ gpr_read_mask (const struct mips_cl_insn
>        if (pinfo & MIPS16_INSN_READ_GPR_X)
>  	mask |= 1 << MIPS16_EXTRACT_OPERAND (REGR32, *ip);
>      }
> -  else if (mips_opts.micromips)
> -    {
> -      if (pinfo & INSN_READ_GPR_T)
> -	mask |= 1 << EXTRACT_OPERAND (1, RT, *ip);
> -      if (pinfo & INSN_READ_GPR_S)
> -	mask |= 1 << EXTRACT_OPERAND (1, RS, *ip);
> -      if (pinfo2 & INSN2_READ_GPR_31)
> -	mask |= 1 << RA;
> -      if (pinfo2 & INSN2_READ_GP)
> -	mask |= 1 << GP;
> -    }
>    else
>      {
>        if (pinfo2 & INSN2_READ_GPR_D)
> -	mask |= 1 << EXTRACT_OPERAND (0, RD, *ip);
> +	mask |= 1 << EXTRACT_OPERAND (mips_opts.micromips, RD, *ip);
>        if (pinfo & INSN_READ_GPR_T)
> -	mask |= 1 << EXTRACT_OPERAND (0, RT, *ip);
> +	mask |= 1 << EXTRACT_OPERAND (mips_opts.micromips, RT, *ip);
>        if (pinfo & INSN_READ_GPR_S)
> -	mask |= 1 << EXTRACT_OPERAND (0, RS, *ip);
> +	mask |= 1 << EXTRACT_OPERAND (mips_opts.micromips, RS, *ip);
> +      if (pinfo2 & INSN2_READ_GP)
> +	mask |= 1 << GP;
> +      if (pinfo2 & INSN2_READ_GPR_31)
> +	mask |= 1 << RA;
>        if (pinfo2 & INSN2_READ_GPR_Z)
> -	mask |= 1 << EXTRACT_OPERAND (0, RZ, *ip);
> +	mask |= 1 << EXTRACT_OPERAND (mips_opts.micromips, RZ, *ip);
>      }
>    /* Don't include register 0.  */
>    return mask & ~1;

OK

> @@ -3023,23 +3012,14 @@ gpr_write_mask (const struct mips_cl_ins
>        if (pinfo & MIPS16_INSN_WRITE_GPR_Y)
>  	mask |= 1 << MIPS16OP_EXTRACT_REG32R (ip->insn_opcode);
>      }
> -  else if (mips_opts.micromips)
> -    {
> -      if (pinfo & INSN_WRITE_GPR_D)
> -	mask |= 1 << EXTRACT_OPERAND (1, RD, *ip);
> -      if (pinfo & INSN_WRITE_GPR_T)
> -	mask |= 1 << EXTRACT_OPERAND (1, RT, *ip);
> -      if (pinfo2 & INSN2_WRITE_GPR_S)
> -	mask |= 1 << EXTRACT_OPERAND (1, RS, *ip);
> -      if (pinfo & INSN_WRITE_GPR_31)
> -	mask |= 1 << RA;
> -    }
>    else
>      {
>        if (pinfo & INSN_WRITE_GPR_D)
>  	mask |= 1 << EXTRACT_OPERAND (mips_opts.micromips, RD, *ip);
>        if (pinfo & INSN_WRITE_GPR_T)
>  	mask |= 1 << EXTRACT_OPERAND (mips_opts.micromips, RT, *ip);
> +      if (pinfo2 & INSN2_WRITE_GPR_S)
> +	mask |= 1 << EXTRACT_OPERAND (mips_opts.micromips, RS, *ip);
>        if (pinfo & INSN_WRITE_GPR_31)
>  	mask |= 1 << RA;
>        if (pinfo2 & INSN2_WRITE_GPR_Z)

OK

> @@ -3060,19 +3040,10 @@ fpr_read_mask (const struct mips_cl_insn
>    mask = 0;
>    pinfo = ip->insn_mo->pinfo;
>    pinfo2 = ip->insn_mo->pinfo2;
> -  if (mips_opts.micromips)
> +  if (!mips_opts.mips16)
>      {
>        if (pinfo2 & INSN2_READ_FPR_D)
> -	mask |= 1 << EXTRACT_OPERAND (1, FD, *ip);
> -      if (pinfo & INSN_READ_FPR_S)
> -	mask |= 1 << EXTRACT_OPERAND (1, FS, *ip);
> -      if (pinfo & INSN_READ_FPR_T)
> -	mask |= 1 << EXTRACT_OPERAND (1, FT, *ip);
> -      if (pinfo & INSN_READ_FPR_R)
> -	mask |= 1 << EXTRACT_OPERAND (1, FR, *ip);
> -    }
> -  else if (!mips_opts.mips16)
> -    {
> +	mask |= 1 << EXTRACT_OPERAND (mips_opts.micromips, FD, *ip);
>        if (pinfo & INSN_READ_FPR_S)
>  	mask |= 1 << EXTRACT_OPERAND (mips_opts.micromips, FS, *ip);
>        if (pinfo & INSN_READ_FPR_T)

OK

> @@ -3100,16 +3071,7 @@ fpr_write_mask (const struct mips_cl_ins
>    mask = 0;
>    pinfo = ip->insn_mo->pinfo;
>    pinfo2 = ip->insn_mo->pinfo2;
> -  if (mips_opts.micromips)
> -    {
> -      if (pinfo2 & INSN_WRITE_FPR_D)
> -	mask |= 1 << EXTRACT_OPERAND (1, FD, *ip);
> -      if (pinfo & INSN_WRITE_FPR_S)
> -	mask |= 1 << EXTRACT_OPERAND (1, FS, *ip);
> -      if (pinfo & INSN_WRITE_FPR_T)
> -	mask |= 1 << EXTRACT_OPERAND (1, FT, *ip);
> -    }
> -  else if (!mips_opts.mips16)
> +  if (!mips_opts.mips16)
>      {
>        if (pinfo & INSN_WRITE_FPR_D)
>  	mask |= 1 << EXTRACT_OPERAND (mips_opts.micromips, FD, *ip);

OK

> @@ -3720,8 +3683,9 @@ can_swap_branch_p (struct mips_cl_insn *
>      return FALSE;
>  
>    /* If the previous instruction is in a variant frag other than this
> -     branch's one, we cannot do the swap.  This does not apply to the
> -     mips16, which uses variant frags for different purposes.  */
> +     branch's one, we cannot do the swap.  This does not apply to
> +     MIPS16/microMIPS code, which uses variant frags for different
> +     purposes.  */
>    if (!HAVE_CODE_COMPRESSION
>        && history[0].frag
>        && history[0].frag->fr_type == rs_machine_dependent)

OK

>        ip->fixp[0] = fix_new_exp (ip->frag, ip->where,
>  				 bfd_get_reloc_size (howto),
>  				 address_expr,
> -				 howto->pc_relative, final_type[0]);
> +				 howto0 && howto0->pc_relative,
> +				 final_type[0]);

The howto0 variable and null check are OK.  Please leave the rest as-is.

> Index: binutils-fsf-trunk-quilt/bfd/elfxx-mips.c
> ===================================================================
> --- binutils-fsf-trunk-quilt.orig/bfd/elfxx-mips.c	2011-07-28 23:18:50.000000000 +0100
> +++ binutils-fsf-trunk-quilt/bfd/elfxx-mips.c	2011-07-28 23:30:26.000000000 +0100
> @@ -11910,8 +11910,7 @@ mips_elf_relax_delete_bytes (bfd *abfd,
>    symtab_hdr = &elf_tdata (abfd)->symtab_hdr;
>    isym = (Elf_Internal_Sym *) symtab_hdr->contents;
>    for (isymend = isym + symtab_hdr->sh_info; isym < isymend; isym++)
> -    if (isym->st_shndx == sec_shndx
> -	&& isym->st_value > addr)
> +    if (isym->st_shndx == sec_shndx && isym->st_value > addr)
>        isym->st_value -= count;
>  
>    /* Now adjust the global symbols defined in this section.  */

OK

> @@ -11989,7 +11987,7 @@ static const struct opcode_descriptor b_
>    { /* "b",	"mD",		*/ 0xcc00,     0xfc00 };
>  
>  static const struct opcode_descriptor bz_insn_16 =
> -  { /* "b(eq|ne)z", "md,mE",	*/ 0x8c00,     0xac00 };
> +  { /* "b(eq|ne)z", "md,mE",	*/ 0x8c00,     0xdc00 };
>  
>  
>  /* 32-bit and 16-bit branch EQ and NE zero.  */

I don't recall discussing this.  Is it a separate thing you noticed?
OK regardless.

> @@ -12241,7 +12239,7 @@ check_br16 (bfd *abfd, bfd_byte *ptr, un
>  /* If PTR points to a 32-bit branch or jump that doesn't fiddle with REG,
>     then return TRUE, otherwise FALSE.  */
>  
> -static int
> +static bfd_boolean
>  check_br32 (bfd *abfd, bfd_byte *ptr, unsigned long reg)
>  {
>    unsigned long opcode;

Likewise.

> @@ -12361,6 +12364,7 @@ _bfd_mips_elf_relax_section (bfd *abfd, 
>  	  else if (!bfd_malloc_and_get_section (abfd, sec, &contents))
>  	    goto error_return;
>  	}
> +      ptr = contents + irel->r_offset;
>  
>        /* Read this BFD's local symbols if we haven't done so already.  */
>        if (isymbuf == NULL && symtab_hdr->sh_info != 0)

OK

> @@ -12432,8 +12436,8 @@ _bfd_mips_elf_relax_section (bfd *abfd, 
>        if (irel->r_offset + 4 > sec->size)
>  	continue;
>  
> -      opcode  = bfd_get_16 (abfd, contents + irel->r_offset    ) << 16;
> -      opcode |= bfd_get_16 (abfd, contents + irel->r_offset + 2);
> +      opcode  = bfd_get_16 (abfd, ptr    ) << 16;
> +      opcode |= bfd_get_16 (abfd, ptr + 2);
>  
>        /* This is the pc-relative distance from the instruction the
>           relocation is applied to, to the symbol referred.  */

OK

> @@ -12452,6 +12456,7 @@ _bfd_mips_elf_relax_section (bfd *abfd, 
>           out the offset).  */
>        if (r_type == R_MICROMIPS_HI16 && MATCH (opcode, lui_insn))
>  	{
> +	  bfd_boolean bzc = FALSE;
>  	  unsigned long nextopc;
>  	  unsigned long reg;
>  	  bfd_vma offset;
and:
> @@ -12475,19 +12480,19 @@ _bfd_mips_elf_relax_section (bfd *abfd, 
>  	      && ELF32_R_SYM (irel[2].r_info) == r_symndx)
>  	    continue;
>  
> -	  /* See if the LUI instruction *might* be in a branch delay slot.  */
> +	  /* See if the LUI instruction *might* be in a branch delay slot.
> +	     We check whether what looks like a 16-bit branch or jump is
> +	     actually an immediate argument to a compact branch, and let
> +	     it through if so.  */
>  	  if (irel->r_offset >= 2
> -	      && check_br16_dslot (abfd, contents + irel->r_offset - 2) > 0
> +	      && check_br16_dslot (abfd, ptr - 2)
>  	      && !(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)))
> +		   && (bzc = check_bzc (abfd, ptr - 4, irel->r_offset - 4,
> +					internal_relocs, irelend))))
>  	    continue;
>  	  if (irel->r_offset >= 4
> -	      && check_br32_dslot (abfd, contents + irel->r_offset - 4) > 0)
> +	      && !bzc
> +	      && check_br32_dslot (abfd, ptr - 4))
>  	    continue;
>  
>  	  reg = OP32_SREG (opcode);

As regards what you were saying about straightening the patch up:
I think this is the one bit that ought to be split out separately
and treated as a separate patch.  The bzc variable and !bzc check
look suspiciously redundant.

The use of "ptr" is OK now though.

> @@ -12502,11 +12507,11 @@ _bfd_mips_elf_relax_section (bfd *abfd, 
>  	    case 0:
>  	      break;
>  	    case 2:
> -	      if (check_br16 (abfd, contents + irel->r_offset + 4, reg))
> +	      if (check_br16 (abfd, ptr + 4, reg))
>  		break;
>  	      continue;
>  	    case 4:
> -	      if (check_br32 (abfd, contents + irel->r_offset + 4, reg))
> +	      if (check_br32 (abfd, ptr + 4, reg))
>  		break;
>  	      continue;
>  	    default:

OK

> @@ -12581,8 +12586,7 @@ _bfd_mips_elf_relax_section (bfd *abfd, 
>  	       && irel->r_offset + 5 < sec->size
>  	       && ((fndopc = find_match (opcode, bz_rs_insns_32)) >= 0
>  		   || (fndopc = find_match (opcode, bz_rt_insns_32)) >= 0)
> -	       && MATCH (bfd_get_16 (abfd, contents + irel->r_offset + 4),
> -			 nop_insn_16))
> +	       && MATCH (bfd_get_16 (abfd, ptr + 4), nop_insn_16))
>  	{
>  	  unsigned long reg;
>  

OK

> @@ -12593,10 +12597,8 @@ _bfd_mips_elf_relax_section (bfd *abfd, 
>  		    | BZC32_REG_FIELD (reg)
>  		    | (opcode & 0xffff));		/* Addend value.  */
>  
> -	  bfd_put_16 (abfd, (opcode >> 16) & 0xffff,
> -		      contents + irel->r_offset);
> -	  bfd_put_16 (abfd,  opcode        & 0xffff,
> -		      contents + irel->r_offset + 2);
> +	  bfd_put_16 (abfd, (opcode >> 16) & 0xffff, ptr);
> +	  bfd_put_16 (abfd,  opcode        & 0xffff, ptr + 2);
>  
>  	  /* Delete the 16-bit delay slot NOP: two bytes from
>  	     irel->offset + 4.  */

OK

> @@ -12617,7 +12619,7 @@ _bfd_mips_elf_relax_section (bfd *abfd, 
>  	  bfd_put_16 (abfd,
>  		      (b_insn_16.match
>  		       | (opcode & 0x3ff)),		/* Addend value.  */
> -		      contents + irel->r_offset);
> +		      ptr);
>  
>  	  /* Delete 2 bytes from irel->r_offset + 2.  */
>  	  delcnt = 2;

OK

> @@ -12645,7 +12647,7 @@ _bfd_mips_elf_relax_section (bfd *abfd, 
>  		      (bz_insns_16[fndopc].match
>  		       | BZ16_REG_FIELD (reg)
>  		       | (opcode & 0x7f)),		/* Addend value.  */
> -		      contents + irel->r_offset);
> +		      ptr);
>  
>  	  /* Delete 2 bytes from irel->r_offset + 2.  */
>  	  delcnt = 2;

OK

> @@ -12661,14 +12663,13 @@ _bfd_mips_elf_relax_section (bfd *abfd, 
>  	  unsigned long n32opc;
>  	  bfd_boolean relaxed = FALSE;
>  
> -	  n32opc  = bfd_get_16 (abfd, contents + irel->r_offset + 4) << 16;
> -	  n32opc |= bfd_get_16 (abfd, contents + irel->r_offset + 6);
> +	  n32opc  = bfd_get_16 (abfd, ptr + 4) << 16;
> +	  n32opc |= bfd_get_16 (abfd, ptr + 6);
>  
>  	  if (MATCH (n32opc, nop_insn_32))
>  	    {
>  	      /* Replace delay slot 32-bit NOP with a 16-bit NOP.  */
> -	      bfd_put_16 (abfd, nop_insn_16.match,
> -			  contents + irel->r_offset + 4);
> +	      bfd_put_16 (abfd, nop_insn_16.match, ptr + 4);
>  
>  	      relaxed = TRUE;
>  	    }

OK

> @@ -12679,7 +12680,7 @@ _bfd_mips_elf_relax_section (bfd *abfd, 
>  			  (move_insn_16.match
>  			   | MOVE16_RD_FIELD (MOVE32_RD (n32opc))
>  			   | MOVE16_RS_FIELD (MOVE32_RS (n32opc))),
> -			  contents + irel->r_offset + 4);
> +			  ptr + 4);
>  
>  	      relaxed = TRUE;
>  	    }

OK

> @@ -12691,9 +12692,9 @@ _bfd_mips_elf_relax_section (bfd *abfd, 
>  	      /* JAL with 32-bit delay slot that is changed to a JALS
>  	         with 16-bit delay slot.  */
>  	      bfd_put_16 (abfd, (jal_insn_32_bd16.match >> 16) & 0xffff,
> -			  contents + irel->r_offset);
> +			  ptr);
>  	      bfd_put_16 (abfd,  jal_insn_32_bd16.match        & 0xffff,
> -			  contents + irel->r_offset + 2);
> +			  ptr + 2);
>  
>  	      /* Delete 2 bytes from irel->r_offset + 6.  */
>  	      delcnt = 2;

OK

> Index: binutils-fsf-trunk-quilt/include/opcode/mips.h
> ===================================================================
> --- binutils-fsf-trunk-quilt.orig/include/opcode/mips.h	2011-07-28 23:29:25.000000000 +0100
> +++ binutils-fsf-trunk-quilt/include/opcode/mips.h	2011-07-28 23:30:26.000000000 +0100
> @@ -1332,13 +1332,10 @@ extern int bfd_mips_num_opcodes;
>  extern const struct mips_opcode mips16_opcodes[];
>  extern const int bfd_mips16_num_opcodes;
>  
> -/* These are the bitmasks and shift counts used for the different
> -   fields in the instruction formats.  Other than MAJOR, no masks are
> -   provided for the fixed portions of an instruction, since they are
> -   not needed.  */
> +/* These are the bit masks and shift counts used for the different fields
> +   in the microMIPS instruction formats.  No masks are provided for the
> +   fixed portions of an instruction, since they are not needed.  */
>  
> -#define MICROMIPSOP_MASK_MAJOR		0x3f
> -#define MICROMIPSOP_SH_MAJOR		26
>  #define MICROMIPSOP_MASK_IMMEDIATE	0xffff
>  #define MICROMIPSOP_SH_IMMEDIATE	0
>  #define MICROMIPSOP_MASK_DELTA		0xffff

OK

Thanks,
Richard


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