| Message ID | 20130402114657.GK20616@tucnak.redhat.com |
|---|---|
| State | New |
| Headers | show |
On Tue, 2 Apr 2013, Jakub Jelinek wrote: > Hi! > > On the (undefined behavior) testcase below, we end up with > then_bb ending with __builtin_unreachable () at the tree level, therefore > no successor at the RTL level, and else_bb being EXIT_BLOCK_PTR (i.e. > conditional return before a bb with undefined behavior at the end). > Trying to optimize that into a conditional execution of the then_bb insns > doesn't work, we can't merge the else_bb with then_bb and test_bb in this > case, plus it doesn't look like something that would be desirable to do > (conditional return is surely better). > > Fixed thusly, ok for trunk/4.8? I wonder if /* Make sure IF, THEN, and ELSE, blocks are adjacent. Actually, we get the first condition for free, since we've already asserted that there's a fallthru edge from IF to THEN. Likewise for the && and || blocks, since we checked the FALLTHRU flag, those are already adjacent to the last IF block. */ /* ??? As an enhancement, move the ELSE block. Have to deal with BLOCK notes, if by no other means than backing out the merge if they exist. Sticky enough I don't want to think about it now. */ next = then_bb; if (else_bb && (next = next->next_bb) != else_bb) return FALSE; if ((next = next->next_bb) != join_bb && join_bb != EXIT_BLOCK_PTR) { if (else_bb) join_bb = NULL; else return FALSE; } somehow tries to guard against join_bb == EXIT_BLOCK_PTR but fails. Thus, why not do that explicitely here instead of just in the single case you cover? (I can't see why join_bb could not be set to EXIT_BLOCK_PTR in some weird case) Richard. > 2013-04-02 Jakub Jelinek <jakub@redhat.com> > > PR rtl-optimization/56745 > * ifcvt.c (cond_exec_find_if_block): Don't try to optimize > if then_bb has no successors and else_bb is EXIT_BLOCK_PTR. > > * gcc.c-torture/compile/pr56745.c: New test. > > --- gcc/ifcvt.c.jj 2013-03-05 23:25:15.000000000 +0100 > +++ gcc/ifcvt.c 2013-04-02 12:31:19.476859094 +0200 > @@ -3473,7 +3473,7 @@ cond_exec_find_if_block (struct ce_if_bl > code processing. ??? we should fix this in the future. */ > if (EDGE_COUNT (then_bb->succs) == 0) > { > - if (single_pred_p (else_bb)) > + if (single_pred_p (else_bb) && else_bb != EXIT_BLOCK_PTR) > { > rtx last_insn = BB_END (then_bb); > > --- gcc/testsuite/gcc.c-torture/compile/pr56745.c.jj 2013-04-02 12:38:54.718258527 +0200 > +++ gcc/testsuite/gcc.c-torture/compile/pr56745.c 2013-04-02 12:33:22.000000000 +0200 > @@ -0,0 +1,15 @@ > +/* PR rtl-optimization/56745 */ > + > +unsigned char a[6]; > + > +void > +foo () > +{ > + int i; > + for (i = 5; i >= 0; i++) > + { > + if (++a[i] != 0) > + break; > + ++a[i]; > + } > +} > > Jakub > >
On Tue, Apr 02, 2013 at 02:57:13PM +0200, Richard Biener wrote: > On Tue, 2 Apr 2013, Jakub Jelinek wrote: > > On the (undefined behavior) testcase below, we end up with > > then_bb ending with __builtin_unreachable () at the tree level, therefore > > no successor at the RTL level, and else_bb being EXIT_BLOCK_PTR (i.e. > > conditional return before a bb with undefined behavior at the end). > > Trying to optimize that into a conditional execution of the then_bb insns > > doesn't work, we can't merge the else_bb with then_bb and test_bb in this > > case, plus it doesn't look like something that would be desirable to do > > (conditional return is surely better). > > > > Fixed thusly, ok for trunk/4.8? > > I wonder if > > /* Make sure IF, THEN, and ELSE, blocks are adjacent. Actually, we get > the > first condition for free, since we've already asserted that there's a > fallthru edge from IF to THEN. Likewise for the && and || blocks, > since > we checked the FALLTHRU flag, those are already adjacent to the last > IF > block. */ > /* ??? As an enhancement, move the ELSE block. Have to deal with > BLOCK notes, if by no other means than backing out the merge if they > exist. Sticky enough I don't want to think about it now. */ > next = then_bb; > if (else_bb && (next = next->next_bb) != else_bb) > return FALSE; > if ((next = next->next_bb) != join_bb && join_bb != EXIT_BLOCK_PTR) > { > if (else_bb) > join_bb = NULL; > else > return FALSE; > } > > somehow tries to guard against join_bb == EXIT_BLOCK_PTR but fails. > Thus, why not do that explicitely here instead of just in the > single case you cover? (I can't see why join_bb could not be > set to EXIT_BLOCK_PTR in some weird case) From my reading of the code, it can handle the normal case where join_bb is EXIT_BLOCK_PTR just fine, provided that single_succ (then_bb) == join_bb and !else_bb || single_succ (else_bb) == join_bb. The ICE is there only because of the extra optimization I've tweaked, the problem is there that then_bb has no successors and join_bb is EXIT_BLOCK_PTR, so while then_bb can be successfully merged together with test_bb, it has no successor and as join_bb is EXIT_BLOCK_PTR, we just give up. If then_bb has no successor and join_bb isn't EXIT_BLOCK_PTR, we'd normally do: else if (EDGE_COUNT (join_bb->preds) < 2 && join_bb != EXIT_BLOCK_PTR) { /* We can merge the JOIN cleanly and update the dataflow try again on this pass.*/ merge_blocks (combo_bb, join_bb); num_true_changes++; } and all is fine, and if then_bb (and else_bb if it exists) has a single successor of join_bb, all is fine too. Jakub
On Tue, 2 Apr 2013, Jakub Jelinek wrote: > On Tue, Apr 02, 2013 at 02:57:13PM +0200, Richard Biener wrote: > > On Tue, 2 Apr 2013, Jakub Jelinek wrote: > > > On the (undefined behavior) testcase below, we end up with > > > then_bb ending with __builtin_unreachable () at the tree level, therefore > > > no successor at the RTL level, and else_bb being EXIT_BLOCK_PTR (i.e. > > > conditional return before a bb with undefined behavior at the end). > > > Trying to optimize that into a conditional execution of the then_bb insns > > > doesn't work, we can't merge the else_bb with then_bb and test_bb in this > > > case, plus it doesn't look like something that would be desirable to do > > > (conditional return is surely better). > > > > > > Fixed thusly, ok for trunk/4.8? > > > > I wonder if > > > > /* Make sure IF, THEN, and ELSE, blocks are adjacent. Actually, we get > > the > > first condition for free, since we've already asserted that there's a > > fallthru edge from IF to THEN. Likewise for the && and || blocks, > > since > > we checked the FALLTHRU flag, those are already adjacent to the last > > IF > > block. */ > > /* ??? As an enhancement, move the ELSE block. Have to deal with > > BLOCK notes, if by no other means than backing out the merge if they > > exist. Sticky enough I don't want to think about it now. */ > > next = then_bb; > > if (else_bb && (next = next->next_bb) != else_bb) > > return FALSE; > > if ((next = next->next_bb) != join_bb && join_bb != EXIT_BLOCK_PTR) > > { > > if (else_bb) > > join_bb = NULL; > > else > > return FALSE; > > } > > > > somehow tries to guard against join_bb == EXIT_BLOCK_PTR but fails. > > Thus, why not do that explicitely here instead of just in the > > single case you cover? (I can't see why join_bb could not be > > set to EXIT_BLOCK_PTR in some weird case) > > From my reading of the code, it can handle the normal case where > join_bb is EXIT_BLOCK_PTR just fine, provided that single_succ (then_bb) > == join_bb and !else_bb || single_succ (else_bb) == join_bb. > The ICE is there only because of the extra optimization I've tweaked, > the problem is there that then_bb has no successors and join_bb is > EXIT_BLOCK_PTR, so while then_bb can be successfully merged together with > test_bb, it has no successor and as join_bb is EXIT_BLOCK_PTR, we just give > up. If then_bb has no successor and join_bb isn't EXIT_BLOCK_PTR, we'd > normally do: > else if (EDGE_COUNT (join_bb->preds) < 2 > && join_bb != EXIT_BLOCK_PTR) > { > /* We can merge the JOIN cleanly and update the dataflow try > again on this pass.*/ > merge_blocks (combo_bb, join_bb); > num_true_changes++; > } > and all is fine, and if then_bb (and else_bb if it exists) has a single > successor of join_bb, all is fine too. Ah, I see. The patch is ok then. Thanks, Richard.
--- gcc/ifcvt.c.jj 2013-03-05 23:25:15.000000000 +0100 +++ gcc/ifcvt.c 2013-04-02 12:31:19.476859094 +0200 @@ -3473,7 +3473,7 @@ cond_exec_find_if_block (struct ce_if_bl code processing. ??? we should fix this in the future. */ if (EDGE_COUNT (then_bb->succs) == 0) { - if (single_pred_p (else_bb)) + if (single_pred_p (else_bb) && else_bb != EXIT_BLOCK_PTR) { rtx last_insn = BB_END (then_bb); --- gcc/testsuite/gcc.c-torture/compile/pr56745.c.jj 2013-04-02 12:38:54.718258527 +0200 +++ gcc/testsuite/gcc.c-torture/compile/pr56745.c 2013-04-02 12:33:22.000000000 +0200 @@ -0,0 +1,15 @@ +/* PR rtl-optimization/56745 */ + +unsigned char a[6]; + +void +foo () +{ + int i; + for (i = 5; i >= 0; i++) + { + if (++a[i] != 0) + break; + ++a[i]; + } +}