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

[Kai Tietz <ktietz70 at googlemail dot com>]

2011/5/11 Richard Guenther <richard.guenther@gmail.com>:
> On Tue, May 10, 2011 at 9:26 PM, Kai Tietz <ktietz70@googlemail.com> wrote:
>> 2011/5/10 Kai Tietz <ktietz70@googlemail.com>:
>>> 2011/5/10 Richard Guenther <richard.guenther@gmail.com>:
>>>> On Tue, May 10, 2011 at 5:30 PM, Paolo Bonzini <bonzini@gnu.org> wrote:
>>>>> On 05/10/2011 05:23 PM, Richard Guenther wrote:
>>>>>>
>>>>>> I suppose you have testcases for all the cases you looked at, please
>>>>>> add some that cover these corner cases.
>>>>>
>>>>> Also, there is quite some tree-vrp.c dead code with these changes. Removing
>>>>> the TRUTH_*_CODE handling in VRP will help finding more places where the
>>>>> middle-end is building boolean operations. ?There should be testcases
>>>>> covering these parts of VRP.
>>>>
>>>> Btw, you can split the patch into two pieces - first, make TRUTH_*
>>>> expressions correctly typed (take boolean typed operands and procude
>>>> a boolean typed result) and verify that in verify_gimple_assign_binary.
>>>> A second patch than can do the s/TRUTH_/BIT_/ substitution during
>>>> gimplification. ?That way the first (and more difficult) part doesn't get
>>>> too big with unrelated changes.
>>>>
>>>> Richard.
>>>>
>>>>> Paolo
>>>>>
>>>>
>>>
>>> Well, I think I found one big issue here about booified expression of
>>> condition. The gimple_boolify needs to handle COND_EXPR in more
>>> detail. As if a conditional expression has to be boolified, it means
>>> its condition and its other operands need to be boolified, too. And
>>> this is for sure one cause, why I see for ANDIF/ORIF and the truth
>>> AND|OR|XOR none boolean types.
>>>
>>> I will continue on that.
>>>
>>> To split this seems to make sense, as I have to touch much more areas
>>> for the TRUTH to BIT conversion.
>>>
>>> Regards,
>>> Kai
>>>
>>
>> So I use this thread for first part of the series of patches. This one
>> improves boolean type-logic
>> during gimplification.
>> To gimple_boolify the handling for COND_EXPR are added, and in general
>> it is tried to do
>> boolean operation just on boolean type. ?As sadly fold-const (and here
>> in special fold_truth_not_expr (), which doesn't provide by default
>> boolean type and uses instead operand-type, which is IMHO a major flaw
>> here. ?But well, there are some comments indicating that this behavior
>> is required due some other quirks. But this is for sure something to
>> be cleaned up) produces truth operations with wrong type, which are in
>> calculations not necessarily identifyable as truth ops anymore, this
>> patch makes sure that for truth AND|OR|XOR original type remains.
>>
>> 2011-05-10 ?Kai Tietz
>>
>> ? ? ? ?* gimplify.c (gimple_boolify): Handle COND_EXPR
>> ? ? ? ?and make sure that even if type is BOOLEAN for
>> ? ? ? ?TRUTH-opcodes the operands getting boolified.
>> ? ? ? ?(gimplify_expr): Boolify operand condition for
>> ? ? ? ?COND_EXPR.
>> ? ? ? ?Boolify truth opcodes AND, ANDIF, OR, ORIF, and XOR. Additional
>> ? ? ? ?take care that we keep expression's type.
>>
>> Ok for apply?
>
> Posting patches inline makes it easier to put in review comments, so
> please consider doing that.

Ok, I think about this. Not sure how well, google-mail handles this.
I'll try next time.

> Index: gcc/gcc/gimplify.c
> ===================================================================
> --- gcc.orig/gcc/gimplify.c ? ? 2011-05-10 18:31:40.000000000 +0200
> +++ gcc/gcc/gimplify.c ?2011-05-10 21:14:49.106340400 +0200
> @@ -2824,11 +2824,20 @@ gimple_boolify (tree expr)
> ? ? ? ?}
> ? ? }
>
> - ?if (TREE_CODE (type) == BOOLEAN_TYPE)
> - ? ?return expr;
> -
> ? switch (TREE_CODE (expr))
> ? ? {
> + ? ?case COND_EXPR:
> + ? ? ?/* If we have a conditional expression, which shall be
> + ? ? ? ? boolean, take care we boolify also their left and right arm. ?*/
> + ? ? ?if (TREE_OPERAND (expr, 2) != NULL_TREE && !VOID_TYPE_P
> (TREE_TYPE (TREE_OPERAND (expr, 2))))
> + ? ? ? ?TREE_OPERAND (expr, 2) = gimple_boolify (TREE_OPERAND (expr, 2));
> + ? ? ?if (TREE_OPERAND (expr, 1) != NULL_TREE && !VOID_TYPE_P
> (TREE_TYPE (TREE_OPERAND (expr, 1))))
> + ? ? ? ?TREE_OPERAND (expr, 1) = gimple_boolify (TREE_OPERAND (expr, 1));
> + ? ? ?TREE_OPERAND (expr, 0) = gimple_boolify (TREE_OPERAND (expr, 0));
> + ? ? ?if (TREE_CODE (type) == BOOLEAN_TYPE)
> + ? ? ? return expr;
> + ? ? ?return fold_convert_loc (loc, boolean_type_node, expr);
> +
>
> That looks like premature optimization. ?Why isn't it enough to
> fold-convert the COND_EXPR itself? ?Thus, I don't see why the
> existing gimple_boolify isn't enough.

See description of recent update patch.  It isn't enough.

> ? ? case TRUTH_AND_EXPR:
> ? ? case TRUTH_OR_EXPR:
> ? ? case TRUTH_XOR_EXPR:
> @@ -2851,6 +2860,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;
> ? ? ? return fold_convert_loc (loc, boolean_type_node, expr);
> ? ? }
> ?}
> @@ -6714,6 +6725,7 @@ gimplify_expr (tree *expr_p, gimple_seq
> ? ? ? ? ?break;
>
> ? ? ? ?case COND_EXPR:
> + ? ? ? ? TREE_OPERAND (*expr_p, 0) = gimple_boolify (TREE_OPERAND (*expr_p, 0));
> ? ? ? ? ?ret = gimplify_cond_expr (expr_p, pre_p, fallback);
>
> This boolification should be done by gimplify_cond_expr as I said
> in the previous review. ?It does already handle this perfectly fine.

No, but this variant is a bit too weak (see again updated patch). As
the COND itself can be TRUTH operation, which otherwise will later on
introduces none-boolified inner ANDIF/ORIF/AND/OR/XOR truth
expressions.


> ? ? ? ? ?/* C99 code may assign to an array in a structure value of a
> @@ -6762,6 +6774,17 @@ gimplify_expr (tree *expr_p, gimple_seq
>
> ? ? ? ?case TRUTH_ANDIF_EXPR:
> ? ? ? ?case TRUTH_ORIF_EXPR:
> + ? ? ? ? /* This shouldn't happen, but due fold-const (and here especially
> + ? ? ? ? ? ?fold_truth_not_expr) happily uses operand type and doesn't
> + ? ? ? ? ? ?automatically uses boolean_type as result, this happens. ?*/
> + ? ? ? ? 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;
> + ? ? ? ? ? }
> + ? ? ? ? *expr_p = gimple_boolify (*expr_p);
>
> This shouldn't be necessary at all. ?Instead gimplify_boolean_expr
> should deal with it - which it magically does by building a COND_EXPR
> which should already deal with all cases just fine.
>
> ? ? ? ? ?/* Pass the source location of the outer expression. ?*/
> ? ? ? ? ?ret = gimplify_boolean_expr (expr_p, saved_location);
> ? ? ? ? ?break;
> @@ -7203,6 +7226,17 @@ gimplify_expr (tree *expr_p, gimple_seq
> ? ? ? ?case TRUTH_AND_EXPR:
> ? ? ? ?case TRUTH_OR_EXPR:
> ? ? ? ?case TRUTH_XOR_EXPR:
> + ? ? ? ? /* This shouldn't happen, but due fold-const (and here especially
> + ? ? ? ? ? ?fold_truth_not_expr) happily uses operand type and doesn't
> + ? ? ? ? ? ?automatically uses boolean_type as result, this happens. ?*/
> + ? ? ? ? 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;
> + ? ? ? ? ? }
> + ? ? ? ? *expr_p = gimple_boolify (*expr_p);
>
> Now this is the only place where we need to do something. ?And it
> nearly looks ok but IMHO should simply do
>
> ? ? ? ? ? ? tree orig_type = TREE_TYPE (*expr_p);
> ? ? ? ? ? ? *expr_p = gimple_boolify (*expr_p);
> ? ? ? ? ? ? if (TREE_CODE (orig_type) != BOOLEAN_TYPE)
> ? ? ? ? ? ? ? {
> ? ? ? ? ? ? ? ? *expr_p = fold_convert (orig_type, *expr_p);
> ? ? ? ? ? ? ? ? ret = GS_OK;
> ? ? ? ? ? ? ? ? break;
> ? ? ? ? ? ? ? }
>
> ? ? ? ? ?/* Classified as tcc_expression. ?*/
> ? ? ? ? ?goto expr_2;
>
> You lack a tree-cfg.c hunk to verify that the TRUTH_* exprs have
> correct types. ?Sth like

Ok, I will add this.  With conditional expression checking, this
should be doable to check here for XOR/AND/OR truth.

> Index: gcc/tree-cfg.c
> ===================================================================
> --- gcc/tree-cfg.c ? ? ?(revision 173611)
> +++ gcc/tree-cfg.c ? ? ?(working copy)
> @@ -3542,9 +3615,9 @@ do_pointer_plus_expr_check:
> ? ? case TRUTH_XOR_EXPR:
> ? ? ? {
> ? ? ? ?/* We allow any kind of integral typed argument and result. ?*/
> - ? ? ? if (!INTEGRAL_TYPE_P (rhs1_type)
> - ? ? ? ? ? || !INTEGRAL_TYPE_P (rhs2_type)
> - ? ? ? ? ? || !INTEGRAL_TYPE_P (lhs_type))
> + ? ? ? if (TREE_CODE (rhs1_type) != BOOLEAN_TYPE
> + ? ? ? ? ? || TREE_CODE (rhs2_type) != BOOLEAN_TYPE
> + ? ? ? ? ? || TREE_CODE (lhs_type) != BOOLEAN_TYPE)
> ? ? ? ? ?{
> ? ? ? ? ? ?error ("type mismatch in binary truth expression");
> ? ? ? ? ? ?debug_generic_expr (lhs_type);
>
>
> Otherwise you wouldn't know whether your patch was complete (how
> did you verify that?).
>
> Richard.
>
>
>> Regards,
>> Kai
>>

Regards,
Kai