diff mbox series

Fix logical_combine OR operation. Again.

Message ID 6d3383e2-5c10-53dc-0673-b3925fe961ea@redhat.com
State New
Headers show
Series Fix logical_combine OR operation. Again. | expand

Commit Message

Andrew MacLeod Nov. 10, 2020, 1:01 a.m. UTC 
Doh.

The original fix for PR97657 was incorrect.  It papered over a problem 
by reducing opportunities it could find. Given

   if (c_2 || c_3)

If the FALSE edge is taken, this is ! (c_2 || c_3) which is equivalent 
to !c_2 && !c_3.. so performing the intersection as combine_logical was 
originally doing was correct.

I found this by examining cases  we were missing because this was no 
longer being combined properly.  which sent be back to this PR to figure 
out what the real reason for the failure was.

The GORI design specification calculates outgoing ranges using 
dependency chains, but as soon as the chain goes outside the current 
block, we are suppose to revert to using the on-entry range since 
control flow could dictate further range changes.

I had noticed that on certain occasions we'd peek into other blocks, and 
each time I saw it, it was beneficial and seemed like a harmless 
recalucaltion, So I let it go.

In this particular case, during the on-entry cache propagation we were 
peeking into another block to pick up a value used in the logical OR 
operation. Unfortunately, the on-entry cache hadnt finished propagating 
and the incomplete range was picked up from that other block instead of 
the current one, and we ended up calculating a [0,0] range on an 
outgoing edge that should have been VARYING.  And bad things happened.

The fix is to patch the logical evaluation code to do the same thing as 
the non-logical code, follow the spec, and simply use the range-on-entry 
value for the block if the def chain  leads out of the current block.   
Next release will integrate the full re-evaluation of def chains from 
outside the basic block.. properly planned :-)

I've added an additional test case to confirm that the logical or FALSE 
case is being optimized properly. The original fix fails this test, the 
current fix passes it,.

Bootstrapped on x86_64-pc-linux-gnu, no regressions. pushed.

Andrew
diff mbox series

Patch

commit 7d26a337bfa1135d95caa3c213e82f2a97f18a01
Author: Andrew MacLeod <amacleod@redhat.com>
Date:   Mon Nov 9 19:38:22 2020 -0500

    Fix logical_combine OR operation. Again.
    
    The original fix was incorrect and results in loss of opportunities.
    Revert the original fix. When processing logical chains, do not
    follow chains outside of the current basic block.  Use the import
    value instead.
    
            gcc/
            PR tree-optimization/97567
            * gimple-range-gori.cc: (gori_compute::logical_combine): False
            OR operations should intersect the 2 results.
            (gori_compute::compute_logical_operands_in_chain): If def chains
            are outside the current basic block, don't follow them.
            gcc/testsuite/
            * gcc.dg/pr97567-2.c: New.

diff --git a/gcc/gimple-range-gori.cc b/gcc/gimple-range-gori.cc
index 54385baa629..af3609e414e 100644
--- a/gcc/gimple-range-gori.cc
+++ b/gcc/gimple-range-gori.cc
@@ -730,10 +730,10 @@  gori_compute::logical_combine (irange &r, enum tree_code code,
         if (lhs.zero_p ())
 	  {
 	    // An OR operation will only take the FALSE path if both
-	    // operands are false, so either [20, 255] or [0, 5] is the
-	    // union: [0,5][20,255].
+	    // operands are false simlulateously, which means they should
+	    // be intersected.  !(x || y) == !x && !y
 	    r = op1.false_range;
-	    r.union_ (op2.false_range);
+	    r.intersect (op2.false_range);
 	  }
 	else
 	  {
@@ -804,9 +804,12 @@  gori_compute::compute_logical_operands_in_chain (tf_range &range,
 						 tree name,
 						 tree op, bool op_in_chain)
 {
-  if (!op_in_chain)
+  gimple *src_stmt = gimple_range_ssa_p (op) ? SSA_NAME_DEF_STMT (op) : NULL;
+  basic_block bb = gimple_bb (stmt);
+  if (!op_in_chain || (src_stmt != NULL && bb != gimple_bb (src_stmt)))
     {
-      // If op is not in chain, use its known value.
+      // If op is not in the def chain, or defined in this block,
+      // use its known value on entry to the block.
       expr_range_in_bb (range.true_range, name, gimple_bb (stmt));
       range.false_range = range.true_range;
       return;
@@ -814,14 +817,12 @@  gori_compute::compute_logical_operands_in_chain (tf_range &range,
   if (optimize_logical_operands (range, stmt, lhs, name, op))
     return;
 
-  // Calulate ranges for true and false on both sides, since the false
+  // Calculate ranges for true and false on both sides, since the false
   // path is not always a simple inversion of the true side.
-  if (!compute_operand_range (range.true_range, SSA_NAME_DEF_STMT (op),
-			      m_bool_one, name))
-    expr_range_in_bb (range.true_range, name, gimple_bb (stmt));
-  if (!compute_operand_range (range.false_range, SSA_NAME_DEF_STMT (op),
-			      m_bool_zero, name))
-    expr_range_in_bb (range.false_range, name, gimple_bb (stmt));
+  if (!compute_operand_range (range.true_range, src_stmt, m_bool_one, name))
+    expr_range_in_bb (range.true_range, name, bb);
+  if (!compute_operand_range (range.false_range, src_stmt, m_bool_zero, name))
+    expr_range_in_bb (range.false_range, name, bb);
 }
 
 // Given a logical STMT, calculate true and false for each potential
diff --git a/gcc/testsuite/gcc.dg/pr97567-2.c b/gcc/testsuite/gcc.dg/pr97567-2.c
new file mode 100644
index 00000000000..dee31c6dc01
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr97567-2.c
@@ -0,0 +1,24 @@ 
+/* { dg-do compile} */
+/* { dg-options "-O2 -fdump-tree-evrp" } */
+
+char a[2];
+
+extern int x;
+
+void foo(void);
+
+signed char g (signed char min, signed char max)
+{
+   signed char i = x;
+   return i < min || max < i ? min : i;
+}
+
+void gg (void)
+{
+   signed char t = g (0, 9);
+   /* Ranger should be able to remove the call to foo ().  */
+   if (t > 9 || t < 0)
+     foo ();
+}
+
+/* { dg-final { scan-tree-dump-not "foo" "evrp" } }  */