Re: [PR66726] Factor conversion out of COND_EXPR

[Kugan <kugan dot vivekanandarajah at linaro dot org>]

On 24/07/15 05:05, Jeff Law wrote:
> On 07/15/2015 11:52 PM, Kugan wrote:
>>
>>>>
>>>> diff --git a/gcc/tree-ssa-reassoc.c b/gcc/tree-ssa-reassoc.c
>>>> index 932c83a..3058eb5 100644
>>>> --- a/gcc/tree-ssa-reassoc.c
>>>> +++ b/gcc/tree-ssa-reassoc.c
>>>
>>>>        return false;
>>>>      bb = gimple_bb (stmt);
>>>>      if (!single_succ_p (bb))
>>>> @@ -2729,9 +2743,8 @@ final_range_test_p (gimple stmt)
>>>>
>>>>      lhs = gimple_assign_lhs (stmt);
>>>>      rhs = gimple_assign_rhs1 (stmt);
>>>> -  if (!INTEGRAL_TYPE_P (TREE_TYPE (lhs))
>>>> -      || TREE_CODE (rhs) != SSA_NAME
>>>> -      || TREE_CODE (TREE_TYPE (rhs)) != BOOLEAN_TYPE)
>>>> +  if (TREE_CODE (TREE_TYPE (rhs)) != BOOLEAN_TYPE
>>>> +      && TREE_CODE (TREE_TYPE (lhs)) != BOOLEAN_TYPE)
>>>>        return false;
>>> So you're ensuring that one of the two is a boolean...  Note that
>>> previously we ensured that the rhs was a boolean and the lhs was an
>>> integral type (which I believe is true for booleans).
>>>
>>> Thus if we had
>>> bool x;
>>> int y;
>>>
>>> x = (bool) y;
>>>
>>> The old code would have rejected that case.  But I think it gets through
>>> now, right?
>>>
>>> I think once that issue is addressed, this will be good for the trunk.
>>>
>>
>> Thanks for the review. How about:
>>
>> -  if (!INTEGRAL_TYPE_P (TREE_TYPE (lhs))
>> -      || TREE_CODE (rhs) != SSA_NAME
>> -      || TREE_CODE (TREE_TYPE (rhs)) != BOOLEAN_TYPE)
>> +  if (gimple_assign_cast_p (stmt)
>> +      && (!INTEGRAL_TYPE_P (TREE_TYPE (lhs))
>> +      || TREE_CODE (rhs) != SSA_NAME
>> +      || TREE_CODE (TREE_TYPE (rhs)) != BOOLEAN_TYPE))
> But then I think you need to verify that for the  _234 = a_2(D) == 2;
> case that type of the RHS is a boolean.
> 
> ie, each case has requirements for the types.  I don't think they can be
> reasonably unified.  So something like this:
> 
> if (gimple_assign_cast_p (stmt)
>     && ! (correct types for cast)
>    return false;
> 
> if (!gimple_assign_cast_p (stmt)
>     && ! (correct types for tcc_comparison case))
>   return false;
> 
> 
> This works because we've already verified that it's either a type
> conversion or a comparison on the RHS.
>
I thought that when !gimple_assign_cast_p (stmt), RHS will always
boolean. I have now added this check in the attached patch.

I also noticed that in maybe_optimize_range_tests, GIMPLE_COND can
have non compatible types when new_op is updated
(boolean types coming from tcc_compare results) and hence need to be
converted. Changed that as well.

Bootstrapped and regression tested on x86-64-none-linux-gnu with no new
regressions. Is this OK for trunk?

Thanks,
Kugan

Attachment: p.txt
Description: Text document