* [patch - gas] MIPS: fix problem with mips16 delay branch optimization.
@ 2005-09-01 14:30 David Ung
2005-09-01 15:48 ` Thiemo Seufer
2005-09-06 0:57 ` Richard Sandiford
0 siblings, 2 replies; 13+ messages in thread
From: David Ung @ 2005-09-01 14:30 UTC (permalink / raw)
To: binutils
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 <davidu@mips.com>
* 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)
{
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [patch - gas] MIPS: fix problem with mips16 delay branch optimization.
2005-09-01 14:30 [patch - gas] MIPS: fix problem with mips16 delay branch optimization David Ung
@ 2005-09-01 15:48 ` Thiemo Seufer
2005-09-06 0:57 ` Richard Sandiford
1 sibling, 0 replies; 13+ messages in thread
From: Thiemo Seufer @ 2005-09-01 15:48 UTC (permalink / raw)
To: David Ung; +Cc: binutils
David Ung wrote:
[snip]
> 2005-09-01 David Ung <davidu@mips.com>
>
> * 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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [patch - gas] MIPS: fix problem with mips16 delay branch optimization.
2005-09-01 14:30 [patch - gas] MIPS: fix problem with mips16 delay branch optimization David Ung
2005-09-01 15:48 ` Thiemo Seufer
@ 2005-09-06 0:57 ` Richard Sandiford
2005-09-06 11:45 ` David Ung
1 sibling, 1 reply; 13+ messages in thread
From: Richard Sandiford @ 2005-09-06 0:57 UTC (permalink / raw)
To: David Ung; +Cc: binutils
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?
Richard
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [patch - gas] MIPS: fix problem with mips16 delay branch optimization.
2005-09-06 0:57 ` Richard Sandiford
@ 2005-09-06 11:45 ` David Ung
2005-09-06 12:15 ` Richard Sandiford
0 siblings, 1 reply; 13+ messages in thread
From: David Ung @ 2005-09-06 11:45 UTC (permalink / raw)
To: Richard Sandiford; +Cc: binutils
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.
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.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [patch - gas] MIPS: fix problem with mips16 delay branch optimization.
2005-09-06 11:45 ` David Ung
@ 2005-09-06 12:15 ` Richard Sandiford
2005-09-06 12:48 ` Maciej W. Rozycki
2005-09-06 15:50 ` David Ung
0 siblings, 2 replies; 13+ messages in thread
From: Richard Sandiford @ 2005-09-06 12:15 UTC (permalink / raw)
To: David Ung; +Cc: binutils
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [patch - gas] MIPS: fix problem with mips16 delay branch optimization.
2005-09-06 12:15 ` Richard Sandiford
@ 2005-09-06 12:48 ` Maciej W. Rozycki
2005-09-06 15:50 ` David Ung
1 sibling, 0 replies; 13+ messages in thread
From: Maciej W. Rozycki @ 2005-09-06 12:48 UTC (permalink / raw)
To: Richard Sandiford; +Cc: David Ung, binutils
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [patch - gas] MIPS: fix problem with mips16 delay branch optimization.
2005-09-06 12:15 ` Richard Sandiford
2005-09-06 12:48 ` Maciej W. Rozycki
@ 2005-09-06 15:50 ` David Ung
2005-09-06 15:59 ` Richard Sandiford
1 sibling, 1 reply; 13+ messages in thread
From: David Ung @ 2005-09-06 15:50 UTC (permalink / raw)
To: Richard Sandiford; +Cc: binutils
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.)
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [patch - gas] MIPS: fix problem with mips16 delay branch optimization.
2005-09-06 15:50 ` David Ung
@ 2005-09-06 15:59 ` Richard Sandiford
2005-09-06 16:43 ` David Ung
0 siblings, 1 reply; 13+ messages in thread
From: Richard Sandiford @ 2005-09-06 15:59 UTC (permalink / raw)
To: David Ung; +Cc: binutils
David Ung <davidu@mips.com> 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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [patch - gas] MIPS: fix problem with mips16 delay branch optimization.
2005-09-06 15:59 ` Richard Sandiford
@ 2005-09-06 16:43 ` David Ung
2005-09-06 16:47 ` Richard Sandiford
0 siblings, 1 reply; 13+ messages in thread
From: David Ung @ 2005-09-06 16:43 UTC (permalink / raw)
To: Richard Sandiford; +Cc: binutils
On Tue, 2005-09-06 at 13:48 +0100, Richard Sandiford wrote:
> David Ung <davidu@mips.com> 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.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [patch - gas] MIPS: fix problem with mips16 delay branch optimization.
2005-09-06 16:43 ` David Ung
@ 2005-09-06 16:47 ` Richard Sandiford
2005-09-06 18:23 ` David Ung
0 siblings, 1 reply; 13+ messages in thread
From: Richard Sandiford @ 2005-09-06 16:47 UTC (permalink / raw)
To: David Ung; +Cc: binutils
David Ung <davidu@mips.com> 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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [patch - gas] MIPS: fix problem with mips16 delay branch optimization.
2005-09-06 16:47 ` Richard Sandiford
@ 2005-09-06 18:23 ` David Ung
2005-09-06 18:38 ` Richard Sandiford
0 siblings, 1 reply; 13+ messages in thread
From: David Ung @ 2005-09-06 18:23 UTC (permalink / raw)
To: Richard Sandiford; +Cc: binutils
> >> 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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [patch - gas] MIPS: fix problem with mips16 delay branch optimization.
2005-09-06 18:23 ` David Ung
@ 2005-09-06 18:38 ` Richard Sandiford
2005-09-07 13:29 ` David Ung
0 siblings, 1 reply; 13+ messages in thread
From: Richard Sandiford @ 2005-09-06 18:38 UTC (permalink / raw)
To: David Ung; +Cc: binutils
David Ung <davidu@mips.com> 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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [patch - gas] MIPS: fix problem with mips16 delay branch optimization.
2005-09-06 18:38 ` Richard Sandiford
@ 2005-09-07 13:29 ` David Ung
0 siblings, 0 replies; 13+ messages in thread
From: David Ung @ 2005-09-07 13:29 UTC (permalink / raw)
To: Richard Sandiford; +Cc: binutils
On Tue, 2005-09-06 at 17:47 +0100, Richard Sandiford wrote:
> David Ung <davidu@mips.com> 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 <davidu@mips.com>
* 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)
{
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2005-09-07 11:39 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2005-09-01 14:30 [patch - gas] MIPS: fix problem with mips16 delay branch optimization David Ung
2005-09-01 15:48 ` Thiemo Seufer
2005-09-06 0:57 ` Richard Sandiford
2005-09-06 11:45 ` David Ung
2005-09-06 12:15 ` Richard Sandiford
2005-09-06 12:48 ` Maciej W. Rozycki
2005-09-06 15:50 ` David Ung
2005-09-06 15:59 ` Richard Sandiford
2005-09-06 16:43 ` David Ung
2005-09-06 16:47 ` Richard Sandiford
2005-09-06 18:23 ` David Ung
2005-09-06 18:38 ` Richard Sandiford
2005-09-07 13:29 ` David Ung
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).