This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
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.)