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

[Kai Tietz <ktietz70@googlemail.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