Re: [PATCH] MIPS/GCC: Unconditional jump generation bug fix
Maciej W. Rozycki ma...@codesourcery.com writes:
2014-11-17 Maciej W. Rozycki ma...@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.0 +
+++ gcc-fsf-trunk-quilt/gcc/config/mips/mips.md 2014-11-17
04:44:32.847732003 +
@@ -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
Richard Sandiford richard.sandif...@arm.com writes:
Maciej W. Rozycki ma...@codesourcery.com writes:
2014-11-17 Maciej W. Rozycki ma...@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.md2014-11-16
19:54:17.0 +
+++ gcc-fsf-trunk-quilt/gcc/config/mips/mips.md 2014-11-17
04:44:32.847732003 +
@@ -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
Matthew Fortune matthew.fort...@imgtec.com writes:
Richard Sandiford richard.sandif...@arm.com writes:
Maciej W. Rozycki ma...@codesourcery.com writes:
2014-11-17 Maciej W. Rozycki ma...@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.0 +
+++ gcc-fsf-trunk-quilt/gcc/config/mips/mips.md2014-11-17
04:44:32.847732003 +
@@ -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
RE: [PATCH] MIPS/GCC: Unconditional jump generation bug fix
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
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
Maciej W. Rozycki ma...@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
-Original Message- From: Matthew Fortune [mailto:matthew.fort...@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 ma...@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
-Original Message- From: Matthew Fortune [mailto:matthew.fort...@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 ma...@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
OK to apply? 2014-11-17 Maciej W. Rozycki ma...@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
