Patchwork Fix cond_exec_find_if_block (PR rtl-optimization/56745)

login
register
mail settings
Submitter Jakub Jelinek
Date April 2, 2013, 11:46 a.m.
Message ID <20130402114657.GK20616@tucnak.redhat.com>
Download mbox | patch
Permalink /patch/232968/
State New
Headers show

Comments

Jakub Jelinek - April 2, 2013, 11:46 a.m.
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?

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.


	Jakub
Richard Guenther - April 2, 2013, 12:57 p.m.
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
> 
>
Jakub Jelinek - April 2, 2013, 1:31 p.m.
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
Richard Guenther - April 2, 2013, 1:39 p.m.
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.

Patch

--- 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];
+    }
+}