Re: [patch gimplifier]: Boolify more strict conditional expressions and transform simple form to binary

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

On Tue, May 10, 2011 at 5:08 PM, Kai Tietz <ktietz70@googlemail.com> wrote:
> 2011/5/10 Richard Guenther <richard.guenther@gmail.com>:
>> On Tue, May 10, 2011 at 3:56 PM, Kai Tietz <ktietz70@googlemail.com> wrote:
>>> Hello,
>>>
>>> this patch converts TRUTH_AND_EXPR, TRUTH_OR_EXPR, and TRUTH_XOR_EXPR
>>> expressions
>>> on gimplification to their binary form.  Additionally it takes care
>>> that conditions
>>> are getting boolified for operation.
>>>
>>> ChangeLog
>>>
>>> 2011-05-10  Kai Tietz
>>>
>>>        * gimplify.c (gimplify_exit_expr): Boolify conditional
>>>        expression part.
>>>        (shortcut_cond_r): Likewise.
>>>        (shortcut_cond_expr): Likewise.
>>>        (gimplify_cond_expr): Likewise.
>>>        (gimplify_modify_expr_rhs): Likewise.
>>>        (gimplify_boolean_expr): Likewise.
>>>        (gimple_boolify): Boolify operands for BOOLEAN typed
>>>        base expressions.
>>>        (gimplify_expr): Boolify TRUTH_ANDIF_EXPR, TRUTH_ORIF_EXPR,
>>>        TRUTH_AND_EXPR, TRUTH_OR_EXPR, and TRUTH_XOR_EXPR. Additionally
>>>        move TRUTH_AND|OR|XOR_EXPR to its binary form.
>>>
>>> Tested for x86_64-w64-mingw32 and x86_64-pc-linux-gnu. Ok for apply?
>>
>> Comments inline (the attachment makes my comments harder to spot,
>> look close).
>>
>> Index: gcc/gcc/gimplify.c
>> ===================================================================
>> --- gcc.orig/gcc/gimplify.c     2011-05-10 15:44:49.000000000 +0200
>> +++ gcc/gcc/gimplify.c  2011-05-10 15:46:58.365473600 +0200
>> @@ -1664,10 +1664,12 @@ build_and_jump (tree *label_p)
>>  static enum gimplify_status
>>  gimplify_exit_expr (tree *expr_p)
>>  {
>> -  tree cond = TREE_OPERAND (*expr_p, 0);
>> -  tree expr;
>> +  tree cond, expr;
>>
>> +  TREE_OPERAND (*expr_p, 0) = gimple_boolify (TREE_OPERAND (*expr_p, 0));
>> +  cond = TREE_OPERAND (*expr_p, 0);
>>
>> Why do you need to boolify things at all when we build a COND_EXPR
>> that gets re-gimplified anyway?  I'm confused by this (similar to other
>> places you do that).  I would expect that you at most would do this
>> when gimplifying COND_EXPRs, not when you build them.
>>
>>   expr = build_and_jump (&gimplify_ctxp->exit_label);
>> +
>>   expr = build3 (COND_EXPR, void_type_node, cond, expr, NULL_TREE);
>>   *expr_p = expr;
>>
>> @@ -2586,6 +2588,7 @@ shortcut_cond_r (tree pred, tree *true_l
>>       /* Keep the original source location on the first 'if'.  Set the source
>>         location of the ? on the second 'if'.  */
>>       new_locus = EXPR_HAS_LOCATION (pred) ? EXPR_LOCATION (pred) : locus;
>> +      TREE_OPERAND (pred, 0) = gimple_boolify (TREE_OPERAND (pred, 0));
>>       expr = build3 (COND_EXPR, void_type_node, TREE_OPERAND (pred, 0),
>>                     shortcut_cond_r (TREE_OPERAND (pred, 1), true_label_p,
>>                                      false_label_p, locus),
>> @@ -2594,7 +2597,7 @@ shortcut_cond_r (tree pred, tree *true_l
>>     }
>>   else
>>     {
>> -      expr = build3 (COND_EXPR, void_type_node, pred,
>> +      expr = build3 (COND_EXPR, void_type_node, gimple_boolify (pred),
>>                     build_and_jump (true_label_p),
>>                     build_and_jump (false_label_p));
>>       SET_EXPR_LOCATION (expr, locus);
>> @@ -2625,7 +2628,8 @@ shortcut_cond_expr (tree expr)
>>   bool emit_end, emit_false, jump_over_else;
>>   bool then_se = then_ && TREE_SIDE_EFFECTS (then_);
>>   bool else_se = else_ && TREE_SIDE_EFFECTS (else_);
>> -
>> +
>> +  pred = TREE_OPERAND (expr, 0) = gimple_boolify (TREE_OPERAND (expr, 0));
>>   /* First do simple transformations.  */
>>   if (!else_se)
>>     {
>> @@ -2665,7 +2669,7 @@ shortcut_cond_expr (tree expr)
>>            SET_EXPR_LOCATION (expr, EXPR_LOCATION (pred));
>>          else_ = shortcut_cond_expr (expr);
>>          else_se = else_ && TREE_SIDE_EFFECTS (else_);
>> -         pred = TREE_OPERAND (pred, 0);
>> +         pred = gimple_boolify (TREE_OPERAND (pred, 0));
>>          expr = build3 (COND_EXPR, void_type_node, pred, NULL_TREE, else_);
>>          SET_EXPR_LOCATION (expr, locus);
>>        }
>> @@ -2824,9 +2828,6 @@ gimple_boolify (tree expr)
>>        }
>>     }
>>
>> -  if (TREE_CODE (type) == BOOLEAN_TYPE)
>> -    return expr;
>> -
>>   switch (TREE_CODE (expr))
>>     {
>>     case TRUTH_AND_EXPR:
>> @@ -2851,6 +2852,8 @@ gimple_boolify (tree expr)
>>     default:
>>       /* Other expressions that get here must have boolean values, but
>>         might need to be converted to the appropriate mode.  */
>> +      if (TREE_CODE (type) == BOOLEAN_TYPE)
>> +       return expr;
>>
>> Why do you need to move this?
>>
>>       return fold_convert_loc (loc, boolean_type_node, expr);
>>     }
>>  }
>> @@ -2865,7 +2868,7 @@ gimplify_pure_cond_expr (tree *expr_p, g
>>   enum gimplify_status ret, tret;
>>   enum tree_code code;
>>
>> -  cond = gimple_boolify (COND_EXPR_COND (expr));
>> +  cond = COND_EXPR_COND (expr) = gimple_boolify (COND_EXPR_COND (expr));
>>
>> why change COND_EXPR_COND?
>>
>>   /* We need to handle && and || specially, as their gimplification
>>      creates pure cond_expr, thus leading to an infinite cycle otherwise.  */
>> @@ -2937,6 +2940,7 @@ gimplify_cond_expr (tree *expr_p, gimple
>>   enum tree_code pred_code;
>>   gimple_seq seq = NULL;
>>
>> +  TREE_OPERAND (*expr_p, 0) = gimple_boolify (TREE_OPERAND (*expr_p, 0));
>>
>> So you are doing it here - why do it everywhere else?  I'd like to see
>> this change at exactly _one_ place.
>>
>>   /* If this COND_EXPR has a value, copy the values into a temporary within
>>      the arms.  */
>>   if (!VOID_TYPE_P (type))
>> @@ -4276,6 +4280,7 @@ gimplify_modify_expr_rhs (tree *expr_p,
>>                                            false);
>>
>>        case COND_EXPR:
>> +
>>          /* If we're assigning to a non-register type, push the assignment
>>             down into the branches.  This is mandatory for ADDRESSABLE types,
>>             since we cannot generate temporaries for such, but it saves a
>> @@ -4287,6 +4292,7 @@ gimplify_modify_expr_rhs (tree *expr_p,
>>              tree cond = *from_p;
>>              tree result = *to_p;
>>
>> +             TREE_OPERAND (cond, 0) = gimple_boolify (TREE_OPERAND (cond, 0));
>>              ret = gimplify_expr (&result, pre_p, post_p,
>>                                   is_gimple_lvalue, fb_lvalue);
>>              if (ret != GS_ERROR)
>> @@ -4710,6 +4716,7 @@ gimplify_boolean_expr (tree *expr_p, loc
>>  {
>>   /* Preserve the original type of the expression.  */
>>   tree type = TREE_TYPE (*expr_p);
>> +  *expr_p = gimple_boolify (*expr_p);
>>
>>   *expr_p = build3 (COND_EXPR, type, *expr_p,
>>                    fold_convert_loc (locus, type, boolean_true_node),
>> @@ -6762,6 +6769,13 @@ gimplify_expr (tree *expr_p, gimple_seq
>>
>>        case TRUTH_ANDIF_EXPR:
>>        case TRUTH_ORIF_EXPR:
>> +         if (TREE_CODE (TREE_TYPE (*expr_p)) != BOOLEAN_TYPE)
>> +           {
>> +             tree type = TREE_TYPE (*expr_p);
>> +             *expr_p = fold_convert (type, gimple_boolify (*expr_p));
>> +             ret = GS_OK;
>> +             break;
>> +           }
>>
>> Huh, that doesn't make sense.  Why convert back to the original
>> type?
>>
>> I know COND_EXPRs don't have a boolean value type-wise, but
>> they do have one semantically.  It looks like you are just papering
>> over the inconsistencies at a lot of places instead of trying to fix them
>> during gimplification.
>>
>>          /* Pass the source location of the outer expression.  */
>>          ret = gimplify_boolean_expr (expr_p, saved_location);
>>          break;
>> @@ -7203,6 +7217,30 @@ gimplify_expr (tree *expr_p, gimple_seq
>>        case TRUTH_AND_EXPR:
>>        case TRUTH_OR_EXPR:
>>        case TRUTH_XOR_EXPR:
>> +         if (TREE_CODE (TREE_TYPE (*expr_p)) != BOOLEAN_TYPE)
>> +           {
>> +             tree type = TREE_TYPE (*expr_p);
>> +             *expr_p = fold_convert (type, gimple_boolify (*expr_p));
>> +             ret = GS_OK;
>> +             break;
>> +           }
>> +         /* Call it to make sure that operands are boolified, too. */
>> +         *expr_p = gimple_boolify (*expr_p);
>> +         switch (TREE_CODE (*expr_p))
>> +           {
>> +           case TRUTH_AND_EXPR:
>> +             TREE_SET_CODE (*expr_p, BIT_AND_EXPR);
>> +             break;
>> +           case TRUTH_OR_EXPR:
>> +             TREE_SET_CODE (*expr_p, BIT_IOR_EXPR);
>> +             break;
>> +           case TRUTH_XOR_EXPR:
>> +             TREE_SET_CODE (*expr_p, BIT_XOR_EXPR);
>> +             break;
>> +           default:
>> +             break;
>> +           }
>> +
>>          /* Classified as tcc_expression.  */
>>          goto expr_2;
>>
>> Please also change verify_gimple_assign_binary to barf on
>> the TRUTH_* codes.  We don't want other passes re-introducing them.
>>
>> Richard.
>
> ChangeLog
>
> 2011-05-10  Kai Tietz
>
>        * tree-cfg.c (verify_gimple_assign_binary): Barf on
>        TRUTH_AND_EXPR, TRUTH_OR_EXPR, and TRUTH_XOR_EXPR.
>        * gimplify.c (gimplify_exit_expr): Boolify conditional
>        expression part.
>        (shortcut_cond_r): Likewise.
>        (shortcut_cond_expr): Likewise.
>        (gimplify_cond_expr): Likewise.
>        (gimplify_modify_expr_rhs): Likewise.
>        (gimplify_boolean_expr): Likewise.
>        (gimple_boolify): Boolify operands for BOOLEAN typed
>        base expressions.
>        (gimplify_expr): Boolify TRUTH_ANDIF_EXPR, TRUTH_ORIF_EXPR,
>        TRUTH_AND_EXPR, TRUTH_OR_EXPR, and TRUTH_XOR_EXPR. Additionally
>        move TRUTH_AND|OR|XOR_EXPR to its binary form.
>
> The issue about this gimplification is just in case of freshly
> generated COND_EXPR and making sure its generated conditional is
> boolified before creating. As late we might run exactly in those
> retype-cast for conditional-part being result.  The real pain are the
> comparison expressions, which getting generated in some cases with
> integer types, as they are often used as an implicit type cast.
> The re-type type-casts I introduced after seeing exactly for
> ANDIF/ORIF and TRUTH AND/OR/XOR the appearance of resulting types not
> boolean.

But you know they are boolean in the sense that they are
truth_value_p, that is they result from an operation that has
a boolean result.  Take advantage of that (thus, a truncation
is never necessary).

This doesn't answer all my remarks.  I know that comparisons are
badly typed (fold happily folds integer conversions into the comparison
type, but likely the FEs also create those directly).  So, the gimplifier
already has to deal with this case.  Thus, you shouldn't need to sprinkle
explicit handling all over the place.

I suppose you have testcases for all the cases you looked at, please
add some that cover these corner cases.

Also I see middle-end pieces that still call (fold_)build2 (TRUTH_ ...
eventually re-gimplifying the stuff, so it might not show up with
testing.  Those should be changed to build the BIT_ variant.

Richard.

> Regards,
> Kai
>