diff mbox

Improve PHI-OPT when there are multiple phis

Message ID CA+=Sn1m_DiQ8_Fj=4Ytx1pDmd3o8pM-tA-NJBe2Es2BpOQP4hg@mail.gmail.com
State New
Headers show

Commit Message

Andrew Pinski March 10, 2012, 7:55 a.m. UTC
Woops I forgot the patch.

Thanks,
Andrew Pinski

On Fri, Mar 9, 2012 at 11:45 AM, Andrew Pinski
<andrew.pinski@caviumnetworks.com> wrote:
> On Tue, Jan 24, 2012 at 2:50 AM, Richard Guenther
> <richard.guenther@gmail.com> wrote:
>> On Tue, Jan 24, 2012 at 7:34 AM, Andrew Pinski
>> <andrew.pinski@caviumnetworks.com> wrote:
>>> Hi,
>>>  Right now PHI-OPT does try to handle the case where we have multiple
>>> PHIs but the other PHIs have the same value for the edges we care
>>> about.
>>> This fixes the issue and allows PHI-OPT to handle a few more cases and
>>> it removes the TODO in the comments.
>>>
>>> OK For 4.8? Bootstrapped and tested on x86_64-linux-gnu with no regressions.
>>>
>>> Thanks,
>>> Andrew Pinski
>>>
>>> ChangeLog:
>>> * tree-ssa-phiopt.c (gimple_phi_singleton_for_edges): New function.
>>
>> The name is confusing I think, because it returns the single non-singleton
>> PHI for the edge pair ... you can avoid choosing a better name by
>> inlining it to its single call site.  I'd maybe name it
>> single_non_singleton_phi_for_edges.
>>
>> Otherwise ok.
>
> Here is the updated patch with one small change, value_replacement
> uses single_non_singleton_phi_for_edges now too.
>
> OK still?  Bootstrapped and tested on x86_64-linux-gnu with no regressions.
>
>
> Thanks,
> Andrew Pinski
>
>
> ChangeLog:
> * tree-ssa-phiopt.c (single_non_singleton_phi_for_edges): New function.
> (tree_ssa_phiopt_worker): Use single_non_singleton_phi_for_edges.
> (value_replacement): Likewise.
> (empty_block_p): Check also if the PHIs for the block are empty.
>
> testsuite/ChangeLog:
> * gcc.dg/tree-ssa/phi-opt-7.c: New testcase.
>
>
>>
>> Thanks,
>> Richard.
>>
>>> (tree_ssa_phiopt_worker): Use gimple_phi_singleton_for_edges.
>>> (empty_block_p): Check also if the PHIs for the block are empty.
>>>
>>> testsuite/ChangeLog:
>>> * gcc.dg/tree-ssa/phi-opt-7.c: New testcase.

Comments

Richard Biener March 12, 2012, 11:49 a.m. UTC | #1
On Sat, Mar 10, 2012 at 8:55 AM, Andrew Pinski
<andrew.pinski@caviumnetworks.com> wrote:
> Woops I forgot the patch.

Ok.

Thanks,
Richard.

> Thanks,
> Andrew Pinski
>
> On Fri, Mar 9, 2012 at 11:45 AM, Andrew Pinski
> <andrew.pinski@caviumnetworks.com> wrote:
>> On Tue, Jan 24, 2012 at 2:50 AM, Richard Guenther
>> <richard.guenther@gmail.com> wrote:
>>> On Tue, Jan 24, 2012 at 7:34 AM, Andrew Pinski
>>> <andrew.pinski@caviumnetworks.com> wrote:
>>>> Hi,
>>>>  Right now PHI-OPT does try to handle the case where we have multiple
>>>> PHIs but the other PHIs have the same value for the edges we care
>>>> about.
>>>> This fixes the issue and allows PHI-OPT to handle a few more cases and
>>>> it removes the TODO in the comments.
>>>>
>>>> OK For 4.8? Bootstrapped and tested on x86_64-linux-gnu with no regressions.
>>>>
>>>> Thanks,
>>>> Andrew Pinski
>>>>
>>>> ChangeLog:
>>>> * tree-ssa-phiopt.c (gimple_phi_singleton_for_edges): New function.
>>>
>>> The name is confusing I think, because it returns the single non-singleton
>>> PHI for the edge pair ... you can avoid choosing a better name by
>>> inlining it to its single call site.  I'd maybe name it
>>> single_non_singleton_phi_for_edges.
>>>
>>> Otherwise ok.
>>
>> Here is the updated patch with one small change, value_replacement
>> uses single_non_singleton_phi_for_edges now too.
>>
>> OK still?  Bootstrapped and tested on x86_64-linux-gnu with no regressions.
>>
>>
>> Thanks,
>> Andrew Pinski
>>
>>
>> ChangeLog:
>> * tree-ssa-phiopt.c (single_non_singleton_phi_for_edges): New function.
>> (tree_ssa_phiopt_worker): Use single_non_singleton_phi_for_edges.
>> (value_replacement): Likewise.
>> (empty_block_p): Check also if the PHIs for the block are empty.
>>
>> testsuite/ChangeLog:
>> * gcc.dg/tree-ssa/phi-opt-7.c: New testcase.
>>
>>
>>>
>>> Thanks,
>>> Richard.
>>>
>>>> (tree_ssa_phiopt_worker): Use gimple_phi_singleton_for_edges.
>>>> (empty_block_p): Check also if the PHIs for the block are empty.
>>>>
>>>> testsuite/ChangeLog:
>>>> * gcc.dg/tree-ssa/phi-opt-7.c: New testcase.
diff mbox

Patch

Index: testsuite/gcc.dg/tree-ssa/phi-opt-7.c
===================================================================
--- testsuite/gcc.dg/tree-ssa/phi-opt-7.c	(revision 0)
+++ testsuite/gcc.dg/tree-ssa/phi-opt-7.c	(revision 0)
@@ -0,0 +1,23 @@ 
+/* { dg-do compile } */
+/* { dg-options "-O1 -fdump-tree-optimized" } */
+
+int g(int,int);
+int f(int t, int c)
+{
+  int d = 0;
+  int e = 0;
+  if (t)
+    {
+      d = t;
+      if (c) e = 1;
+    }
+  else d = 0, e = 0;
+  return g(d,e);
+}
+
+/* There should be one ifs as one of them should be changed into
+   a conditional and the other should be there still.  */
+/* { dg-final { scan-tree-dump-times "if" 1 "optimized" }  }*/
+/* { dg-final { scan-tree-dump-times "D.\[0-9\]*_\[0-9\]* = c_\[0-9\]*.D. != 0" 1 "optimized"  } } */
+/* { dg-final { cleanup-tree-dump "optimized" } } */
+
Index: testsuite/gcc.dg/tree-ssa/phi-opt-8.c
===================================================================
--- testsuite/gcc.dg/tree-ssa/phi-opt-8.c	(revision 185132)
+++ testsuite/gcc.dg/tree-ssa/phi-opt-8.c	(working copy)
@@ -19,7 +19,7 @@  int f(int t, int c)
    but currently is not as PHI-OPT does not reduce the t PHI as we have
    two phis.  Note this is fixed with
    http://gcc.gnu.org/ml/gcc-patches/2012-01/msg01195.html .  */
-/* { dg-final { scan-tree-dump-not "if" "phiopt1" { xfail *-*-* } } } */
+/* { dg-final { scan-tree-dump-not "if" "phiopt1" } } */
 /* { dg-final { scan-tree-dump "g .t_\[0-9\]*.D.," "optimized" } } */
 /* { dg-final { scan-tree-dump-not "PHI" "optimized" } } */
 /* { dg-final { cleanup-tree-dump "phiopt1" } } */
Index: tree-ssa-phiopt.c
===================================================================
--- tree-ssa-phiopt.c	(revision 185132)
+++ tree-ssa-phiopt.c	(working copy)
@@ -193,6 +193,33 @@  tree_ssa_cs_elim (void)
   return tree_ssa_phiopt_worker (true);
 }
 
+/* Return the singleton PHI in the SEQ of PHIs for edges E0 and E1. */
+
+static gimple
+single_non_singleton_phi_for_edges (gimple_seq seq, edge e0, edge e1)
+{
+  gimple_stmt_iterator i;
+  gimple phi = NULL;
+  if (gimple_seq_singleton_p (seq))
+    return gsi_stmt (gsi_start (seq));
+  for (i = gsi_start (seq); !gsi_end_p (i); gsi_next (&i))
+    {
+      gimple p = gsi_stmt (i);
+      /* If the PHI arguments are equal then we can skip this PHI. */
+      if (operand_equal_for_phi_arg_p (gimple_phi_arg_def (p, e0->dest_idx),
+				       gimple_phi_arg_def (p, e1->dest_idx)))
+	continue;
+
+      /* If we already have a PHI that has the two edge arguments are
+	 different, then return it is not a singleton for these PHIs. */
+      if (phi)
+	return NULL;
+
+      phi = p;
+    }
+  return phi;
+}
+
 /* For conditional store replacement we need a temporary to
    put the old contents of the memory in.  */
 static tree condstoretemp;
@@ -316,6 +343,7 @@  tree_ssa_phiopt_worker (bool do_store_el
 	  gimple_seq phis = phi_nodes (bb2);
 	  gimple_stmt_iterator gsi;
 	  bool candorest = true;
+
 	  /* Value replacement can work with more than one PHI
 	     so try that first. */
 	  for (gsi = gsi_start (phis); !gsi_end_p (gsi); gsi_next (&gsi))
@@ -333,21 +361,8 @@  tree_ssa_phiopt_worker (bool do_store_el
 
 	  if (!candorest)
 	    continue;
-	  /* Check to make sure that there is only one non-virtual PHI node.
-	     TODO: we could do it with more than one iff the other PHI nodes
-	     have the same elements for these two edges.  */
-	  phi = NULL;
-	  for (gsi = gsi_start (phis); !gsi_end_p (gsi); gsi_next (&gsi))
-	    {
-	      if (!is_gimple_reg (gimple_phi_result (gsi_stmt (gsi))))
-		continue;
-	      if (phi)
-		{
-		  phi = NULL;
-		  break;
-		}
-	      phi = gsi_stmt (gsi);
-	    }
+	  
+	  phi = single_non_singleton_phi_for_edges (phis, e1, e2);
 	  if (!phi)
 	    continue;
 
@@ -447,6 +462,8 @@  empty_block_p (basic_block bb)
 {
   /* BB must have no executable statements.  */
   gimple_stmt_iterator gsi = gsi_after_labels (bb);
+  if (phi_nodes (bb))
+    return false;
   if (gsi_end_p (gsi))
     return true;
   if (is_gimple_debug (gsi_stmt (gsi)))
@@ -736,10 +753,11 @@  value_replacement (basic_block cond_bb,
 	arg = arg1;
 
       /* If the middle basic block was empty or is defining the
-	 PHI arguments and this is a singleton phi then we can remove
-         the middle basic block. */
+	 PHI arguments and this is a single phi where the args are different
+	 for the edges e0 and e1 then we can remove the middle basic block. */
       if (emtpy_or_with_defined_p
-	  && gimple_seq_singleton_p (phi_nodes (gimple_bb (phi))))
+	  && single_non_singleton_phi_for_edges (phi_nodes (gimple_bb (phi)),
+							    e0, e1))
 	{
           replace_phi_edge_with_variable (cond_bb, e1, phi, arg);
 	  /* Note that we optimized this PHI.  */
@@ -754,7 +772,8 @@  value_replacement (basic_block cond_bb,
 	    {
 	      fprintf (dump_file, "PHI ");
 	      print_generic_expr (dump_file, gimple_phi_result (phi), 0);
-	      fprintf (dump_file, " reduced for COND_EXPR in block %d to ", cond_bb->index);
+	      fprintf (dump_file, " reduced for COND_EXPR in block %d to ",
+		       cond_bb->index);
 	      print_generic_expr (dump_file, arg, 0);
 	      fprintf (dump_file, ".\n");
             }