| Message ID | 20130405152230.GH24873@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 04/05/2013 09:22 AM, Marek Polacek wrote: > This patch prevents segfault when using --param min-crossjump-insns=0. > What can happen in that case is that flow_find_cross_jump returns 0, > thus nmatch is 0, then > nmatch < PARAM_VALUE (PARAM_MIN_CROSSJUMP_INSNS) > doesn't hold, thus we continue, but we segfault later on when > doing split_block. I think it's better to just bail out in that > case; moreover setting min-crossjump-insns to 0 isn't very common... > > Regtested/bootstrapped on x86_64-linux, ok for trunk/4.8? > > 2013-04-05 Marek Polacek <polacek@redhat.com> > > PR rtl-optimization/48182 > * cfgcleanup.c (try_crossjump_to_edge): Bail out if > PARAM_MIN_CROSSJUMP_INSNS is 0. > > * gcc.dg/pr48182.c: New test. OK for the trunk. Release manager's decision for 4.8. jeff
On Fri, Apr 05, 2013 at 02:21:57PM -0600, Jeff Law wrote: > On 04/05/2013 09:22 AM, Marek Polacek wrote: > >This patch prevents segfault when using --param min-crossjump-insns=0. > >What can happen in that case is that flow_find_cross_jump returns 0, > >thus nmatch is 0, then > >nmatch < PARAM_VALUE (PARAM_MIN_CROSSJUMP_INSNS) > >doesn't hold, thus we continue, but we segfault later on when > >doing split_block. I think it's better to just bail out in that > >case; moreover setting min-crossjump-insns to 0 isn't very common... > > > >Regtested/bootstrapped on x86_64-linux, ok for trunk/4.8? > > > >2013-04-05 Marek Polacek <polacek@redhat.com> > > > > PR rtl-optimization/48182 > > * cfgcleanup.c (try_crossjump_to_edge): Bail out if > > PARAM_MIN_CROSSJUMP_INSNS is 0. > > > > * gcc.dg/pr48182.c: New test. > OK for the trunk. Release manager's decision for 4.8. Wouldn't it be better to change params.def to instead say: 5, 1, 0) Because with the cfgcleanup.c change, --param min-crossjump-insns=0 is handled as =infinity rather than something smaller than 0. Jakub
On 04/05/2013 02:33 PM, Jakub Jelinek wrote: > On Fri, Apr 05, 2013 at 02:21:57PM -0600, Jeff Law wrote: >> On 04/05/2013 09:22 AM, Marek Polacek wrote: >>> This patch prevents segfault when using --param min-crossjump-insns=0. >>> What can happen in that case is that flow_find_cross_jump returns 0, >>> thus nmatch is 0, then >>> nmatch < PARAM_VALUE (PARAM_MIN_CROSSJUMP_INSNS) >>> doesn't hold, thus we continue, but we segfault later on when >>> doing split_block. I think it's better to just bail out in that >>> case; moreover setting min-crossjump-insns to 0 isn't very common... >>> >>> Regtested/bootstrapped on x86_64-linux, ok for trunk/4.8? >>> >>> 2013-04-05 Marek Polacek <polacek@redhat.com> >>> >>> PR rtl-optimization/48182 >>> * cfgcleanup.c (try_crossjump_to_edge): Bail out if >>> PARAM_MIN_CROSSJUMP_INSNS is 0. >>> >>> * gcc.dg/pr48182.c: New test. >> OK for the trunk. Release manager's decision for 4.8. > > Wouldn't it be better to change params.def to instead say: > 5, 1, 0) > Because with the cfgcleanup.c change, --param min-crossjump-insns=0 > is handled as =infinity rather than something smaller than 0. ? I must be missing something, the change causes an early bail out from try_crossjump_to_edge. We don't want to raise the min to > 0 as that doesn't allow the user to turn on this specific transformation. jeff
On Fri, Apr 05, 2013 at 02:42:19PM -0600, Jeff Law wrote: > ? I must be missing something, the change causes an early bail out > from try_crossjump_to_edge. > > We don't want to raise the min to > 0 as that doesn't allow the user > to turn on this specific transformation. The condition is if (nmatch < PARAM_VALUE (PARAM_MIN_CROSSJUMP_INSNS)) return false; // aka "don't crossjump" So, the smaller the N in --param min-crossjump-insns=N is, the more likely we crossjump. Thus N=0 should mean that it is most likely we crossjump, and as N=1 requires that at least one insn matches, N=0 would mean that even zero insns can match. If we for --param min-crossjump-insns=0 always return false, it means we never crossjump, so it is least likely that we crossjump, which corresponds to largest possible N, not smallest one. Jakub
On 04/05/2013 02:50 PM, Jakub Jelinek wrote: > On Fri, Apr 05, 2013 at 02:42:19PM -0600, Jeff Law wrote: >> ? I must be missing something, the change causes an early bail out >> from try_crossjump_to_edge. >> >> We don't want to raise the min to > 0 as that doesn't allow the user >> to turn on this specific transformation. > > The condition is > if (nmatch < PARAM_VALUE (PARAM_MIN_CROSSJUMP_INSNS)) > return false; // aka "don't crossjump" > So, the smaller the N in --param min-crossjump-insns=N is, the more likely > we crossjump. Thus N=0 should mean that it is most likely we crossjump, > and as N=1 requires that at least one insn matches, N=0 would mean that > even zero insns can match. If we for --param min-crossjump-insns=0 > always return false, it means we never crossjump, so it is least likely > that we crossjump, which corresponds to largest possible N, not smallest > one. Yes the smaller the N, the more likely we are to crossjump, of course the value 0 would make no sense (I'm clearly out of practice on reviews :-). Yea, changing the min value in params.def to 1 would be a better way to fix. Consider that patch pre-approved. jeff
--- gcc/testsuite/gcc.dg/pr48182.c.mp 2013-04-05 14:58:06.373269392 +0200 +++ gcc/testsuite/gcc.dg/pr48182.c 2013-04-05 14:57:47.867211373 +0200 @@ -0,0 +1,11 @@ +/* PR rtl-optimization/48182 */ +/* { dg-do compile } */ +/* { dg-options "-fcrossjumping --param min-crossjump-insns=0" } */ + +extern int bar (void); + +int +foo (int x) +{ + return x && bar (); +} --- gcc/cfgcleanup.c.mp 2013-04-05 14:55:01.634675751 +0200 +++ gcc/cfgcleanup.c 2013-04-05 16:33:23.701814048 +0200 @@ -1929,8 +1929,9 @@ try_crossjump_to_edge (int mode, edge e1 of matching instructions or the 'from' block was totally matched (such that its predecessors will hopefully be redirected and the block removed). */ - if ((nmatch < PARAM_VALUE (PARAM_MIN_CROSSJUMP_INSNS)) + if ((nmatch < PARAM_VALUE (PARAM_MIN_CROSSJUMP_INSNS) && (newpos1 != BB_HEAD (src1))) + || PARAM_VALUE (PARAM_MIN_CROSSJUMP_INSNS) == 0) return false; /* Avoid deleting preserve label when redirecting ABNORMAL edges. */