Re: [patch gimplify]: Make sure comparison using boolean-type after gimplification

[Richard Guenther <richard.guenther@gmail.com>]

On Thu, May 26, 2011 at 1:20 PM, Kai Tietz <ktietz70@googlemail.com> wrote:
> 2011/5/26 Richard Guenther <richard.guenther@gmail.com>:
>> On Thu, May 26, 2011 at 1:11 PM, Kai Tietz <ktietz70@googlemail.com> wrote:
>>> 2011/5/26 Richard Guenther <richard.guenther@gmail.com>:
>>>> On Thu, May 26, 2011 at 12:46 PM, Richard Guenther
>>>> <richard.guenther@gmail.com> wrote:
>>>>> On Thu, May 26, 2011 at 12:20 PM, Kai Tietz <ktietz@redhat.com> wrote:
>>>>>> Hello,
>>>>>>
>>>>>> this patch ensures that after gimplification also comparison expressions using FE's boolean_type_node.  As we need to deal here with C/C++'s (obj-c/c++ and java's), Ada's, and Fortran's specific boolean types, this patch alters some checks in tree-cfg for Ada's sake, and we need to deal in fold-const about type-conversion of comparisons special.
>>>>>> Additionally it takes care that in forwprop pass we don't do type hoising for boolean types.
>>>>>>
>>>>>> ChangeLog
>>>>>>
>>>>>> 2011-05-26  Kai Tietz
>>>>>>
>>>>>>          * gimplify.c (gimple_boolify): Boolify all comparison
>>>>>>          expressions.
>>>>>>          (gimplify_expr): Use 'useless_type_conversion_p' for comparing
>>>>>>          org_type with boolean_type_node for TRUTH-expressions and comparisons.
>>>>>>          * fold-const.c (fold_unary_loc): Handle comparison conversions with
>>>>>>          boolean-type special.
>>>>>>          * tree-cfg.c (verify_gimple_comparison): Adjust check for boolean
>>>>>>          or compatible types.
>>>>>>          (verify_gimple_assign_unary): Likewise.
>>>>>>          * tree-ssa-forwprop.c (forward_propagate_comparison): Handle
>>>>>>          boolean case special.
>>>>>>
>>>>>> Tested on x86_64-pc-linux-gnu (multilib) with regression test for all standard languages (C, C++, Obj-C, Fortran, Java) plus Obj-C++ and Ada. Ok for apply?
>>>>>
>>>>> It obviously isn't ok to apply before a patch has gone in that will resolve
>>>>> all of the FAILs you specify.  Comments on the patch:
>>>>>
>>>>> @@ -7281,9 +7284,28 @@ gimplify_expr (tree *expr_p, gimple_seq
>>>>>                 plain wrong if bitfields are involved.  */
>>>>>                {
>>>>>                  tree type = TREE_TYPE (TREE_OPERAND (*expr_p, 1));
>>>>> +                 tree org_type = TREE_TYPE (*expr_p);
>>>>> +
>>>>> +                 if (!useless_type_conversion_p (org_type, boolean_type_node))
>>>>> +                   {
>>>>> +                     TREE_TYPE (*expr_p) = boolean_type_node;
>>>>> +                     *expr_p = fold_convert_loc (saved_location, org_type, *expr_p);
>>>>> +                     ret = GS_OK;
>>>>> +                     goto dont_recalculate;
>>>>> +                   }
>>>>>
>>>>> The above should be only done for !AGGREGATE_TYPE_P.  Probably then
>>>>> the strange dont_recalcuate goto can go away as well.
>>>>>
>>>>>                  if (!AGGREGATE_TYPE_P (type))
>>>>> -                   goto expr_2;
>>>>> +                   {
>>>>> +                     enum gimplify_status r0, r1;
>>>>> +
>>>>> +                     r0 = gimplify_expr (&TREE_OPERAND (*expr_p, 0), pre_p,
>>>>> +                                         post_p, is_gimple_val, fb_rvalue);
>>>>> +                     r1 = gimplify_expr (&TREE_OPERAND (*expr_p, 1), pre_p,
>>>>> +                                         post_p, is_gimple_val, fb_rvalue);
>>>>> +
>>>>> +                     ret = MIN (r0, r1);
>>>>> +                   }
>>>>> +
>>>>>
>>>>> why change this?
>>>>>
>>>>> @@ -7641,6 +7641,12 @@ fold_unary_loc (location_t loc, enum tre
>>>>>        }
>>>>>       else if (COMPARISON_CLASS_P (arg0))
>>>>>        {
>>>>> +         /* Don't optimize type change, if op0 is of kind boolean_type_node.
>>>>> +            Otherwise this will lead to race-condition on gimplification
>>>>> +            trying to boolify comparison expression.  */
>>>>> +         if (TREE_TYPE (op0) == boolean_type_node)
>>>>> +           return NULL_TREE;
>>>>> +
>>>>>          if (TREE_CODE (type) == BOOLEAN_TYPE)
>>>>>            {
>>>>>              arg0 = copy_node (arg0);
>>>>>
>>>>> The code leading here looks quite strange to me ...
>>>>>
>>>>> tree
>>>>> fold_unary_loc (location_t loc, enum tree_code code, tree type, tree op0)
>>>>> {
>>>>> ...
>>>>>  if (TREE_CODE_CLASS (code) == tcc_unary)
>>>>>    {
>>>>> ...
>>>>>      else if (COMPARISON_CLASS_P (arg0))
>>>>>        {
>>>>>          if (TREE_CODE (type) == BOOLEAN_TYPE)
>>>>>            {
>>>>>              arg0 = copy_node (arg0);
>>>>>              TREE_TYPE (arg0) = type;
>>>>>              return arg0;
>>>>>            }
>>>>>          else if (TREE_CODE (type) != INTEGER_TYPE)
>>>>>            return fold_build3_loc (loc, COND_EXPR, type, arg0,
>>>>>                                fold_build1_loc (loc, code, type,
>>>>>                                             integer_one_node),
>>>>>                                fold_build1_loc (loc, code, type,
>>>>>                                             integer_zero_node));
>>>>>        }
>>>>>
>>>>> so, for any tcc_unary, like NEGATE_EXPR, with BOOLEAN_TYPE,
>>>>> return arg0 ... sure.  Same for the 2nd case.  ~ (a == b) isn't
>>>>> the same as a == b ? ~1 : ~0.  I _suppose_ those cases were
>>>>> ment for CONVERT_EXPR_CODE_P (code) instead of all of tcc_unary,
>>>>> in which case they should be dropped or moved below where we
>>>>> handle conversions explicitly.
>>>>>
>>>>> That said - does anyone remember anything about that above code?
>>>>> Trying to do some svn blame history tracking now ...
>>>>
>>>> Oh, the patch continues...
>>>>
>>>> @@ -3208,7 +3208,10 @@ verify_gimple_comparison (tree type, tre
>>>>        && (!POINTER_TYPE_P (op0_type)
>>>>           || !POINTER_TYPE_P (op1_type)
>>>>           || TYPE_MODE (op0_type) != TYPE_MODE (op1_type)))
>>>> -      || !INTEGRAL_TYPE_P (type))
>>>> +      || !(TREE_CODE (type) == BOOLEAN_TYPE
>>>> +          || (TREE_TYPE (type) && TREE_CODE (type) == INTEGER_TYPE
>>>> +              && TREE_CODE (TREE_TYPE (type)) == BOOLEAN_TYPE)
>>>> +          || (INTEGRAL_TYPE_P (type) && TYPE_PRECISION (type) == 1)))
>>>>     {
>>>>
>>>> why that strange TREE_TYPE (TREE_TYPE ())) thing again?  Drop
>>>> that.
>>>>
>>>> @@ -3352,6 +3355,8 @@ verify_gimple_assign_unary (gimple stmt)
>>>>     case TRUTH_NOT_EXPR:
>>>>       /* We require two-valued operand types.  */
>>>>       if (!(TREE_CODE (rhs1_type) == BOOLEAN_TYPE
>>>> +           || (TREE_TYPE (rhs1_type) && TREE_CODE (rhs1_type) == INTEGER_TYPE
>>>> +               && TREE_CODE (TREE_TYPE (rhs1_type)) == BOOLEAN_TYPE)
>>>>            || (INTEGRAL_TYPE_P (rhs1_type)
>>>>                && TYPE_PRECISION (rhs1_type) == 1)))
>>>>         {
>>>>
>>>> likewise.
>>>
>>> Well, those checks are necessary for Ada and its crippled
>>> boolean_type_node and computed boolean-based integer construct.  Ada
>>> derives here the boolean-type to an integer with range 0..1 and the
>>> only way to find out that it is in fact such a beast is by looking
>>> into TREE_TYPE of type.  See here Ada's code for getting base-type
>>> information.
>>> As such things are treated as compatible they can appear for TRUTH_NOT
>>> expressions and comparisons.
>>
>> No they can't as we boolify them.
>>
>> Richard.
>
> Well, they do.  Test can prove this by running fortran's dg tests.
>
> The logic we do in gimplification is:
>
> 1) save original type of expression.
> 2) set expression's type to boolean_type_node.
> 3) if conversion from boolean_type_node to saved type isn't useless
> (and here we have the issue about Fortran's different booleans with
> different modes, which make those kinds not useless) then cast
> expression's result to original type and retry gimplification.
> 4) Otherwise gimplify operands and continue.

You don't make sense.  Btw,

>>>> +                 if (!useless_type_conversion_p (org_type, boolean_type_node))
>>>> +                   {
>>>> +                     TREE_TYPE (*expr_p) = boolean_type_node;
>>>> +                     *expr_p = fold_convert_loc (saved_location, org_type,

is bogus it should be

TREE_TYPE (*expr_p) = boolean_type_node;
if (!useless_type_conversion_p (org_type, boolean_type_node))
 {
    *expr_p = fold_convert_loc (saved_location, org_type,
...




> Regards,
> Kai
>
> Kai
>