Re: [PATCH] MIPS/GCC: Unconditional jump generation bug fix

[Richard Sandiford <richard dot sandiford at arm dot com>]

Matthew Fortune <Matthew.Fortune@imgtec.com> writes:
> Richard Sandiford <richard.sandiford@arm.com> writes:
>> "Maciej W. Rozycki" <macro@codesourcery.com> writes:
>> > 2014-11-17  Maciej W. Rozycki  <macro@codesourcery.com>
>> >
>> > 	gcc/
>> > 	* gcc/config/mips/mips.md (*jump_absolute): Use a branch when in
>> > 	range, a jump otherwise.
>> >
>> >   Maciej
>> >
>> > gcc-mips-jump-branch.diff
>> > Index: gcc-fsf-trunk-quilt/gcc/config/mips/mips.md
>> > ===================================================================
>> > --- gcc-fsf-trunk-quilt.orig/gcc/config/mips/mips.md	2014-11-16
>> 19:54:17.000000000 +0000
>> > +++ gcc-fsf-trunk-quilt/gcc/config/mips/mips.md	2014-11-17
>> 04:44:32.847732003 +0000
>> > @@ -5957,14 +5957,12 @@
>> >  	(label_ref (match_operand 0)))]
>> >    "!TARGET_MIPS16 && TARGET_ABSOLUTE_JUMPS"
>> >  {
>> > -  /* Use a branch for microMIPS.  The assembler will choose
>> > -     a 16-bit branch, a 32-bit branch, or a 32-bit jump.  */
>> > -  if (TARGET_MICROMIPS && !TARGET_ABICALLS_PIC2)
>> > +  if (get_attr_length (insn) <= 8)
>> >      return "%*b\t%l0%/";
>> >    else
>> >      return MIPS_ABSOLUTE_JUMP ("%*j\t%l0%/");
>> >  }
>> > -  [(set_attr "type" "jump")])
>> > +  [(set_attr "type" "branch")])
>> 
>> You didn't mention it explicitly, but this will have the effect of
>> overestimating the length of the insn by 8 bytes in cases where the
>> jump is used.  That might be an acceptable trade-off (even for
>> non-microMIPS code) but it's probably worth mentioning in a comment.
>
> I honestly haven't digested all the detail of the length attribute
> calculation but I assume this comes from the fact that type=branch are
> assumed to only support 16-bit PC-relative displacement and a multi
> instruction sequence otherwise?

Yeah, and the patch relies on that overestimation by using the
"get_attr_length (insn) <= 8" condition to tell whether a branch is OK.
I.e. it uses the branch if the insn is assumed to be 4 bytes + delay
slot and uses a jump if the insn is assumed to be bigger.  But the
insn we actually emit is never bigger; it's always 4 bytes + delay slot.

Obviously the cases where we need the jump should be rare,
but those are also the cases where overestimating hurts most.
Saying that this instruction is 12 bytes + delay slot means
that conditional branches around it may be turned into
long-branch sequences even if they are actually in range.

> Perhaps in the long run we need to educate the length calculation for
> jumps to know about the unconditional branch range and size the
> instruction appropriately if the range is known to be within a 16-bit.
> This pattern could then change back to a jump.
>
> I suspect all the length calculation logic for jumps/branches etc will
> need an overhaul as part of adding R6 compact branch support. I have
> been working on this with AndrewB and the first cut just leaves the
> length calculation to overestimate as it is hard enough to just get it
> all working.

Yeah, can imagine that would be tricky :-)

Thanks,
Richard