2011/5/26 Richard Guenther <richard.guent...@gmail.com>:
> On Thu, May 26, 2011 at 1:28 PM, Kai Tietz <ktiet...@googlemail.com> wrote:
>> 2011/5/26 Richard Guenther <richard.guent...@gmail.com>:
>>> On Thu, May 26, 2011 at 1:20 PM, Kai Tietz <ktiet...@googlemail.com> wrote:
>>>> 2011/5/26 Richard Guenther <richard.guent...@gmail.com>:
>>>>> On Thu, May 26, 2011 at 1:11 PM, Kai Tietz <ktiet...@googlemail.com>
>>>>> wrote:
>>>>>> 2011/5/26 Richard Guenther <richard.guent...@gmail.com>:
>>>>>>> On Thu, May 26, 2011 at 12:46 PM, Richard Guenther
>>>>>>> <richard.guent...@gmail.com> wrote:
>>>>>>>> On Thu, May 26, 2011 at 12:20 PM, Kai Tietz <kti...@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,
>>
>> Well, I spared here the type conversion to boolean_type_node for
>> useless conversion,
>
> useless_type_conversion_p isn't symmetric, so you can't do that.
Right, I am aware of that. I will move in updated patch the setting
boolean before the if here. As type conversion for the cast back to
original-type via fold_convert_loc is only of interest AFAICS, when
this cast isn't useless (means outter is org_type, and inner is
boolean_type_node). But well, we can replace the if here to the check
against org_type != boolean_type_node, if you prefer this.
Kai
