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.
Richard.
