On Tue, May 10, 2011 at 9:26 PM, Kai Tietz <[email protected]> wrote:
> 2011/5/10 Kai Tietz <[email protected]>:
>> 2011/5/10 Richard Guenther <[email protected]>:
>>> On Tue, May 10, 2011 at 5:30 PM, Paolo Bonzini <[email protected]> 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.
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.
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.
/* 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
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
>
