[patch - gas] MIPS: fix problem with mips16 delay branch optimization.

classic Classic list List threaded Threaded
13 messages Options
Reply | Threaded
Open this post in threaded view
|

[patch - gas] MIPS: fix problem with mips16 delay branch optimization.

David Ung

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.

Schedule of variations:
    mips-sim-idt32//-mips32/-mips16
...
FAIL: gcc.dg/compat/scalar-return-3 c_compat_x_tst.o-c_compat_y_tst.o execute
FAIL: tmpdir-gcc.dg-struct-layout-1/t001 c_compat_x_tst.o-c_compat_y_tst.o execute
FAIL: tmpdir-gcc.dg-struct-layout-1/t002 c_compat_x_tst.o-c_compat_y_tst.o execute
FAIL: tmpdir-gcc.dg-struct-layout-1/t004 c_compat_x_tst.o-c_compat_y_tst.o execute
FAIL: tmpdir-gcc.dg-struct-layout-1/t005 c_compat_x_tst.o-c_compat_y_tst.o execute
FAIL: tmpdir-gcc.dg-struct-layout-1/t024 c_compat_x_tst.o-c_compat_y_tst.o execute
FAIL: tmpdir-gcc.dg-struct-layout-1/t026 c_compat_x_tst.o-c_compat_y_tst.o execute
...
                === gcc Summary ===

# of expected passes            35866
# of unexpected failures        14
# of unexpected successes       1
# of expected failures          74
# of untested testcases         28
# of unsupported tests          532
/local/fsf-mainline/gcc/xgcc  version 4.1.0 20050822 (experimental)


after applying the patch, I now get

                === gcc Summary ===

# of expected passes            35911
# of unexpected failures        7
# of unexpected successes       1
# of expected failures          74
# of untested testcases         28
# of unsupported tests          534
/local/fsf-mainline/gcc/xgcc  version 4.1.0 20050826 (experimental)

ok for mainline?

David.


2005-09-01  David Ung  <[hidden email]>

        * config/tc-mips.c (append_insn): Correctly handle mips16 case
        when the frags are different for the 2 instructions we want to
        swap.  If the lengths of the 2 instructions are not the same, we
        won't do the swap but emit an nop.

Index: config/tc-mips.c
===================================================================
RCS file: /cvs/src/src/gas/config/tc-mips.c,v
retrieving revision 1.318
diff -c -p -b -r1.318 tc-mips.c
*** config/tc-mips.c 25 Aug 2005 18:17:36 -0000 1.318
--- config/tc-mips.c 1 Sep 2005 14:14:56 -0000
*************** 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)
  {

Reply | Threaded
Open this post in threaded view
|

Re: [patch - gas] MIPS: fix problem with mips16 delay branch optimization.

Thiemo Seufer
David Ung wrote:
[snip]
> 2005-09-01  David Ung  <[hidden email]>
>
> * config/tc-mips.c (append_insn): Correctly handle mips16 case
> when the frags are different for the 2 instructions we want to
> swap.  If the lengths of the 2 instructions are not the same, we
> won't do the swap but emit an nop.

Ok.


Thiemo
Reply | Threaded
Open this post in threaded view
|

Re: [patch - gas] MIPS: fix problem with mips16 delay branch optimization.

Richard Sandiford-3
In reply to this post by David Ung
David Ung <[hidden email]> 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?

Richard
Reply | Threaded
Open this post in threaded view
|

Re: [patch - gas] MIPS: fix problem with mips16 delay branch optimization.

David Ung
On Mon, 2005-09-05 at 22:46 +0100, Richard Sandiford wrote:

> David Ung <[hidden email]> 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.

You need to create enough instructions for the above case to happen and
I could reproduce it by compiling the c_compat_x_tst.c file from the GCC
scalar-return-3 regression suite.  Turning on just these tests.

c_compat_x_tst.c:

T(ci, _Complex int, CINT (2, 3))
T(cl, _Complex long, CINT (3, 4))

from the c_compat_x_tst.s file, you'll get thses functions:
checkci, testitci, checkcl, testitcl and scalar_return_3_x.
And it's the last "jr" from "testitcl" that shows the problem.

function testitcl:
...
.L36:
        move    $sp,$17
        lw      $7,88($sp)
        lw      $17,84($sp)
        lw      $16,80($sp)
        addiu   $sp,$sp,96
        jr      $7
        .align  2
.L37:
        .word   g01cl
.L38:
        .word   g02cl
...

after assembling with gas, objdump of fuction testitcl:

     ....
     ca4:       65b9            move    sp,s1
     ca6:       9716            lw      a3,88(sp)
     ca8:       9115            lw      s1,84(sp)
     caa:       9014            lw      s0,80(sp)
     cac:       ef00            jr      a3
     cae:       ef00            jr      a3
     cb0:       0000            addiu   s0,sp,0
                        cb0: R_MIPS_32  g01cl
        ...
                        cb4: R_MIPS_32  g02cl


David.

Reply | Threaded
Open this post in threaded view
|

Re: [patch - gas] MIPS: fix problem with mips16 delay branch optimization.

Richard Sandiford-3
David Ung <[hidden email]> writes:

> On Mon, 2005-09-05 at 22:46 +0100, Richard Sandiford wrote:
>> David Ung <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: [patch - gas] MIPS: fix problem with mips16 delay branch optimization.

Maciej W. Rozycki
On Tue, 6 Sep 2005, Richard Sandiford wrote:

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

 IIRC, for code as is there is also a nasty interaction with DWARF-2
information for these swaps and for extended branches it's hopeless as the
result is "gdb" tends to place breakpoints in the middle, i.e. starting
from byte #2 rather than the beginning, of the swapped branch.  Which
breaks debugging badly -- you usually get SIGILL if you hit such a
breakpoint or code actually executed may be different from what you'd
expect.  For 2-byte branches at least it works somehow, but
single-stepping at the source level is inexact, so execution may run away
at times.

 Regrettably I have failed finding an easy way of updating DWARF-2 records
at the time the swap happens.

  Maciej
Reply | Threaded
Open this post in threaded view
|

Re: [patch - gas] MIPS: fix problem with mips16 delay branch optimization.

David Ung
In reply to this post by Richard Sandiford-3
On Tue, 2005-09-06 at 11:56 +0100, Richard Sandiford wrote:

> David Ung <[hidden email]> writes:
> > On Mon, 2005-09-05 at 22:46 +0100, Richard Sandiford wrote:
> >> David Ung <[hidden email]> 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.)



Reply | Threaded
Open this post in threaded view
|

Re: [patch - gas] MIPS: fix problem with mips16 delay branch optimization.

Richard Sandiford-3
David Ung <[hidden email]> writes:

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

Oh, so it was specifically "jr" that was the problem?  That's the
extra detail I was hoping for, thanks.

> Perhaps a better approach would be to extend the above check
> to handle such cases.

FWIW, I agree that that would be better.  The reason I didn't like your
original patch was that it made the proximity of the insn to the end
of the frag a factor in deciding whether to fill a branch delay slot.
That seems like a bad design IMO.  It would be very confusing for a
user who's looking at disassembly code if, for example, inserting a
nop at some seemingly-unrelated point in the file made the difference
between gas filling or not filling a slot.  There's no way we should
expect the user to understand stuff like gas frags.

At first glance, it looks like it should just be a case of guarding
"frag_grow (6);" with "(pinfo & INSN_UNCOND_BRANCH_DELAY) == 0".

Richard
Reply | Threaded
Open this post in threaded view
|

Re: [patch - gas] MIPS: fix problem with mips16 delay branch optimization.

David Ung
On Tue, 2005-09-06 at 13:48 +0100, Richard Sandiford wrote:

> David Ung <[hidden email]> writes:
> >> 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.
>
> Oh, so it was specifically "jr" that was the problem?  That's the
> extra detail I was hoping for, thanks.

yeah.  I thought I've mention that "jr" was creating the new fragment,
but obviously it wasn't clear enough.

>
> > Perhaps a better approach would be to extend the above check
> > to handle such cases.
>
> FWIW, I agree that that would be better.  The reason I didn't like your
> original patch was that it made the proximity of the insn to the end
> of the frag a factor in deciding whether to fill a branch delay slot.
> That seems like a bad design IMO.  It would be very confusing for a
> user who's looking at disassembly code if, for example, inserting a
> nop at some seemingly-unrelated point in the file made the difference
> between gas filling or not filling a slot.  There's no way we should
> expect the user to understand stuff like gas frags.
>
> At first glance, it looks like it should just be a case of guarding
> "frag_grow (6);" with "(pinfo & INSN_UNCOND_BRANCH_DELAY) == 0".

yeah, something like that might do it.

  else if (mips_opts.mips16
           && ! ip->use_extend
+   && (pinfo & INSN_UNCOND_BRANCH_DELAY) == 0
           && *reloc_type != BFD_RELOC_MIPS16_JMP)

I'll run the regressions and see.

David.

Reply | Threaded
Open this post in threaded view
|

Re: [patch - gas] MIPS: fix problem with mips16 delay branch optimization.

Richard Sandiford-3
David Ung <[hidden email]> writes:
>> Oh, so it was specifically "jr" that was the problem?  That's the
>> extra detail I was hoping for, thanks.
>
> yeah.  I thought I've mention that "jr" was creating the new fragment,
> but obviously it wasn't clear enough.

I missed the reference to "jr" in your first followup.  Sorry about that.

>> At first glance, it looks like it should just be a case of guarding
>> "frag_grow (6);" with "(pinfo & INSN_UNCOND_BRANCH_DELAY) == 0".
>
> yeah, something like that might do it.
>
>   else if (mips_opts.mips16
>   && ! ip->use_extend
> +   && (pinfo & INSN_UNCOND_BRANCH_DELAY) == 0
>   && *reloc_type != BFD_RELOC_MIPS16_JMP)
>
> I'll run the regressions and see.

No, that won't work.  The next arm of the if branch doesn't expect to
see normal non-extended instructions.  I really did mean what I said
above about guarding the call to frag_grow.

Richard
Reply | Threaded
Open this post in threaded view
|

Re: [patch - gas] MIPS: fix problem with mips16 delay branch optimization.

David Ung
> >> At first glance, it looks like it should just be a case of guarding
> >> "frag_grow (6);" with "(pinfo & INSN_UNCOND_BRANCH_DELAY) == 0".
> >
> > yeah, something like that might do it.
> >
> >   else if (mips_opts.mips16
> >   && ! ip->use_extend
> > +   && (pinfo & INSN_UNCOND_BRANCH_DELAY) == 0
> >   && *reloc_type != BFD_RELOC_MIPS16_JMP)
> >
> > I'll run the regressions and see.
>
> No, that won't work.  The next arm of the if branch doesn't expect to
> see normal non-extended instructions.  I really did mean what I said
> above about guarding the call to frag_grow.

ok, corrected.

  else if (mips_opts.mips16
           && ! ip->use_extend
           && *reloc_type != BFD_RELOC_MIPS16_JMP)
    {    
      if ((pinfo & INSN_UNCOND_BRANCH_DELAY) == 0)
        /* Make sure there is enough room to swap this instruction with
           a following jump instruction.  */
        frag_grow (6);
      add_fixed_insn (ip);

David

Reply | Threaded
Open this post in threaded view
|

Re: [patch - gas] MIPS: fix problem with mips16 delay branch optimization.

Richard Sandiford-3
David Ung <[hidden email]> writes:
>   else if (mips_opts.mips16
>   && ! ip->use_extend
>   && *reloc_type != BFD_RELOC_MIPS16_JMP)
>     {    
>       if ((pinfo & INSN_UNCOND_BRANCH_DELAY) == 0)
> /* Make sure there is enough room to swap this instruction with
>   a following jump instruction.  */
> frag_grow (6);
>       add_fixed_insn (ip);

Looks good to me, thanks.  [ And sorry again for not reading your earlier
message properly.  It couldn't really have been clearer.  I think I somehow
failed to read it all first time round. ;( ]

Richard
Reply | Threaded
Open this post in threaded view
|

Re: [patch - gas] MIPS: fix problem with mips16 delay branch optimization.

David Ung

On Tue, 2005-09-06 at 17:47 +0100, Richard Sandiford wrote:

> David Ung <[hidden email]> writes:
> >   else if (mips_opts.mips16
> >   && ! ip->use_extend
> >   && *reloc_type != BFD_RELOC_MIPS16_JMP)
> >     {    
> >       if ((pinfo & INSN_UNCOND_BRANCH_DELAY) == 0)
> > /* Make sure there is enough room to swap this instruction with
> >   a following jump instruction.  */
> > frag_grow (6);
> >       add_fixed_insn (ip);
>
> Looks good to me, thanks.  [ And sorry again for not reading your earlier
> message properly.  It couldn't really have been clearer.  I think I somehow
> failed to read it all first time round. ;( ]
>
> Richard
>

ok.  Here is the new patch then.
re-ran regressions and was ok.   now commited to cvs.

David.


2005-09-07  David Ung  <[hidden email]>

        * config/tc-mips.c (append_insn): Undo last change.  Instead add
        guard to suppress calling frag_grow if the current instruction is
        one that allows a delay slot.

Index: config/tc-mips.c
===================================================================
RCS file: /cvs/src/src/gas/config/tc-mips.c,v
retrieving revision 1.320
diff -c -p -b -r1.320 tc-mips.c
*** config/tc-mips.c 6 Sep 2005 18:53:02 -0000 1.320
--- config/tc-mips.c 7 Sep 2005 11:33:49 -0000
*************** append_insn (struct mips_cl_insn *ip, ex
*** 2313,2318 ****
--- 2313,2319 ----
    && ! ip->use_extend
    && *reloc_type != BFD_RELOC_MIPS16_JMP)
      {    
+       if ((pinfo & INSN_UNCOND_BRANCH_DELAY) == 0)
  /* Make sure there is enough room to swap this instruction with
    a following jump instruction.  */
  frag_grow (6);
*************** append_insn (struct mips_cl_insn *ip, ex
*** 2707,2728 ****
       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)
  {
--- 2708,2716 ----
       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)
  {