diff mbox

[PR,tree-optimization/64823] Handle threading through blocks with PHIs, but no statements V2

Message ID 54E25A70.6020601@redhat.com
State New
Headers show

Commit Message

Jeff Law Feb. 16, 2015, 9 p.m. UTC
The prior version of this patch failed to bootstrap with some 
non-standard configure options on x86_64-unknown-linux-gnu.  The problem 
was existing code which looked for the last statement in a block.  It 
should have looked through non-debug insns which was a trivial change to 
use gsi_last_nondebug_bb in one more place.

Bootstrapped and regression tested on x86_64-unknown-linux-gnu with HJ's 
options.  Installed on the trunk.  Will obviously keep my eye out for 
any new issues.

Jeff
commit 4c181d63db537424b28e5d022f6cbec53594ac8f
Author: Jeff Law <law@redhat.com>
Date:   Mon Feb 16 13:54:35 2015 -0700

    	PR tree-optimization/64823
    	* tree-vrp.c (identify_jump_threads): Handle blocks with no real
    	statements.
    	* tree-ssa-threadedge.c (potentially_threadable_block): Allow
    	threading through blocks with PHIs, but no statements.
    	(thread_through_normal_block): Distinguish between blocks where
    	we did not process all the statements and blocks with no statements.
    
    	PR tree-optimization/64823
    	* gcc.dg/uninit-20.c: New test.

Comments

Jakub Jelinek Feb. 16, 2015, 9:11 p.m. UTC | #1
On Mon, Feb 16, 2015 at 02:00:32PM -0700, Jeff Law wrote:
> --- a/gcc/tree-vrp.c
> +++ b/gcc/tree-vrp.c
> @@ -10176,13 +10176,20 @@ identify_jump_threads (void)
>        /* We only care about blocks ending in a COND_EXPR.  While there
>  	 may be some value in handling SWITCH_EXPR here, I doubt it's
>  	 terribly important.  */
> -      last = gsi_stmt (gsi_last_bb (bb));
> +      last = gsi_stmt (gsi_last_nondebug_bb (bb));

Isn't that just
	last = last_stmt (bb);
equivalent?

	Jakub
Richard Biener Feb. 16, 2015, 9:20 p.m. UTC | #2
On February 16, 2015 10:11:07 PM CET, Jakub Jelinek <jakub@redhat.com> wrote:
>On Mon, Feb 16, 2015 at 02:00:32PM -0700, Jeff Law wrote:
>> --- a/gcc/tree-vrp.c
>> +++ b/gcc/tree-vrp.c
>> @@ -10176,13 +10176,20 @@ identify_jump_threads (void)
>>        /* We only care about blocks ending in a COND_EXPR.  While
>there
>>  	 may be some value in handling SWITCH_EXPR here, I doubt it's
>>  	 terribly important.  */
>> -      last = gsi_stmt (gsi_last_bb (bb));
>> +      last = gsi_stmt (gsi_last_nondebug_bb (bb));

And if the comment is correct then it should not even matter as the condition ends a basic block.

Richard.

>Isn't that just
>	last = last_stmt (bb);
>equivalent?
>
>	Jakub
Jakub Jelinek Feb. 16, 2015, 9:22 p.m. UTC | #3
On Mon, Feb 16, 2015 at 10:20:23PM +0100, Richard Biener wrote:
> On February 16, 2015 10:11:07 PM CET, Jakub Jelinek <jakub@redhat.com> wrote:
> >On Mon, Feb 16, 2015 at 02:00:32PM -0700, Jeff Law wrote:
> >> --- a/gcc/tree-vrp.c
> >> +++ b/gcc/tree-vrp.c
> >> @@ -10176,13 +10176,20 @@ identify_jump_threads (void)
> >>        /* We only care about blocks ending in a COND_EXPR.  While
> >there
> >>  	 may be some value in handling SWITCH_EXPR here, I doubt it's
> >>  	 terribly important.  */
> >> -      last = gsi_stmt (gsi_last_bb (bb));
> >> +      last = gsi_stmt (gsi_last_nondebug_bb (bb));
> 
> And if the comment is correct then it should not even matter as the condition ends a basic block.

It matters, because the use is:
      if (!last
          || gimple_code (last) == GIMPLE_SWITCH
          || (gimple_code (last) == GIMPLE_COND
              && TREE_CODE (gimple_cond_lhs (last)) == SSA_NAME
              && (INTEGRAL_TYPE_P (TREE_TYPE (gimple_cond_lhs (last)))
                  || POINTER_TYPE_P (TREE_TYPE (gimple_cond_lhs (last))))
              && (TREE_CODE (gimple_cond_rhs (last)) == SSA_NAME
                  || is_gimple_min_invariant (gimple_cond_rhs (last)))))
thus, if a bb contains only debug statements and nothing else,
the condition is false, while if it for corresponding -g0 doesn't contain
anything, it is true (!last).

	Jakub
Jeff Law Feb. 17, 2015, 1:09 a.m. UTC | #4
On 02/16/15 14:11, Jakub Jelinek wrote:
> On Mon, Feb 16, 2015 at 02:00:32PM -0700, Jeff Law wrote:
>> --- a/gcc/tree-vrp.c
>> +++ b/gcc/tree-vrp.c
>> @@ -10176,13 +10176,20 @@ identify_jump_threads (void)
>>         /* We only care about blocks ending in a COND_EXPR.  While there
>>   	 may be some value in handling SWITCH_EXPR here, I doubt it's
>>   	 terribly important.  */
>> -      last = gsi_stmt (gsi_last_bb (bb));
>> +      last = gsi_stmt (gsi_last_nondebug_bb (bb));
>
> Isn't that just
> 	last = last_stmt (bb);
> equivalent?
It is.  I'll make that change and update the comment as well.

jeff
diff mbox

Patch

diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index 9ef0d8c..bbeee3f 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,13 @@ 
+2015-02-16  Jeff Law  <law@redhat.com>
+
+	PR tree-optimization/64823
+	* tree-vrp.c (identify_jump_threads): Handle blocks with no real
+	statements.
+	* tree-ssa-threadedge.c (potentially_threadable_block): Allow
+	threading through blocks with PHIs, but no statements.
+	(thread_through_normal_block): Distinguish between blocks where
+	we did not process all the statements and blocks with no statements.
+
 2015-02-16  Jakub Jelinek  <jakub@redhat.com>
 	    James Greenhalgh  <james.greenhalgh@arm.com>
 
diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog
index d5769b7..06ed820 100644
--- a/gcc/testsuite/ChangeLog
+++ b/gcc/testsuite/ChangeLog
@@ -1,3 +1,8 @@ 
+2015-02-16  Jeff Law  <law@redhat.com>
+
+	PR tree-optimization/64823
+	* gcc.dg/uninit-20.c: New test.
+
 2015-02-16  Jakub Jelinek  <jakub@redhat.com>
 	    James Greenhalgh  <james.greenhalgh@arm.com>
 
diff --git a/gcc/testsuite/gcc.dg/uninit-20.c b/gcc/testsuite/gcc.dg/uninit-20.c
new file mode 100644
index 0000000..12001ae
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/uninit-20.c
@@ -0,0 +1,18 @@ 
+/* Spurious uninitialized variable warnings, from gdb */
+/* { dg-do compile } */
+/* { dg-options "-O2 -Wuninitialized" } */
+struct os { struct o *o; };
+struct o { struct o *next; struct os *se; };
+void f(struct o *o){
+  struct os *s;
+  if(o) s = o->se;
+  while(o && s == o->se){
+    s++; // here `o' is non-zero and thus s is initialized
+    s == o->se  // `?' is essential, `if' does not trigger the warning
+      ? (o = o->next, o ? s = o->se : 0)
+      : 0;
+  }
+}
+
+
+
diff --git a/gcc/tree-ssa-threadedge.c b/gcc/tree-ssa-threadedge.c
index 4f83991..7187d06 100644
--- a/gcc/tree-ssa-threadedge.c
+++ b/gcc/tree-ssa-threadedge.c
@@ -110,6 +110,15 @@  potentially_threadable_block (basic_block bb)
 {
   gimple_stmt_iterator gsi;
 
+  /* Special case.  We can get blocks that are forwarders, but are
+     not optimized away because they forward from outside a loop
+     to the loop header.   We want to thread through them as we can
+     sometimes thread to the loop exit, which is obviously profitable. 
+     the interesting case here is when the block has PHIs.  */
+  if (gsi_end_p (gsi_start_nondebug_bb (bb))
+      && !gsi_end_p (gsi_start_phis (bb)))
+    return true;
+  
   /* If BB has a single successor or a single predecessor, then
      there is no threading opportunity.  */
   if (single_succ_p (bb) || single_pred_p (bb))
@@ -1281,16 +1290,32 @@  thread_through_normal_block (edge e,
     = record_temporary_equivalences_from_stmts_at_dest (e, stack, simplify,
 							*backedge_seen_p);
 
-  /* If we didn't look at all the statements, the most likely reason is
-     there were too many and thus duplicating this block is not profitable.
+  /* There's two reasons STMT might be null, and distinguishing
+     between them is important.
 
-     Also note if we do not look at all the statements, then we may not
-     have invalidated equivalences that are no longer valid if we threaded
-     around a loop.  Thus we must signal to our caller that this block
-     is not suitable for use as a joiner in a threading path.  */
+     First the block may not have had any statements.  For example, it
+     might have some PHIs and unconditionally transfer control elsewhere.
+     Such blocks are suitable for jump threading, particularly as a
+     joiner block.
+
+     The second reason would be if we did not process all the statements
+     in the block (because there were too many to make duplicating the
+     block profitable.   If we did not look at all the statements, then
+     we may not have invalidated everything needing invalidation.  Thus
+     we must signal to our caller that this block is not suitable for
+     use as a joiner in a threading path.  */
   if (!stmt)
-    return -1;
+    {
+      /* First case.  The statement simply doesn't have any instructions, but
+	 does have PHIs.  */
+      if (gsi_end_p (gsi_start_nondebug_bb (e->dest))
+	  && !gsi_end_p (gsi_start_phis (e->dest)))
+	return 0;
 
+      /* Second case.  */
+      return -1;
+    }
+  
   /* If we stopped at a COND_EXPR or SWITCH_EXPR, see if we know which arm
      will be taken.  */
   if (gimple_code (stmt) == GIMPLE_COND
diff --git a/gcc/tree-vrp.c b/gcc/tree-vrp.c
index dad1830..d996bcc 100644
--- a/gcc/tree-vrp.c
+++ b/gcc/tree-vrp.c
@@ -10176,13 +10176,20 @@  identify_jump_threads (void)
       /* We only care about blocks ending in a COND_EXPR.  While there
 	 may be some value in handling SWITCH_EXPR here, I doubt it's
 	 terribly important.  */
-      last = gsi_stmt (gsi_last_bb (bb));
+      last = gsi_stmt (gsi_last_nondebug_bb (bb));
 
       /* We're basically looking for a switch or any kind of conditional with
 	 integral or pointer type arguments.  Note the type of the second
 	 argument will be the same as the first argument, so no need to
-	 check it explicitly.  */
-      if (gimple_code (last) == GIMPLE_SWITCH
+	 check it explicitly. 
+
+	 We also handle the case where there are no statements in the
+	 block.  This come up with forwarder blocks that are not
+	 optimized away because they lead to a loop header.  But we do
+	 want to thread through them as we can sometimes thread to the
+	 loop exit which is obviously profitable.  */
+      if (!last
+	  || gimple_code (last) == GIMPLE_SWITCH
 	  || (gimple_code (last) == GIMPLE_COND
       	      && TREE_CODE (gimple_cond_lhs (last)) == SSA_NAME
 	      && (INTEGRAL_TYPE_P (TREE_TYPE (gimple_cond_lhs (last)))