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.
Regards,
Kai
