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

[Richard Sandiford <rdsandiford at googlemail dot com>]

"Maciej W. Rozycki" <macro@codesourcery.com> writes:
>  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...

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

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.

Richard