| Message ID | alpine.DEB.1.10.1411160038070.2881@tp.orcam.me.uk |
|---|---|
| State | Accepted |
| Headers | show |
> 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
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
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
> -----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.
> > -----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
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
"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
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
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
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)