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

[Kai Tietz <ktietz70 at googlemail dot com>]

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.

Regards,
Kai

Attachment: truth_op_gimplify.txt
Description: Text document