diff mbox

[PR,tree-optimization/65917] Record both equivalences from if (x == y) style conditionals

Message ID 56B84F3B.7040502@redhat.com
State New
Headers show

Commit Message

Jeff Law Feb. 8, 2016, 8:18 a.m. UTC
This turns out to be far easier than expected.  Given a conditional like 
x == y, we already record the canonicalized x = y equivalence.  If we 
just record y = x then this "just works".

The only tricky thing is the following of the SSA_NAME_VALUE chain for 
the source argument -- we need to avoid that when recording the y = x 
variant (since following x's value chain would lead back to y).  That's 
easily resolved with an _raw variant which doesn't follow the source 
value chain.

While working through the code, I saw the old comment WRT loop depth and 
PR 61757 in record_equality.  With the code to follow backedges in the 
CFG gone from the old threader, that code is no longer needed.  So I 
removed it, restoring Richi's cleanup work from early 2015.

Bootstrapped & regression tested on x86-linux.  Installed on the trunk.

Jeff
commit 122fa7e277f132dcfef5d66378f7ef3411ae8b84
Author: law <law@138bc75d-0d04-0410-961f-82ee72b054a4>
Date:   Mon Feb 8 08:17:32 2016 +0000

    	PR tree-optimization/65917
    	* tree-ssa-dom.c (record_temporary_equivalences): Record both
    	equivalences from if (x == y) style conditionals.
    	(loop_depth_of_name): Remove.
    	(record_equality): Remove loop depth check.
    	* tree-ssa-scopedtables.h (const_and_copies): Refine comments.
    	(const_and_copies::record_const_or_copy_raw): New member function.
    	* tree-ssa-scopedtables.c
    	(const_and_copies::record_const_or_copy_raw): New, factored out of
    	(const_and_copies::record_const_or_copy): Call new member function.
    
            PR tree-optimization/65917
    	* gcc.dg/tree-ssa/20030922-2.c: No longer xfailed.
    
    git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/trunk@233207 138bc75d-0d04-0410-961f-82ee72b054a4

Comments

Jeff Law April 27, 2016, 8:52 p.m. UTC | #1
On 02/09/2016 12:55 AM, Bernhard Reutner-Fischer wrote:
> On February 8, 2016 9:18:03 AM GMT+01:00, Jeff Law <law@redhat.com> wrote:
>>
>> This turns out to be far easier than expected.  Given a conditional
>> like
>> x == y, we already record the canonicalized x = y equivalence.  If we
>> just record y = x then this "just works".
>>
>> The only tricky thing is the following of the SSA_NAME_VALUE chain for
>> the source argument -- we need to avoid that when recording the y = x
>> variant (since following x's value chain would lead back to y).  That's
>>
>> easily resolved with an _raw variant which doesn't follow the source
>> value chain.
>>
>> While working through the code, I saw the old comment WRT loop depth
>> and
>> PR 61757 in record_equality.  With the code to follow backedges in the
>> CFG gone from the old threader, that code is no longer needed.  So I
>> removed it, restoring Richi's cleanup work from early 2015.
>>
>> Bootstrapped & regression tested on x86-linux.  Installed on the trunk.
>
> +      /* We already recorded that LHS = RHS, with canonicalization,
> +	 value chain following, etc.
> +
> +	 We also want to return RHS = LHS, but without any canonicalization
>
> Just curious and for my education, we want to *record*, not to return, don't we?
Correct.  Thanks for pointing it out.  I've just fixed this typo on the 
trunk.

jeff
diff mbox

Patch

diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index a465156..2c4a8b6 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,17 @@ 
+2016-02-08  Jeff Law  <law@redhat.com>
+
+	PR tree-optimization/65917
+	* tree-ssa-dom.c (record_temporary_equivalences): Record both
+	equivalences from if (x == y) style conditionals.
+	(loop_depth_of_name): Remove.
+	(record_equality): Remove loop depth check.
+	* tree-ssa-scopedtables.h (const_and_copies): Refine comments.
+	(const_and_copies::record_const_or_copy_raw): New member function.
+	* tree-ssa-scopedtables.c
+	(const_and_copies::record_const_or_copy_raw): New, factored out of
+	(const_and_copies::record_const_or_copy): Call new member function.
+	
+
 2016-02-05  Jeff Law  <law@redhat.com>
 
 	PR tree-optimization/68541
diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog
index 8d3438e..6940136 100644
--- a/gcc/testsuite/ChangeLog
+++ b/gcc/testsuite/ChangeLog
@@ -1,3 +1,8 @@ 
+2016-02-08  Jeff Law  <law@redhat.com>
+
+        PR tree-optimization/65917
+	* gcc.dg/tree-ssa/20030922-2.c: No longer xfailed.
+
 2016-02-07  Jerry DeLisle  <jvdelisle@gcc.gnu.org>
 
 	PR fortran/50555
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/20030922-2.c b/gcc/testsuite/gcc.dg/tree-ssa/20030922-2.c
index 6c133bd..16c79da 100644
--- a/gcc/testsuite/gcc.dg/tree-ssa/20030922-2.c
+++ b/gcc/testsuite/gcc.dg/tree-ssa/20030922-2.c
@@ -20,8 +20,4 @@  rgn_rank (rtx insn1, rtx insn2)
 }
 
 /* There should be two IF conditionals.  */
-/* This now fails as it requires a very specific decision of DOM which
-   SSA name to record as a copy of the other when DOM derives copies
-   from temporary equivalences.  The heuristics there no longer do
-   the correct thing.  VRP still optimizes this testcase.  */
-/* { dg-final { scan-tree-dump-times "if " 2 "dom2" { xfail *-*-* } } } */
+/* { dg-final { scan-tree-dump-times "if " 2 "dom2" } } */
diff --git a/gcc/tree-ssa-dom.c b/gcc/tree-ssa-dom.c
index b690d92..f44ac13 100644
--- a/gcc/tree-ssa-dom.c
+++ b/gcc/tree-ssa-dom.c
@@ -917,6 +917,15 @@  record_temporary_equivalences (edge e,
       tree rhs = edge_info->rhs;
       record_equality (lhs, rhs, const_and_copies);
 
+      /* We already recorded that LHS = RHS, with canonicalization,
+	 value chain following, etc.
+
+	 We also want to return RHS = LHS, but without any canonicalization
+	 or value chain following.  */
+      if (TREE_CODE (rhs) == SSA_NAME)
+	const_and_copies->record_const_or_copy_raw (rhs, lhs,
+						    SSA_NAME_VALUE (rhs));
+
       /* If LHS is an SSA_NAME and RHS is a constant integer and LHS was
 	 set via a widening type conversion, then we may be able to record
 	 additional equivalences.  */
@@ -1161,33 +1170,6 @@  record_cond (cond_equivalence *p,
     delete element;
 }
 
-/* Return the loop depth of the basic block of the defining statement of X.
-   This number should not be treated as absolutely correct because the loop
-   information may not be completely up-to-date when dom runs.  However, it
-   will be relatively correct, and as more passes are taught to keep loop info
-   up to date, the result will become more and more accurate.  */
-
-static int
-loop_depth_of_name (tree x)
-{
-  gimple *defstmt;
-  basic_block defbb;
-
-  /* If it's not an SSA_NAME, we have no clue where the definition is.  */
-  if (TREE_CODE (x) != SSA_NAME)
-    return 0;
-
-  /* Otherwise return the loop depth of the defining statement's bb.
-     Note that there may not actually be a bb for this statement, if the
-     ssa_name is live on entry.  */
-  defstmt = SSA_NAME_DEF_STMT (x);
-  defbb = gimple_bb (defstmt);
-  if (!defbb)
-    return 0;
-
-  return bb_loop_depth (defbb);
-}
-
 /* Similarly, but assume that X and Y are the two operands of an EQ_EXPR.
    This constrains the cases in which we may treat this as assignment.  */
 
@@ -1224,10 +1206,7 @@  record_equality (tree x, tree y, class const_and_copies *const_and_copies)
      long as we canonicalize on one value.  */
   if (is_gimple_min_invariant (y))
     ;
-  else if (is_gimple_min_invariant (x)
-	   /* ???  When threading over backedges the following is important
-	      for correctness.  See PR61757.  */
-	   || (loop_depth_of_name (x) < loop_depth_of_name (y)))
+  else if (is_gimple_min_invariant (x))
     prev_x = x, x = y, y = prev_x, prev_x = prev_y;
   else if (prev_x && is_gimple_min_invariant (prev_x))
     x = y, y = prev_x, prev_x = prev_y;
diff --git a/gcc/tree-ssa-scopedtables.c b/gcc/tree-ssa-scopedtables.c
index 613f50b..b18978e 100644
--- a/gcc/tree-ssa-scopedtables.c
+++ b/gcc/tree-ssa-scopedtables.c
@@ -700,6 +700,28 @@  const_and_copies::pop_to_marker (void)
     }
 }
 
+/* Record that X has the value Y and that X's previous value is PREV_X. 
+
+   This variant does not follow the value chain for Y.  */
+
+void
+const_and_copies::record_const_or_copy_raw (tree x, tree y, tree prev_x)
+{
+  if (dump_file && (dump_flags & TDF_DETAILS))
+    {
+      fprintf (dump_file, "0>>> COPY ");
+      print_generic_expr (dump_file, x, 0);
+      fprintf (dump_file, " = ");
+      print_generic_expr (dump_file, y, 0);
+      fprintf (dump_file, "\n");
+    }
+
+  set_ssa_name_value (x, y);
+  m_stack.reserve (2);
+  m_stack.quick_push (prev_x);
+  m_stack.quick_push (x);
+}
+
 /* Record that X has the value Y.  */
 
 void
@@ -708,7 +730,9 @@  const_and_copies::record_const_or_copy (tree x, tree y)
   record_const_or_copy (x, y, SSA_NAME_VALUE (x));
 }
 
-/* Record that X has the value Y and that X's previous value is PREV_X.  */
+/* Record that X has the value Y and that X's previous value is PREV_X. 
+
+   This variant follow's Y value chain.  */
 
 void
 const_and_copies::record_const_or_copy (tree x, tree y, tree prev_x)
@@ -720,19 +744,7 @@  const_and_copies::record_const_or_copy (tree x, tree y, tree prev_x)
       y = tmp ? tmp : y;
     }
 
-  if (dump_file && (dump_flags & TDF_DETAILS))
-    {
-      fprintf (dump_file, "0>>> COPY ");
-      print_generic_expr (dump_file, x, 0);
-      fprintf (dump_file, " = ");
-      print_generic_expr (dump_file, y, 0);
-      fprintf (dump_file, "\n");
-    }
-
-  set_ssa_name_value (x, y);
-  m_stack.reserve (2);
-  m_stack.quick_push (prev_x);
-  m_stack.quick_push (x);
+  record_const_or_copy_raw (x, y, prev_x);
 }
 
 bool
diff --git a/gcc/tree-ssa-scopedtables.h b/gcc/tree-ssa-scopedtables.h
index 30f92d8..c933821 100644
--- a/gcc/tree-ssa-scopedtables.h
+++ b/gcc/tree-ssa-scopedtables.h
@@ -161,11 +161,18 @@  class const_and_copies
      was pushed.  */
   void pop_to_marker (void);
 
-  /* Record a single const/copy pair that can be unwound.  */
+  /* Record a single const/copy pair that can be unwound.  This version
+     may follow the value chain for the RHS.  */
   void record_const_or_copy (tree, tree);
 
+  /* Record a single const/copy pair that can be unwound.  This version
+     does not follow the value chain for the RHS.  */
+  void record_const_or_copy_raw (tree, tree, tree);
+
   /* Special entry point when we want to provide an explicit previous
-     value for the first argument.  Try to get rid of this in the future.  */
+     value for the first argument.  Try to get rid of this in the future. 
+
+     This version may also follow the value chain for the RHS.  */
   void record_const_or_copy (tree, tree, tree);
 
  private: