MIPS/GCC: Unconditional jump generation bug fix
diff mbox

Message ID alpine.DEB.1.10.1411160038070.2881@tp.orcam.me.uk
State Accepted
Headers show

Commit Message

Maciej W. Rozycki Nov. 17, 2014, 4:06 p.m. UTC
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

Comments

Matthew Fortune Nov. 17, 2014, 4:47 p.m. UTC | #1
>  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
Maciej W. Rozycki Nov. 18, 2014, 4:49 p.m. UTC | #2
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
Matthew Fortune Nov. 18, 2014, 5:21 p.m. UTC | #3
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
Moore, Catherine Nov. 18, 2014, 5:23 p.m. UTC | #4
> -----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.
Matthew Fortune Nov. 18, 2014, 7:21 p.m. UTC | #5
> > -----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
Maciej W. Rozycki Nov. 19, 2014, 1:29 p.m. UTC | #6
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
Richard Sandiford Dec. 5, 2014, 10:48 a.m. UTC | #7
"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
Matthew Fortune Dec. 5, 2014, 11:06 a.m. UTC | #8
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
Richard Sandiford Dec. 5, 2014, 11:25 a.m. UTC | #9
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

Patch
diff mbox

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)