This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: [PATCH] MIPS: microMIPS compact branch linker relaxation check
- From: "Maciej W. Rozycki" <macro at codesourcery dot com>
- To: Richard Sandiford <rdsandiford at googlemail dot com>
- Cc: binutils at sourceware dot org, Chao-ying Fu <fu at mips dot com>, Rich Fuhler <rich at mips dot com>, David Lau <davidlau at mips dot com>, Kevin Mills <kevinm at mips dot com>, Ilie Garbacea <ilie at mips dot com>, Catherine Moore <clm at codesourcery dot com>, Nathan Sidwell <nathan at codesourcery dot com>, Joseph Myers <joseph at codesourcery dot com>
- Date: Mon, 1 Aug 2011 15:51:25 +0100 (BST)
- Subject: Re: [PATCH] MIPS: microMIPS compact branch linker relaxation check
- References: <alpine.DEB.1.10.1107292358340.4083@tp.orcam.me.uk> <87bowcnvcr.fsf@firetop.home>
On Sat, 30 Jul 2011, Richard Sandiford wrote:
> > +/* If the instruction encoding at PTR and relocations [INTERNAL_RELOCS,
> > + IRELEND) at OFFSET indicate that it is a compact branch there, then
> > + return TRUE, otherwise FALSE. */
>
> Probably worth s/that it is/that there must be/.
OK, I won't argue about the subtleties of English.
> "check_relocated_bzc" would be a more descriptive name.
Agreed, good idea.
> > @@ -12474,18 +12480,18 @@ _bfd_mips_elf_relax_section (bfd *abfd,
> > && ELF32_R_SYM (irel[2].r_info) == r_symndx)
> > continue;
> >
> > - /* See if the LUI instruction *might* be in a branch delay slot. */
> > + /* See if the LUI instruction *might* be in a branch delay slot.
> > + We check whether what looks like a 16-bit branch or jump is
> > + actually an immediate argument to a compact branch, and let
> > + it through if so. */
> > if (irel->r_offset >= 2
> > && check_br16_dslot (abfd, ptr - 2)
> > && !(irel->r_offset >= 4
> > - /* If the instruction is actually a 4-byte branch,
> > - the value of check_br16_dslot doesn't matter.
> > - We should use check_br32_dslot to check whether
> > - the branch has a delay slot. */
> > - && check_4byte_branch (internal_relocs, irelend,
> > - irel->r_offset - 4)))
> > + && (bzc = check_bzc (abfd, ptr - 4, irel->r_offset - 4,
> > + internal_relocs, irelend))))
> > continue;
> > if (irel->r_offset >= 4
> > + && !bzc
> > && check_br32_dslot (abfd, ptr - 4))
> > continue;
> >
>
> I think the flow is more obvious as:
>
> /* See if the LUI instruction *might* be in a branch delay slot. */
> if (rel->r_offset >= 4
> && check_relocated_bzc (...))
> /* The previous instruction looks like a compact branch,
> and the relocations confirm that it is one. We can be
> confident that there is no delay slot. */
> ;
> else if ((rel->r_offset >= 2 && check_br16_slot (abfd, ptr - 2)
> || (rel->r_offset >= 4 && check_br32_dslot (abfd, ptr - 4)))
> continue;
>
> OK with that change.
I would have done it originally myself, except that I have chosen the
current flow deliberately. Please note that (unlike check_br16_slot() or
check_br32_dslot()) check_relocated_bzc() is *expensive* in that it
requires iterating over the relocation table, making it O(n) (as opposed
to O(1)). Therefore I've chosen to check for it only if check_br16_slot()
indicates the preceding instruction would otherwise be a 16-bit ordinary
branch/jump.
You could argue that it only really matters in the rare case where the
previous instruction looks like a 16-bit ordinary branch/jump and the
second previous instruction has a lower 16-bit halfword that looks like a
compact branch, but I see no reason to deliberately pessimise this case
when we don't have to, essentially for free.
Maciej