Re: [PATC] MIPS16/GAS: Permit branch swapping with PC-relative insns

["Maciej W. Rozycki" <macro at codesourcery dot com>]

On Sat, 6 Aug 2011, Richard Sandiford wrote:

> >  While at microMIPS branch swapping, I believe it is safe to swap MIPS16 
> > basic instructions that use the PC-relative addressing mode too with a 
> > subsequent jump instruction to fill its delay slot.  There are four such 
> > instructions, namely: ADDIU, DADDIU, LD and LW.  There are two arguments 
> > standing both at a time that make me think so:
> >
> > 1. The use of any complex expression (one that evaluates to but a plain 
> >    number) as the immediate argument of any of these instructions causes 
> >    relaxation to trigger (even if the basic form of the instruction is 
> >    forced with .set noautoxtend or .t suffix) that marks the instruction 
> >    fixed and inhibits branch swapping.
> >
> > 2. All these instructions, if placed in a jump delay slot, use the value 
> >    of the PC of the preceding jump rather than that of the delay slot 
> >    itself for calculation -- the value of the PC retrieved therefore 
> >    remains the same after the swap (note that, contrariwise, this is not 
> >    the case with the microMIPS ADDIUPC instruction).
> 
> I agree the calculated address remains the same.  The problem is that
> swapping the instructions would change the contents of that address.
> 
> Of course, it's vanishingly unlikely that anyone would write a
> constant addiupc in a reorder block and follow it with an unfilled
> delayed branch.  Perhaps it could even be called user error.  But given
> that we have deliberately not swapped until now, changing the behaviour
> for this one case (and for this one case only) would just be a gratutious
> break in backwards compatibility.
> 
> So, not ok.  However...

 Hmm, I have troubles buying that -- that can be true about any address 
calculation that gets swapped into a branch delay slot.  And about the 
only use of such calculation, requiring the resulting address to point to 
the instruction doing it, is to patch it with something else in 
self-modifying code, which sounds useless, but let it be that I may be 
overlooking something here.

 What I do not overlook, and neither you do, is that such code requires a 
noreorder block, or otherwise GAS is essentially *by definition* free to 
shuffle code around, transform instructions, add NOPs and do all kinds of 
weird stuff, so I fail to see why we should be refraining from making this 
particular change.

 Have you got any historical records that back up your position and 
explain why this statement has originally been placed in?  Otherwise I'd 
just expect a misunderstanding of how unusually the addition is calculated 
-- which would be unsurprising to me, as I've seen similar mistakes 
elsewhere, e.g. up until recently (I believe) QEMU used to get the result 
of these instructions in delay slots wrong -- hopefully this piece of GAS 
code is not a workaround for that!

> >  While at it I have adjusted a comment about MIPS16 mode fixups that I 
> > believe is no longer accurate -- these days we support MIPS16 mode 
> > relocations on instructions other than branches (hmm, was it jumps that 
> > were meant here instead? -- or relaxed branches that are otherwise fixed 
> > as noted above anyway?) too (but then they are extended instructions).
> 
> ...I agree this comment needs a bit of TLC.
> 
> > @@ -3679,8 +3679,8 @@ can_swap_branch_p (struct mips_cl_insn *
> >      return FALSE;
> >  
> >    /* If the previous instruction had a fixup in mips16 mode, we can not
> > -     swap.  This normally means that the previous instruction was a 4
> > -     byte branch anyhow.  */
> > +     swap.  This normally means that the previous instruction was
> > +     a 4-byte extended instruction anyhow.  */
> >    if (mips_opts.mips16 && history[0].fixp[0])
> >      return FALSE;
> 
> It always means that the previous instruction was a 4-byte instruction.
> I'm not sure adding "extended" is a good idea though, since JAL can have
> a fixup but doesn't use EXTEND.

 Indeed, it's a notable exception (as is JALX).

> A patch that changes the comment to:
> 
>   /* If the previous instruction had a fixup in mips16 mode, we can not swap.
>      This means that the previous instruction was a 4-byte one anyhow.  */
> 
> (and does nothing else) is OK.

 Committed then.

  Maciej

2011-08-10  Maciej W. Rozycki  <macro@codesourcery.com>

	gas/
	* config/tc-mips.c (can_swap_branch_p): Update the comment on 
	MIPS16 fixups.

binutils-gas-mips16-ext-swap.diff
Index: binutils-fsf-trunk-quilt/gas/config/tc-mips.c
===================================================================
--- binutils-fsf-trunk-quilt.orig/gas/config/tc-mips.c	2011-08-10 22:42:21.000000000 +0100
+++ binutils-fsf-trunk-quilt/gas/config/tc-mips.c	2011-08-10 22:52:20.000000000 +0100
@@ -3715,9 +3715,8 @@ can_swap_branch_p (struct mips_cl_insn *
   if (history[1].noreorder_p)
     return FALSE;
 
-  /* If the previous instruction had a fixup in mips16 mode, we can not
-     swap.  This normally means that the previous instruction was a 4
-     byte branch anyhow.  */
+  /* If the previous instruction had a fixup in mips16 mode, we can not swap.
+     This means that the previous instruction was a 4-byte one anyhow.  */
   if (mips_opts.mips16 && history[0].fixp[0])
     return FALSE;