Re: [PATCH] MIPS: microMIPS ASE support

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

On Fri, 29 Jul 2011, Richard Sandiford 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.

 Well, I believed this was really what I did -- with all these 
binutils-ld-lib.diff, binutils-umips-fix.diff, 
binutils-umips-opcode-trap.diff, binutils-umips-relax16.diff, 
binutils-umips-fix-reloc.diff patches (and all the outstanding ones that 
add up to six right now, but will probably become seven or eight) -- but 
if that wasn't what you intended, then I apologise for misunderstanding.  

> 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.

 The problem was I was pulled off to another project as a matter of 
emergency -- apart from the update to take your recent register use 
tracking changes into account I did last week, I haven't really touched 
the pieces for a couple of months now.  Sorry about that.

 I'll go on to explain the bits you had anything but "OK" to say about, 
commit the bits you are happy with and then we can continue with the rest.

> > @@ -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.

 For the sake of consistency I put all the pinfo/2 flag interpretation 
that does not involve microMIPS-specific structures (i.e. 
micromips_to_32_reg_*_map) into (!mips_opts.mips16) rather than 
(mips_opts.micromips) condition blocks under the assumption we don't have 
to imply their microMIPS-ness here.

 Obviously this includes the INSN2_MOD_SP flag as it's self-contained in 
that it determines the register accessed itself, though it probably has 
little chance to be useful for non-microMIPS code even with possible 
future extensions to the base architecture.  Therefore I won't insist on 
this change even though it's consistent with the usage of INSN2_READ_GP 
and INSN2_READ_GPR_31 flags in gpr_read_mask().

> >        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.

 There's no need to reiterate checking for non-BFD_RELOC_UNUSED that's 
been already done earlier on.  I'll split it off for further 
consideration.

> > @@ -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.

 Well, there's nothing to discuss here -- it's a plain bug, the old mask 
is wrong and there was no need to handle it separately -- in fact the 
opposite was the case as it makes no sense to check in known-buggy code.

 I'll split it off and commit separately for clarity.

> > @@ -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.

 I'll split it off too.

> > @@ -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.

 It's a small optimisation -- if we've established for sure the 
instruction before is a compact branch, then there's no need to go through 
the hoops and check if it's an ordinary branch too -- which it will never 
be.

 I'll split this change off of course.

 Thanks for the quick review.

  Maciej