| Message ID | 735be23689dfc1572bd3d87f304b4e9b23ba6442.1479944009.git.segher@kernel.crashing.org |
|---|---|
| State | New |
| Headers | show |
On 11/23/2016 04:55 PM, Segher Boessenkool wrote: > Combine can turn a conditional trap into an unconditional trap. If it > does that it should make the code after it unreachable (an unconditional > trap should be the last insn in its bb, and that bb has no successors). > > This patch seems to work. It is hard to be sure, this is very hard to > trigger. Quite a few other passes look like they need something similar > as well, but I don't see anything else handling it yet either. > > How does this look? Any better ideas? It's probably a reasonable place to start as there's lots of similarities to converting a conditional branch to an unconditional. Like you I worry there may be other instances of this issue may be lurking. The possibility of having a conditional trap in the IL turn into an unconditional trap has always been there. As you note, it's somewhat difficult to trigger since the SSA optimizers ought to expose/optimize this long before we get to RTL. Jeff
On Mon, 2016-11-28 at 14:44 -0700, Jeff Law wrote: > On 11/23/2016 04:55 PM, Segher Boessenkool wrote: > > Combine can turn a conditional trap into an unconditional trap. If > > it > > does that it should make the code after it unreachable (an > > unconditional > > trap should be the last insn in its bb, and that bb has no > > successors). > > > > This patch seems to work. It is hard to be sure, this is very hard > > to > > trigger. Quite a few other passes look like they need something > > similar > > as well, but I don't see anything else handling it yet either. > > > > How does this look? Any better ideas? > It's probably a reasonable place to start as there's lots of > similarities to converting a conditional branch to an unconditional. > Like you I worry there may be other instances of this issue may be > lurking. > > The possibility of having a conditional trap in the IL turn into an > unconditional trap has always been there. As you note, it's somewhat > difficult to trigger since the SSA optimizers ought to > expose/optimize > this long before we get to RTL. Could the RTL frontend be of use here?
On 11/28/2016 03:00 PM, David Malcolm wrote: > On Mon, 2016-11-28 at 14:44 -0700, Jeff Law wrote: >> On 11/23/2016 04:55 PM, Segher Boessenkool wrote: >>> Combine can turn a conditional trap into an unconditional trap. If >>> it >>> does that it should make the code after it unreachable (an >>> unconditional >>> trap should be the last insn in its bb, and that bb has no >>> successors). >>> >>> This patch seems to work. It is hard to be sure, this is very hard >>> to >>> trigger. Quite a few other passes look like they need something >>> similar >>> as well, but I don't see anything else handling it yet either. >>> >>> How does this look? Any better ideas? >> It's probably a reasonable place to start as there's lots of >> similarities to converting a conditional branch to an unconditional. >> Like you I worry there may be other instances of this issue may be >> lurking. >> >> The possibility of having a conditional trap in the IL turn into an >> unconditional trap has always been there. As you note, it's somewhat >> difficult to trigger since the SSA optimizers ought to >> expose/optimize >> this long before we get to RTL. > > Could the RTL frontend be of use here? Maybe -- but only on targets where we have the right insns in the target machine description. jeff
On Mon, 2016-11-28 at 15:02 -0700, Jeff Law wrote: > On 11/28/2016 03:00 PM, David Malcolm wrote: > > On Mon, 2016-11-28 at 14:44 -0700, Jeff Law wrote: > > > On 11/23/2016 04:55 PM, Segher Boessenkool wrote: > > > > Combine can turn a conditional trap into an unconditional trap. > > > > If > > > > it > > > > does that it should make the code after it unreachable (an > > > > unconditional > > > > trap should be the last insn in its bb, and that bb has no > > > > successors). > > > > > > > > This patch seems to work. It is hard to be sure, this is very > > > > hard > > > > to > > > > trigger. Quite a few other passes look like they need > > > > something > > > > similar > > > > as well, but I don't see anything else handling it yet either. > > > > > > > > How does this look? Any better ideas? > > > It's probably a reasonable place to start as there's lots of > > > similarities to converting a conditional branch to an > > > unconditional. > > > Like you I worry there may be other instances of this issue may > > > be > > > lurking. > > > > > > The possibility of having a conditional trap in the IL turn into > > > an > > > unconditional trap has always been there. As you note, it's > > > somewhat > > > difficult to trigger since the SSA optimizers ought to > > > expose/optimize > > > this long before we get to RTL. > > > > Could the RTL frontend be of use here? > Maybe -- but only on targets where we have the right insns in the > target > machine description. Segher: it looks like the "PR78432" in the Subject line is a typo; that bug number appears to relate to go-dump.c and is marked as RESOLVED FIXED.
On Mon, Nov 28, 2016 at 05:09:50PM -0500, David Malcolm wrote: > Segher: it looks like the "PR78432" in the Subject line is a typo; that > bug number appears to relate to go-dump.c and is marked as RESOLVED > FIXED. It's PR78342, oops. Thanks, Segher
On Mon, Nov 28, 2016 at 02:44:44PM -0700, Jeff Law wrote: > On 11/23/2016 04:55 PM, Segher Boessenkool wrote: > >Combine can turn a conditional trap into an unconditional trap. If it > >does that it should make the code after it unreachable (an unconditional > >trap should be the last insn in its bb, and that bb has no successors). > > > >This patch seems to work. It is hard to be sure, this is very hard to > >trigger. Quite a few other passes look like they need something similar > >as well, but I don't see anything else handling it yet either. > > > >How does this look? Any better ideas? > It's probably a reasonable place to start as there's lots of > similarities to converting a conditional branch to an unconditional. > Like you I worry there may be other instances of this issue may be lurking. > > The possibility of having a conditional trap in the IL turn into an > unconditional trap has always been there. As you note, it's somewhat > difficult to trigger since the SSA optimizers ought to expose/optimize > this long before we get to RTL. I committed this with this changelog (and the PR # fixed): 2016-11-28 Segher Boessenkool <segher@kernel.crashing.org> PR rtl-optimization/78342 * combine.c: Include "cfghooks.h". (try_combine): If we create an unconditional trap, break the basic block in two just after it, and remove the edge between; also, set the *new_direct_jump_p flag so that cleanup_cfg is run.
diff --git a/gcc/combine.c b/gcc/combine.c index 18777f8..90397f58 100644 --- a/gcc/combine.c +++ b/gcc/combine.c @@ -82,6 +82,7 @@ along with GCC; see the file COPYING3. If not see #include "target.h" #include "rtl.h" #include "tree.h" +#include "cfghooks.h" #include "predict.h" #include "df.h" #include "memmodel.h" @@ -4648,6 +4649,25 @@ try_combine (rtx_insn *i3, rtx_insn *i2, rtx_insn *i1, rtx_insn *i0, update_cfg_for_uncondjump (undobuf.other_insn); } + if (GET_CODE (PATTERN (i3)) == TRAP_IF + && XEXP (PATTERN (i3), 0) == const1_rtx) + { + basic_block bb = BLOCK_FOR_INSN (i3); + gcc_assert (bb); + remove_edge (split_block (bb, i3)); + *new_direct_jump_p = 1; + } + + if (undobuf.other_insn + && GET_CODE (PATTERN (undobuf.other_insn)) == TRAP_IF + && XEXP (PATTERN (undobuf.other_insn), 0) == const1_rtx) + { + basic_block bb = BLOCK_FOR_INSN (undobuf.other_insn); + gcc_assert (bb); + remove_edge (split_block (bb, undobuf.other_insn)); + *new_direct_jump_p = 1; + } + /* A noop might also need cleaning up of CFG, if it comes from the simplification of a jump. */ if (JUMP_P (i3)