diff mbox series

Use precision and sign to compare types for ranges

Message ID 29040f71-dacd-f887-6dcb-e7f307e0e010@redhat.com
State New
Headers show
Series Use precision and sign to compare types for ranges | expand

Commit Message

Andrew MacLeod Oct. 19, 2020, 11:28 p.m. UTC 
This fixes the second test case in pr 97360.

There are a few places in the ranger where we sanity check the types of 
the ranges.  We were using types_compatible_p() but thats not really 
acccurate as gimple allows types which are useless_type_conversion_p() 
in only one direction, whereas types_compatible_p() requires casts in 
both directions to be useless_type_conversion_p().

And, since its only a sanity check, ranges really only require that the 
precision and sign be the same, so its a faster check anyway.

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

Andrew

Comments

Iain Sandoe Oct. 22, 2020, 3:17 p.m. UTC | #1 
Hi Andrew,

Andrew MacLeod via Gcc-patches <gcc-patches@gcc.gnu.org> wrote:

> This fixes the second test case in pr 97360.
>
> There are a few places in the ranger where we sanity check the types of  
> the ranges.  We were using types_compatible_p() but thats not really  
> acccurate as gimple allows types which are useless_type_conversion_p() in  
> only one direction, whereas types_compatible_p() requires casts in both  
> directions to be useless_type_conversion_p().
>
> And, since its only a sanity check, ranges really only require that the  
> precision and sign be the same, so its a faster check anyway.
>
> bootstrapped on x86_64-pc-linux-gnu, no regressions, pushed.

Just as a heads up ...

This regresses ACATs  C35507N on at least all X86 Darwin I’ve tried.
Both 32 and 64Bit hosts fail (not had the chance to test on power darwin  
yet, because that’s got bootstrap issues).

The sympom is :

,.,. C35507N ACATS 2.5 20-10-22 02:23:12
---- C35507N CHECK THAT THE ATTRIBUTES 'POS' AND 'VAL' YIELD THE CORRECT
                 RESULTS WHEN THE PREFIX IS A FORMAL DISCRETE TYPE WHOSE
                 ACTUAL PARAMETER IS A CHARACTER TYPE WITH AN ENUMERATION
                 REPRESENTATION CLAUSE.
    * C35507N INCORRECT VALUE FOR CHAR'POS OF 'A'.
    * C35507N INCORRECT VALUE FOR CHAR'VAL OF CHARACTER IN POSITION -  1.

The last two lines repeat until the test times out.

====

... acats are not the easiest to debug :/ .. will try to get something more  
constructive to
put in a PR.

Iain
Eric Botcazou Oct. 22, 2020, 4:30 p.m. UTC | #2 
> Just as a heads up ...
> 
> This regresses ACATs  C35507N on at least all X86 Darwin I’ve tried.
> Both 32 and 64Bit hosts fail (not had the chance to test on power darwin
> yet, because that’s got bootstrap issues).

I have attached a reproducer, compile it with

  gnatmake p -O2

and

  gnatmake p -O2 -fno-tree-vrp

to see the regression.
Eric Botcazou Oct. 22, 2020, 4:53 p.m. UTC | #3 
> There are a few places in the ranger where we sanity check the types of
> the ranges.  We were using types_compatible_p() but thats not really
> acccurate as gimple allows types which are useless_type_conversion_p()
> in only one direction, whereas types_compatible_p() requires casts in
> both directions to be useless_type_conversion_p().
> 
> And, since its only a sanity check, ranges really only require that the
> precision and sign be the same, so its a faster check anyway.
> 
> bootstrapped on x86_64-pc-linux-gnu, no regressions, pushed.

The Ada regression comes from this hunk:

diff --git a/gcc/gimple-range-gori.cc b/gcc/gimple-range-gori.cc
index c4bfc658319..983f4c97e87 100644
--- a/gcc/gimple-range-gori.cc
+++ b/gcc/gimple-range-gori.cc
@@ -552,7 +552,7 @@ is_gimple_logical_p (const gimple *gs)
 	case BIT_AND_EXPR:
 	case BIT_IOR_EXPR:
 	  // Bitwise operations on single bits are logical too.
-	  if (types_compatible_p (TREE_TYPE (gimple_assign_rhs1 (gs)),
+	  if (range_compatible_p (TREE_TYPE (gimple_assign_rhs1 (gs)),
 				  boolean_type_node))
 	    return true;
 	  break;

which overlooks that boolean_type_node may have a precision not equal to 1 
(it's 8 in Ada).  See useless_type_conversion_p which has:

      /* Preserve conversions to/from BOOLEAN_TYPE if types are not
	 of precision one.  */
      if (((TREE_CODE (inner_type) == BOOLEAN_TYPE)
	   != (TREE_CODE (outer_type) == BOOLEAN_TYPE))
	  && TYPE_PRECISION (outer_type) != 1)
	return false
Andrew MacLeod Oct. 22, 2020, 7:16 p.m. UTC | #4 
On 10/22/20 12:53 PM, Eric Botcazou wrote:
>> There are a few places in the ranger where we sanity check the types of
>> the ranges.  We were using types_compatible_p() but thats not really
>> acccurate as gimple allows types which are useless_type_conversion_p()
>> in only one direction, whereas types_compatible_p() requires casts in
>> both directions to be useless_type_conversion_p().
>>
>> And, since its only a sanity check, ranges really only require that the
>> precision and sign be the same, so its a faster check anyway.
>>
>> bootstrapped on x86_64-pc-linux-gnu, no regressions, pushed.
> The Ada regression comes from this hunk:
>
> diff --git a/gcc/gimple-range-gori.cc b/gcc/gimple-range-gori.cc
> index c4bfc658319..983f4c97e87 100644
> --- a/gcc/gimple-range-gori.cc
> +++ b/gcc/gimple-range-gori.cc
> @@ -552,7 +552,7 @@ is_gimple_logical_p (const gimple *gs)
>   	case BIT_AND_EXPR:
>   	case BIT_IOR_EXPR:
>   	  // Bitwise operations on single bits are logical too.
> -	  if (types_compatible_p (TREE_TYPE (gimple_assign_rhs1 (gs)),
> +	  if (range_compatible_p (TREE_TYPE (gimple_assign_rhs1 (gs)),
>   				  boolean_type_node))
>   	    return true;
>   	  break;
>
> which overlooks that boolean_type_node may have a precision not equal to 1
> (it's 8 in Ada).  See useless_type_conversion_p which has:
>
>        /* Preserve conversions to/from BOOLEAN_TYPE if types are not
> 	 of precision one.  */
>        if (((TREE_CODE (inner_type) == BOOLEAN_TYPE)
> 	   != (TREE_CODE (outer_type) == BOOLEAN_TYPE))
> 	  && TYPE_PRECISION (outer_type) != 1)
> 	return false
>
bah.  And I cant seem to reproduce it on my machine with your 
reproducer, but I am seeing the other result it in my log.    Point is 
still taken tho.
range_compatible_p should only be used during asserts as a sanity check 
for range interactions, not during actual code processing like that.

im currently testing the following, which reverts 2 places (both which 
check for logicals)  to use types_compatible_p instead.  The remaining 
uses are in range assertion code. This seems to resolve the original 
problem in my log.

Thanks for reducing it to the problematic change.  I'm verifying a new 
failure in libgomp isnt a result of this before I check it in.

Andrew
diff mbox series

Patch


	gcc/ChangeLog:

	PR tree-optimization/97360
	* gimple-range.h (range_compatible_p): New.
	* gimple-range-gori.cc (is_gimple_logical_p): Use range_compatible_p.
	(range_is_either_true_or_false): Ditto.
	(gori_compute::outgoing_edge_range_p): Cast result to the correct
	type if necessary.
	(logical_stmt_cache::cacheable_p): Use range_compatible_p.
	* gimple-range.cc (gimple_ranger::calc_stmt): Check range_compatible_p
	before casting the range.
	(gimple_ranger::range_on_exit): Use range_compatible_p.
	(gimple_ranger::range_on_edge): Ditto.

	gcc/testsuite/ChangeLog:

	* gcc.dg/pr97360-2.c: New test.
	

diff --git a/gcc/gimple-range-gori.cc b/gcc/gimple-range-gori.cc
index c4bfc658319..983f4c97e87 100644
--- a/gcc/gimple-range-gori.cc
+++ b/gcc/gimple-range-gori.cc
@@ -552,7 +552,7 @@  is_gimple_logical_p (const gimple *gs)
 	case BIT_AND_EXPR:
 	case BIT_IOR_EXPR:
 	  // Bitwise operations on single bits are logical too.
-	  if (types_compatible_p (TREE_TYPE (gimple_assign_rhs1 (gs)),
+	  if (range_compatible_p (TREE_TYPE (gimple_assign_rhs1 (gs)),
 				  boolean_type_node))
 	    return true;
 	  break;
@@ -618,7 +618,7 @@  range_is_either_true_or_false (const irange &r)
   // This is complicated by the fact that Ada has multi-bit booleans,
   // so true can be ~[0, 0] (i.e. [1,MAX]).
   tree type = r.type ();
-  gcc_checking_assert (types_compatible_p (type, boolean_type_node));
+  gcc_checking_assert (range_compatible_p (type, boolean_type_node));
   return (r.singleton_p () || !r.contains_p (build_zero_cst (type)));
 }
 
@@ -999,11 +999,20 @@  gori_compute::outgoing_edge_range_p (irange &r, edge e, tree name)
 
   // If NAME can be calculated on the edge, use that.
   if (m_gori_map->is_export_p (name, e->src))
-    return compute_operand_range (r, stmt, lhs, name);
-
-  // Otherwise see if NAME is derived from something that can be
-  // calculated.  This performs no dynamic lookups whatsover, so it is
-  // low cost.
+    {
+      if (compute_operand_range (r, stmt, lhs, name))
+	{
+	  // Sometimes compatible types get interchanged. See PR97360.
+	  // Make sure we are returning the type of the thing we asked for.
+	  if (!r.undefined_p () && r.type () != TREE_TYPE (name))
+	    {
+	      gcc_checking_assert (range_compatible_p (r.type (),
+						       TREE_TYPE (name)));
+	      range_cast (r, TREE_TYPE (name));
+	    }
+	  return true;
+	}
+    }
   return false;
 }
 
@@ -1156,7 +1165,7 @@  bool
 logical_stmt_cache::cacheable_p (gimple *stmt, const irange *lhs_range) const
 {
   if (gimple_code (stmt) == GIMPLE_ASSIGN
-      && types_compatible_p (TREE_TYPE (gimple_assign_lhs (stmt)),
+      && range_compatible_p (TREE_TYPE (gimple_assign_lhs (stmt)),
 			     boolean_type_node)
       && TREE_CODE (gimple_assign_rhs1 (stmt)) == SSA_NAME)
     {
diff --git a/gcc/gimple-range.cc b/gcc/gimple-range.cc
index 999d631c5ee..e4864ba60f6 100644
--- a/gcc/gimple-range.cc
+++ b/gcc/gimple-range.cc
@@ -392,8 +392,14 @@  gimple_ranger::calc_stmt (irange &r, gimple *s, tree name)
     {
       if (r.undefined_p ())
 	return true;
+      // We sometimes get compatible types copied from operands, make sure
+      // the correct type is being returned.
       if (name && TREE_TYPE (name) != r.type ())
-	range_cast (r, TREE_TYPE (name));
+	{
+	  gcc_checking_assert (range_compatible_p (r.type (),
+						   TREE_TYPE (name)));
+	  range_cast (r, TREE_TYPE (name));
+	}
       return true;
     }
   return false;
@@ -928,7 +934,7 @@  gimple_ranger::range_on_exit (irange &r, basic_block bb, tree name)
   else
     gcc_assert (range_of_expr (r, name, s));
   gcc_checking_assert (r.undefined_p ()
-		       || types_compatible_p (r.type(), TREE_TYPE (name)));
+		       || range_compatible_p (r.type (), TREE_TYPE (name)));
 }
 
 // Calculate a range for NAME on edge E and return it in R.
@@ -948,7 +954,7 @@  gimple_ranger::range_on_edge (irange &r, edge e, tree name)
 
   range_on_exit (r, e->src, name);
   gcc_checking_assert  (r.undefined_p ()
-			|| types_compatible_p (r.type(), TREE_TYPE (name)));
+			|| range_compatible_p (r.type(), TREE_TYPE (name)));
 
   // Check to see if NAME is defined on edge e.
   if (m_cache.outgoing_edge_range_p (edge_range, e, name))
diff --git a/gcc/gimple-range.h b/gcc/gimple-range.h
index 041dc7c2a97..a6e8793f284 100644
--- a/gcc/gimple-range.h
+++ b/gcc/gimple-range.h
@@ -115,6 +115,18 @@  gimple_range_ssa_p (tree exp)
   return NULL_TREE;
 }
 
+// Return true if TYPE1 and TYPE2 are compatible range types.
+
+static inline bool
+range_compatible_p (tree type1, tree type2)
+{
+  // types_compatible_p requires conversion in both directions to be useless.
+  // GIMPLE only requires a cast one way in order to be compatible.
+  // Ranges really only need the sign and precision to be the same.
+  return (TYPE_PRECISION (type1) == TYPE_PRECISION (type2)
+	  && TYPE_SIGN (type1) == TYPE_SIGN (type2));
+}
+
 // Return the legacy GCC global range for NAME if it has one, otherwise
 // return VARYING.
 
diff --git a/gcc/testsuite/gcc.dg/pr97360-2.c b/gcc/testsuite/gcc.dg/pr97360-2.c
new file mode 100644
index 00000000000..48aebf1b100
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr97360-2.c
@@ -0,0 +1,14 @@ 
+/* { dg-do compile } */
+/* { dg-options "-O2 " } */
+
+void *a;
+void *b(void);
+void *e(void);
+
+void *
+c() {
+  void *d;
+  if (d == b && e())
+    d = a;
+  return d;
+}