diff mbox series

Use precision and sign to compare types for ranges - update

Message ID f7537f76-2716-6832-35ac-caf0dd523584@redhat.com
State New
Headers show
Series Use precision and sign to compare types for ranges - update | expand

Commit Message

Andrew MacLeod Oct. 22, 2020, 8:52 p.m. UTC 
On 10/22/20 3:16 PM, Andrew MacLeod via Gcc-patches wrote:
> 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


Sorry for missing the regression.. it was there, it just snuck by me in 
the noise :-P

THis seems to resolve the issue on my end, and its the right thing.

Bootstrapped on x86_64-pc-linux-gnu, no regressions, for SURE this time, 
pushed.

Comments

Eric Botcazou Oct. 22, 2020, 10:08 p.m. UTC | #1 
> Sorry for missing the regression.. it was there, it just snuck by me in
> the noise :-P
> 
> THis seems to resolve the issue on my end, and its the right thing.
> 
> Bootstrapped on x86_64-pc-linux-gnu, no regressions, for SURE this time,
> pushed.

No problem, thanks for the quick turnaround!
diff mbox series

Patch

Author: Andrew MacLeod <amacleod@redhat.com>
Date:   Thu Oct 22 15:39:37 2020 -0400

    Use precision and sign to compare types for ranges
    
    Updated to only use range_compatible_p in range assert sanity checks,
    not for actual type cmpatibility.
    
            * gimple-range-gori.cc (is_gimple_logical_p): Use types_compatible_p
            for logical compatibility.
            (logical_stmt_cache::cacheable_p): Ditto.

diff --git a/gcc/gimple-range-gori.cc b/gcc/gimple-range-gori.cc
index 983f4c97e87..5d50b111d2a 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 (range_compatible_p (TREE_TYPE (gimple_assign_rhs1 (gs)),
+	  if (types_compatible_p (TREE_TYPE (gimple_assign_rhs1 (gs)),
 				  boolean_type_node))
 	    return true;
 	  break;
@@ -1165,7 +1165,7 @@  bool
 logical_stmt_cache::cacheable_p (gimple *stmt, const irange *lhs_range) const
 {
   if (gimple_code (stmt) == GIMPLE_ASSIGN
-      && range_compatible_p (TREE_TYPE (gimple_assign_lhs (stmt)),
+      && types_compatible_p (TREE_TYPE (gimple_assign_lhs (stmt)),
 			     boolean_type_node)
       && TREE_CODE (gimple_assign_rhs1 (stmt)) == SSA_NAME)
     {