2011/5/11 Richard Guenther <richard.guent...@gmail.com>:
> On Tue, May 10, 2011 at 9:26 PM, Kai Tietz <ktiet...@googlemail.com> wrote:
>> 2011/5/10 Kai Tietz <ktiet...@googlemail.com>:
>>> 2011/5/10 Richard Guenther <richard.guent...@gmail.com>:
>>>> On Tue, May 10, 2011 at 5:30 PM, Paolo Bonzini <bonz...@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
