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

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

[Maciej W. Rozycki]

Hi,

 This was lost in the original microMIPS submission.

 For absolute microMIPS jumps GCC always produces a branch instruction, 
causing a `relocation truncated to fit: R_MICROMIPS_PC16_S1' linker 
error if the branch target turns out of range.  The TARGET_ABICALLS_PIC2 
macro is never set in absolute code.  This is the only RTL pattern that 
we have that does not handle this case correctly, all the other ones set 
the type of the pattern to "branch" and rely on instruction length 
calculation to see if branch relaxation would trigger on not.  If so, 
then they produce an appropriate jump instruction.

 So do it here as well; this affects the standard mode too (branches are 
now always produced whenever in range), but IMHO in a good or at least 
neutral way.

 Regression-tested with the mips-linux-gnu target and these multilibs:

-EB
-EB -mips16
-EB -mmicromips
-EB -mabi=n32
-EB -mabi=64

and the -EL variants of same, fixing these failures:

g++.dg/opt/longbranch1.C  -std=gnu++11 (test for excess errors)
g++.dg/opt/longbranch1.C  -std=gnu++14 (test for excess errors)
g++.dg/opt/longbranch1.C  -std=gnu++98 (test for excess errors)

across microMIPS multilibs, e.g.:

FAIL: g++.dg/opt/longbranch1.C  -std=gnu++11 (test for excess errors)
Excess errors:
longbranch1.C:(.text+0x15092): relocation truncated to fit: R_MICROMIPS_PC16_S1 against `$L2'

No other changes in test results.

 OK to apply?

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")])
 
 (define_insn "*jump_pic"
   [(set (pc)

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

[Matthew Fortune]

>  OK to apply?
> 
> 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.

OK.

I only got my head around this code last week otherwise I wouldn't have
known whether this was correct!

Thanks,
Matthew

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

[Maciej W. Rozycki]

On Mon, 17 Nov 2014, Matthew Fortune wrote:

> > 	gcc/
> > 	* gcc/config/mips/mips.md (*jump_absolute): Use a branch when in
> > 	range, a jump otherwise.
> 
> OK.
> 
> I only got my head around this code last week otherwise I wouldn't have
> known whether this was correct!

 Committed now, thanks for your review.  How about 4.9? -- once it is 
unfrozen, that is.

  Maciej

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

[Matthew Fortune]

Maciej W. Rozycki <macro@codesourcery.com> writes:
> On Mon, 17 Nov 2014, Matthew Fortune wrote:
> 
> > > 	gcc/
> > > 	* gcc/config/mips/mips.md (*jump_absolute): Use a branch when in
> > > 	range, a jump otherwise.
> >
> > OK.
> >
> > I only got my head around this code last week otherwise I wouldn't
> > have known whether this was correct!
> 
>  Committed now, thanks for your review.  How about 4.9? -- once it is
> unfrozen, that is.

I admit to being a bit more nervous about 4.9 but the test coverage seems
thorough enough. I guess I would have been less concerned if the
optimisation was still just tied to TARGET_MICROMIPS for the 4.9 branch.

Catherine, what do you think?

Matthew

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

[Moore, Catherine]

> -----Original Message-----
> From: Matthew Fortune [mailto:Matthew.Fortune@imgtec.com]
> Sent: Tuesday, November 18, 2014 12:22 PM
> To: Rozycki, Maciej
> Cc: gcc-patches@gcc.gnu.org; Moore, Catherine; Eric Christopher
> Subject: RE: [PATCH] MIPS/GCC: Unconditional jump generation bug fix
> 
> Maciej W. Rozycki <macro@codesourcery.com> writes:
> > On Mon, 17 Nov 2014, Matthew Fortune wrote:
> >
> > > > 	gcc/
> > > > 	* gcc/config/mips/mips.md (*jump_absolute): Use a branch when in
> > > > 	range, a jump otherwise.
> > >
> > > OK.
> > >
> > > I only got my head around this code last week otherwise I wouldn't
> > > have known whether this was correct!
> >
> >  Committed now, thanks for your review.  How about 4.9? -- once it is
> > unfrozen, that is.
> 
> I admit to being a bit more nervous about 4.9 but the test coverage seems
> thorough enough. I guess I would have been less concerned if the
> optimisation was still just tied to TARGET_MICROMIPS for the 4.9 branch.
> 
> Catherine, what do you think?
> 
This is okay for 4.9 IMO.

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

[Matthew Fortune]

> > -----Original Message-----
> > From: Matthew Fortune [mailto:Matthew.Fortune@imgtec.com]
> > Sent: Tuesday, November 18, 2014 12:22 PM
> > To: Rozycki, Maciej
> > Cc: gcc-patches@gcc.gnu.org; Moore, Catherine; Eric Christopher
> > Subject: RE: [PATCH] MIPS/GCC: Unconditional jump generation bug fix
> >
> > Maciej W. Rozycki <macro@codesourcery.com> writes:
> > > On Mon, 17 Nov 2014, Matthew Fortune wrote:
> > >
> > > > > 	gcc/
> > > > > 	* gcc/config/mips/mips.md (*jump_absolute): Use a branch when
> in
> > > > > 	range, a jump otherwise.
> > > >
> > > > OK.
> > > >
> > > > I only got my head around this code last week otherwise I wouldn't
> > > > have known whether this was correct!
> > >
> > >  Committed now, thanks for your review.  How about 4.9? -- once it
> > > is unfrozen, that is.
> >
> > I admit to being a bit more nervous about 4.9 but the test coverage
> > seems thorough enough. I guess I would have been less concerned if the
> > optimisation was still just tied to TARGET_MICROMIPS for the 4.9
> branch.
> >
> > Catherine, what do you think?
> >
> This is okay for 4.9 IMO.

OK

Matthew

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

[Maciej W. Rozycki]

On Tue, 18 Nov 2014, Matthew Fortune wrote:

> > > I admit to being a bit more nervous about 4.9 but the test coverage
> > > seems thorough enough. I guess I would have been less concerned if the
> > > optimisation was still just tied to TARGET_MICROMIPS for the 4.9
> > branch.
> > >
> > > Catherine, what do you think?
> > >
> > This is okay for 4.9 IMO.
> 
> OK

 FWIW we've been using this change since Oct 2012 with no issues (as I 
noted it was meant to be included with the original microMIPS support 
submission, but was lost in transit) and also GAS has code to relax 
out-of-range branches to jumps in non-PIC standard MIPS code under the 
same condition this RTL insn uses, so even if a wrong branch slipped 
through here (which it doesn't), then GAS would fix it up.

 See gas/config/tc-mips.c (md_apply_fix) <BFD_RELOC_16_PCREL_S2> for the 
relaxation piece if interested.

  Maciej

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

[Richard Sandiford]

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

Thanks,
Richard

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

[Matthew Fortune]

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?

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.

Thanks,
Matthew

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

[Richard Sandiford]

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