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. 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 >