Created attachment 28079 [details] A simple test for an issue in reorg.c Please check this example when using GCC 4.6 for mips-linux-gnu. (I think the issue is in GCC 4.7 and later as well. But due to the basic block ordering, I cannot reproduce this issue.) Ex: # /gcc46branch/build-mips/gcc/cc1plus -quiet Bug.i -o Bug.s -mips32r2 -O2 # cat Bug.s _Z3bugP9ValueRTRK: .frame $sp,0,$31 # vars= 0, regs= 0/0, args= 0, gp= 0 .mask 0x00000000,0 .fmask 0x00000000,0 .set noreorder .set nomacro lw $2,0($4) <--------- lw $2,0($2) <--------- (NO RETURN IN THIS FUNCTION.) .set macro .set reorder .end _Z3bugP9ValueRTRK .cfi_endproc $LFE13: .size _Z3bugP9ValueRTRK, .-_Z3bugP9ValueRTRK .ident "GCC: (GNU) 4.6.4 20120823 (prerelease) [gcc-4_6-branch revision 190613]" The delayed-branch pass in reorg.c is too aggressive, such that a return instruction and others are removed. From debugging code in reorg.c: delete_jump() -> delete_computation() -> delete_related_insn(), I think we should not use delete_related_insn() that removes branches and all the following instructions. A simple fix may be just using delete_insn() in delete_computation(). Ex: # svn diff reorg.c Index: reorg.c =================================================================== --- reorg.c (revision 190636) +++ reorg.c (working copy) @@ -3298,7 +3298,7 @@ delete_prior_computation (note, insn); } - delete_related_insns (insn); + delete_insn (insn); } /* If all INSN does is set the pc, delete it, Ex: (Bug.s after this fix.) # cat Bug.s _Z3bugP9ValueRTRK: .frame $sp,0,$31 # vars= 0, regs= 0/0, args= 0, gp= 0 .mask 0x00000000,0 .fmask 0x00000000,0 .set noreorder .set nomacro lw $2,0($4) lw $2,0($2) $L4: j $31 <------------- OK srl $2,$2,8 <------------- OK .set macro .set reorder .end _Z3bugP9ValueRTRK .cfi_endproc $LFE13: .size _Z3bugP9ValueRTRK, .-_Z3bugP9ValueRTRK .ident "GCC: (GNU) 4.6.4 20120823 (prerelease) [gcc-4_6-branch revision 190613]" Thanks!
delete_related_insns isn't supposed to do incorrect things though, so the issue must really be analyzed first.
I think I see where delete_related_insns is going wrong. We call it with a JUMP instruction that we want to remove because we are just jumping to the next instruction (a label) which we would get to anyway without the jump, then after delete_related_insns removes the jump it checks the label it was jumping to, and if it finds no uses it calls delete_related_insns with that label. When delete_related_insns is called with a label, it assumes it can remove all the code after that label as unreachable. But the code after the label can be reached by the default fall-through code sequence. So after removing a jump and finding a label with no uses we should call delete_insn with that label, not delete_related_insns. So my proposed fix would be: diff --git a/gcc/jump.c b/gcc/jump.c index 9721fe1..fa3d65a 100644 --- a/gcc/jump.c +++ b/gcc/jump.c @@ -1207,7 +1207,7 @@ delete_related_insns (rtx insn) /* This can delete NEXT or PREV, either directly if NEXT is JUMP_LABEL (INSN), or indirectly through more levels of jumps. */ - delete_related_insns (lab); + delete_insn (lab); else if (tablejump_p (insn, NULL, &lab_next)) { /* If we're deleting the tablejump, delete the dispatch table.
The fall-through path of this jump has a barrier (resulted from __builtin_unreachable), and the target of this jump has a return instruction. The algorithm in reorg.c may be more conservative to keep this jump for code correctness. Just another solution to this issue. Thanks!
> I think I see where delete_related_insns is going wrong. We call it > with a JUMP instruction that we want to remove because we are just > jumping to the next instruction (a label) which we would get to anyway > without the jump, then after delete_related_insns removes the jump it > checks the label it was jumping to, and if it finds no uses it calls > delete_related_insns with that label. When delete_related_insns > is called with a label, it assumes it can remove all the code after > that label as unreachable. Only if there is a barrier just before the label though; now this barrier should have been removed one level earlier. The root cause of the problem is that MIPS runs its own version of the dbr pass and doesn't make sure that barriers are correctly placed for it, unlike other targets like SPARC. So the fix is: Index: config/mips/mips.c =================================================================== --- config/mips/mips.c (revision 184886) +++ config/mips/mips.c (working copy) @@ -15083,7 +15083,10 @@ mips_reorg (void) } if (optimize > 0 && flag_delayed_branch) - dbr_schedule (get_insns ()); + { + cleanup_barriers (); + dbr_schedule (get_insns ()); + } mips_reorg_process_insns (); if (!TARGET_MIPS16 && TARGET_EXPLICIT_RELOCS
Thanks for digging into this Eric. I tested your patch here against the example and on the GCC testsuite and didn't see any problems. Are you going to check this in on the mainline?
> Thanks for digging into this Eric. I tested your patch here against the > example and on the GCC testsuite and didn't see any problems. You're welcome. Thanks for the testing. > Are you going to check this in on the mainline? Not without Richard's approval, so I've CCed him now.
"ebotcazou at gcc dot gnu.org" <gcc-bugzilla@gcc.gnu.org> writes: >> Are you going to check this in on the mainline? > > Not without Richard's approval, so I've CCed him now. Looks good, thanks. Please go ahead. Just out of interest, you said: > The root cause of the problem is that MIPS runs its own version of the dbr pass > and doesn't make sure that barriers are correctly placed for it, unlike other > targets like SPARC. But how does SPARC do it? sparc_reorg starts: /* The only erratum we handle for now is that of the AT697F processor. */ if (!sparc_fix_at697f) return; /* We need to have the (essentially) final form of the insn stream in order to properly detect the various hazards. Run delay slot scheduling. */ if (optimize > 0 && flag_delayed_branch) dbr_schedule (get_insns ()); without any explicit call to cleanup_barriers (which like you say usually runs after machine_reorg but before delay_slots).
> Looks good, thanks. Please go ahead. Thanks. On which branch(es)? > But how does SPARC do it? sparc_reorg starts: > > /* The only erratum we handle for now is that of the AT697F processor. */ > if (!sparc_fix_at697f) > return; > > /* We need to have the (essentially) final form of the insn stream in order > to properly detect the various hazards. Run delay slot scheduling. */ > if (optimize > 0 && flag_delayed_branch) > dbr_schedule (get_insns ()); > > without any explicit call to cleanup_barriers (which like you say usually > runs after machine_reorg but before delay_slots). Gosh. I didn't remember that sparc_reorg existed... I was referring to the regular pass pipeline, which is almost always used for SPARC. So I found the bug in the MIPS port and you found it in the SPARC port. Time to swap? ;-)
"ebotcazou at gcc dot gnu.org" <gcc-bugzilla@gcc.gnu.org> writes: >> Looks good, thanks. Please go ahead. > > Thanks. On which branch(es)? Hmm, looks like this goes all the way back to 4.5, so as any as you've got the stamina for. It should be a low-risk change. Richard
Author: ebotcazou Date: Sun Sep 2 10:36:27 2012 New Revision: 190858 URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=190858 Log: PR rtl-optimization/54369 * config/mips/mips.c (mips_reorg): Invoke cleanup_barriers before calling dbr_schedule. * config/sparc/sparc.c (sparc_reorg): Likewise. Modified: trunk/gcc/ChangeLog trunk/gcc/config/mips/mips.c trunk/gcc/config/sparc/sparc.c
Author: ebotcazou Date: Sun Sep 2 10:36:54 2012 New Revision: 190859 URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=190859 Log: PR rtl-optimization/54369 * config/mips/mips.c (mips_reorg): Invoke cleanup_barriers before calling dbr_schedule. * config/sparc/sparc.c (sparc_reorg): Likewise. Modified: branches/gcc-4_7-branch/gcc/ChangeLog branches/gcc-4_7-branch/gcc/config/mips/mips.c branches/gcc-4_7-branch/gcc/config/sparc/sparc.c
Author: ebotcazou Date: Sun Sep 2 10:37:49 2012 New Revision: 190860 URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=190860 Log: PR rtl-optimization/54369 * config/mips/mips.c (mips_reorg): Invoke cleanup_barriers before calling dbr_schedule. * config/sparc/sparc.c (sparc_reorg): Likewise. Modified: branches/gcc-4_6-branch/gcc/ChangeLog branches/gcc-4_6-branch/gcc/config/mips/mips.c branches/gcc-4_6-branch/gcc/config/sparc/sparc.c
On all active branches.
*** Bug 260998 has been marked as a duplicate of this bug. *** Seen from the domain http://volichat.com Marked for reference. Resolved as fixed @bugzilla.