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 - gas] MIPS: fix problem with mips16 delay branchoptimization.


On Tue, 2005-09-06 at 11:56 +0100, Richard Sandiford wrote:
> David Ung <davidu@mips.com> writes:
> > On Mon, 2005-09-05 at 22:46 +0100, Richard Sandiford wrote:
> >> David Ung <davidu@mips.com> writes:
> >> > This patch fixes a problem for Mips16 delay branch optimization when
> >> > swapping instructions, but the instructions belongs to different
> >> > fragments.  This shows up in the GCC C regressions for mips16.
> >> 
> >> > *************** append_insn (struct mips_cl_insn *ip, ex
> >> > *** 2698,2706 ****
> >> >   	      struct mips_cl_insn delay = history[0];
> >> >   	      if (mips_opts.mips16)
> >> >   		{
> >> > ! 		  know (delay.frag == ip->frag);
> >> >   		  move_insn (ip, delay.frag, delay.where);
> >> > ! 		  move_insn (&delay, ip->frag, ip->where + insn_length (ip));
> >> >   		}
> >> >   	      else if (relaxed_branch)
> >> >   		{
> >> > --- 2698,2719 ----
> >> >   	      struct mips_cl_insn delay = history[0];
> >> >   	      if (mips_opts.mips16)
> >> >   		{
> >> > ! 		  if (delay.frag == ip->frag)
> >> > ! 		    {
> >> >   		      move_insn (ip, delay.frag, delay.where);
> >> > ! 		      move_insn (&delay, ip->frag, delay.where 
> >> > ! 				 + insn_length (ip));
> >> > ! 		    }
> >> > ! 		  else if (insn_length (ip) == insn_length (&delay))
> >> > ! 		    {
> >> > ! 		      move_insn (&delay, ip->frag, ip->where);
> >> > ! 		      move_insn (ip, history[0].frag, history[0].where);
> >> > ! 		    }
> >> > ! 		  else
> >> > ! 		    {
> >> > ! 		      add_fixed_insn (NOP_INSN);
> >> > ! 		      delay = *NOP_INSN;
> >> > ! 		    }
> >> >   		}
> >> >   	      else if (relaxed_branch)
> >> >   		{
> >> 
> >> Can you explain the patch in more detail?  Do you have a reduced testcase?
> >> 
> >> In theory, the code adding the instruction is supposed to guarantee
> >> that the frag has enough room to accomodate the branch:
> >> 
> >>       /* Make sure there is enough room to swap this instruction with
> >>          a following jump instruction.  */
> >>       frag_grow (6);
> >>       add_fixed_insn (ip);
> >> 
> >> Presumably this code isn't triggering, or something else has decided
> >> to close the frag between the previous instruction and the branch,
> >> even though there was enough room.  If the latter, then how does
> >> this happen?
> >
> > If I remember correctly.  The frag_grow statement will triggering a new
> > frag being created, hence the jump instruction gets added to new frag
> > while the delay instruction would still be in the last frag.
> 
> But the code quoted above is adding the delay instruction (the one
> which originally gets added before the branch).  It shouldn't be
> used for the branch itself.  Note that the condition guarding it is:
> 
>     else if (mips_opts.mips16
>              && ! ip->use_extend
>              && *reloc_type != BFD_RELOC_MIPS16_JMP)
>       {
> 
> The point of reserving 6 bytes is that:
> 
>     (a) we can only use 2-byte instructions to a fill a delay slot
>     (b) branches are at most 4 bytes
>     (c) adding the branch shouldn't grow the frag by more than 4 bytes
> 
> So if we reserve 6 bytes, the branch should get placed in the same
> frag as the delay slot.  That's the idea anyway.

yeah, but the jump instruction "jr" will do this code aswell.  
when it finds that it dosen't have 6 bytes, its going to create a new
fragment.  
Perhaps a better approach would be to extend the above check to handle
such cases.  Also something to think about is how the compat style "jrc"
would fit in though.  

> (Note that I'm not defending this scheme.  It dates from before my
> semi-recent append_insn changes.  But if that's the scheme we're
> using, I think we should at least be consistent.)
> 
> So, could I ask you to explain in more detail why this patch is right?
> (Not that you're obliged to, of course.  I'm not a maintainer and the
> patch has already been approved.)




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