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


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.

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

Richard


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