| Message ID | 1943095191.242428.1306405200685.JavaMail.root@zmail06.collab.prod.int.phx2.redhat.com |
|---|---|
| State | New |
| Headers | show |
On Thu, May 26, 2011 at 12:20 PM, Kai Tietz <ktietz@redhat.com> wrote: > Hello, > > this patch ensures that after gimplification also comparison expressions using FE's boolean_type_node. As we need to deal here with C/C++'s (obj-c/c++ and java's), Ada's, and Fortran's specific boolean types, this patch alters some checks in tree-cfg for Ada's sake, and we need to deal in fold-const about type-conversion of comparisons special. > Additionally it takes care that in forwprop pass we don't do type hoising for boolean types. > > ChangeLog > > 2011-05-26 Kai Tietz > > * gimplify.c (gimple_boolify): Boolify all comparison > expressions. > (gimplify_expr): Use 'useless_type_conversion_p' for comparing > org_type with boolean_type_node for TRUTH-expressions and comparisons. > * fold-const.c (fold_unary_loc): Handle comparison conversions with > boolean-type special. > * tree-cfg.c (verify_gimple_comparison): Adjust check for boolean > or compatible types. > (verify_gimple_assign_unary): Likewise. > * tree-ssa-forwprop.c (forward_propagate_comparison): Handle > boolean case special. > > Tested on x86_64-pc-linux-gnu (multilib) with regression test for all standard languages (C, C++, Obj-C, Fortran, Java) plus Obj-C++ and Ada. Ok for apply? It obviously isn't ok to apply before a patch has gone in that will resolve all of the FAILs you specify. Comments on the patch: @@ -7281,9 +7284,28 @@ gimplify_expr (tree *expr_p, gimple_seq plain wrong if bitfields are involved. */ { tree type = TREE_TYPE (TREE_OPERAND (*expr_p, 1)); + tree org_type = TREE_TYPE (*expr_p); + + if (!useless_type_conversion_p (org_type, boolean_type_node)) + { + TREE_TYPE (*expr_p) = boolean_type_node; + *expr_p = fold_convert_loc (saved_location, org_type, *expr_p); + ret = GS_OK; + goto dont_recalculate; + } The above should be only done for !AGGREGATE_TYPE_P. Probably then the strange dont_recalcuate goto can go away as well. if (!AGGREGATE_TYPE_P (type)) - goto expr_2; + { + enum gimplify_status r0, r1; + + r0 = gimplify_expr (&TREE_OPERAND (*expr_p, 0), pre_p, + post_p, is_gimple_val, fb_rvalue); + r1 = gimplify_expr (&TREE_OPERAND (*expr_p, 1), pre_p, + post_p, is_gimple_val, fb_rvalue); + + ret = MIN (r0, r1); + } + why change this? @@ -7641,6 +7641,12 @@ fold_unary_loc (location_t loc, enum tre } else if (COMPARISON_CLASS_P (arg0)) { + /* Don't optimize type change, if op0 is of kind boolean_type_node. + Otherwise this will lead to race-condition on gimplification + trying to boolify comparison expression. */ + if (TREE_TYPE (op0) == boolean_type_node) + return NULL_TREE; + if (TREE_CODE (type) == BOOLEAN_TYPE) { arg0 = copy_node (arg0); The code leading here looks quite strange to me ... tree fold_unary_loc (location_t loc, enum tree_code code, tree type, tree op0) { ... if (TREE_CODE_CLASS (code) == tcc_unary) { ... else if (COMPARISON_CLASS_P (arg0)) { if (TREE_CODE (type) == BOOLEAN_TYPE) { arg0 = copy_node (arg0); TREE_TYPE (arg0) = type; return arg0; } else if (TREE_CODE (type) != INTEGER_TYPE) return fold_build3_loc (loc, COND_EXPR, type, arg0, fold_build1_loc (loc, code, type, integer_one_node), fold_build1_loc (loc, code, type, integer_zero_node)); } so, for any tcc_unary, like NEGATE_EXPR, with BOOLEAN_TYPE, return arg0 ... sure. Same for the 2nd case. ~ (a == b) isn't the same as a == b ? ~1 : ~0. I _suppose_ those cases were ment for CONVERT_EXPR_CODE_P (code) instead of all of tcc_unary, in which case they should be dropped or moved below where we handle conversions explicitly. That said - does anyone remember anything about that above code? Trying to do some svn blame history tracking now ... Richard. > The following tests are failing due this change (what is to be expected here and needs some additional handling in forwprop). > FAIL: gcc.dg/binop-xor1.c scan-tree-dump-times optimized "<bb[^>]*>" 5 > FAIL: gcc.dg/binop-xor1.c scan-tree-dump-times optimized "\^" 1 > FAIL: gcc.dg/binop-xor3.c scan-tree-dump-times optimized "<bb[^>]*>" 1 > FAIL: gcc.dg/binop-xor3.c scan-tree-dump-times optimized "\^" 1 > XPASS: gcc.dg/tree-ssa/20030807-7.c scan-tree-dump-times vrp1 "if " 1 > FAIL: gcc.dg/tree-ssa/builtin-expect-5.c scan-tree-dump forwprop1 "builtin_expect[^\n]*, 1\);\n[^\n]*if" > FAIL: gcc.dg/tree-ssa/pr21031.c scan-tree-dump-times forwprop1 "Replaced" 2 > FAIL: gcc.dg/tree-ssa/pr30978.c scan-tree-dump optimized "e_. = a_..D. > 0;" > FAIL: gcc.dg/tree-ssa/ssa-fre-6.c scan-tree-dump-times fre1 "Replaced " 5 > FAIL: gcc.dg/tree-ssa/vrp47.c scan-tree-dump-times dom1 "x[^ ]* & y" 1 > FAIL: gcc.dg/tree-ssa/vrp47.c scan-tree-dump-times vrp1 "x[^ ]* \^ 1" 1 > FAIL: gcc.dg/vect/pr49038.c execution test > FAIL: gcc.dg/vect/pr49038.c -flto execution test > FAIL: gcc.target/i386/andor-2.c scan-assembler-not sete > > The Ada, Obj-C, Obj-C++, C++, Fortran, and Java testsuite don't show any new regressions by this. > > Some failing testcases are simply caused by different folding behavior and producing simplier code. The binop-xor tests 1 and 3 might be better removed for now, or marked as being expect to fail. The cause for their failing is in doing tree-analysis via fold-const on gimplified trees, which now don't allow folding here to look through. > To illustrate required changes for other tests, I attach here some required changes for testsuite > > Index: gcc.dg/tree-ssa/pr30978.c > =================================================================== > --- gcc.dg/tree-ssa/pr30978.c (revision 174264) > +++ gcc.dg/tree-ssa/pr30978.c (working copy) > @@ -10,5 +10,5 @@ > return e; > } > > -/* { dg-final { scan-tree-dump "e_. = a_..D. > 0;" "optimized" } } */ > +/* { dg-final { scan-tree-dump " = a_..D. > 0;\n[^\n]*e_. = \\\(int\\\)" "optimized" } } */ > /* { dg-final { cleanup-tree-dump "optimized" } } */ > > Index: gcc.dg/tree-ssa/builtin-expect-5.c > =================================================================== > --- gcc.dg/tree-ssa/builtin-expect-5.c (revision 174264) > +++ gcc.dg/tree-ssa/builtin-expect-5.c (working copy) > @@ -11,5 +11,5 @@ > > /* { dg-final { scan-tree-dump-times { if } 2 "forwprop1"} } */ > /* { dg-final { scan-tree-dump {builtin_expect[^\n]*, 0\);\n[^\n]*if} "forwprop1"} } */ > -/* { dg-final { scan-tree-dump {builtin_expect[^\n]*, 1\);\n[^\n]*if} "forwprop1"} } */ > +/* { dg-final { scan-tree-dump {builtin_expect[^\n]*, 1\);} "forwprop1"} } */ > /* { dg-final { cleanup-tree-dump "forwprop?" } } */ > > Index: gcc.dg/tree-ssa/pr21031.c > =================================================================== > --- gcc.dg/tree-ssa/pr21031.c (revision 174264) > +++ gcc.dg/tree-ssa/pr21031.c (working copy) > @@ -16,5 +16,5 @@ > return 0; > } > > -/* { dg-final { scan-tree-dump-times "Replaced" 2 "forwprop1"} } */ > +/* { dg-final { scan-tree-dump-times "Replaced" 1 "forwprop1"} } */ > /* { dg-final { cleanup-tree-dump "forwprop1" } } */ > > Index: gcc.dg/tree-ssa/ssa-fre-6.c > =================================================================== > --- gcc.dg/tree-ssa/ssa-fre-6.c (revision 174264) > +++ gcc.dg/tree-ssa/ssa-fre-6.c (working copy) > @@ -2,5 +2,5 @@ > /* { dg-options "-O -fdump-tree-fre1-details" } */ > > int i; int foo(void) { i = 2; int j = i * 2; int k = i + 2; return j == k; } > -/* { dg-final { scan-tree-dump-times "Replaced " 5 "fre1" } } */ > +/* { dg-final { scan-tree-dump-times "Replaced " 6 "fre1" } } */ > /* { dg-final { cleanup-tree-dump "fre1" } } */ > > Regards, > Kai >
On Thu, May 26, 2011 at 12:46 PM, Richard Guenther <richard.guenther@gmail.com> wrote: > On Thu, May 26, 2011 at 12:20 PM, Kai Tietz <ktietz@redhat.com> wrote: >> Hello, >> >> this patch ensures that after gimplification also comparison expressions using FE's boolean_type_node. As we need to deal here with C/C++'s (obj-c/c++ and java's), Ada's, and Fortran's specific boolean types, this patch alters some checks in tree-cfg for Ada's sake, and we need to deal in fold-const about type-conversion of comparisons special. >> Additionally it takes care that in forwprop pass we don't do type hoising for boolean types. >> >> ChangeLog >> >> 2011-05-26 Kai Tietz >> >> * gimplify.c (gimple_boolify): Boolify all comparison >> expressions. >> (gimplify_expr): Use 'useless_type_conversion_p' for comparing >> org_type with boolean_type_node for TRUTH-expressions and comparisons. >> * fold-const.c (fold_unary_loc): Handle comparison conversions with >> boolean-type special. >> * tree-cfg.c (verify_gimple_comparison): Adjust check for boolean >> or compatible types. >> (verify_gimple_assign_unary): Likewise. >> * tree-ssa-forwprop.c (forward_propagate_comparison): Handle >> boolean case special. >> >> Tested on x86_64-pc-linux-gnu (multilib) with regression test for all standard languages (C, C++, Obj-C, Fortran, Java) plus Obj-C++ and Ada. Ok for apply? > > It obviously isn't ok to apply before a patch has gone in that will resolve > all of the FAILs you specify. Comments on the patch: > > @@ -7281,9 +7284,28 @@ gimplify_expr (tree *expr_p, gimple_seq > plain wrong if bitfields are involved. */ > { > tree type = TREE_TYPE (TREE_OPERAND (*expr_p, 1)); > + tree org_type = TREE_TYPE (*expr_p); > + > + if (!useless_type_conversion_p (org_type, boolean_type_node)) > + { > + TREE_TYPE (*expr_p) = boolean_type_node; > + *expr_p = fold_convert_loc (saved_location, org_type, *expr_p); > + ret = GS_OK; > + goto dont_recalculate; > + } > > The above should be only done for !AGGREGATE_TYPE_P. Probably then > the strange dont_recalcuate goto can go away as well. > > if (!AGGREGATE_TYPE_P (type)) > - goto expr_2; > + { > + enum gimplify_status r0, r1; > + > + r0 = gimplify_expr (&TREE_OPERAND (*expr_p, 0), pre_p, > + post_p, is_gimple_val, fb_rvalue); > + r1 = gimplify_expr (&TREE_OPERAND (*expr_p, 1), pre_p, > + post_p, is_gimple_val, fb_rvalue); > + > + ret = MIN (r0, r1); > + } > + > > why change this? > > @@ -7641,6 +7641,12 @@ fold_unary_loc (location_t loc, enum tre > } > else if (COMPARISON_CLASS_P (arg0)) > { > + /* Don't optimize type change, if op0 is of kind boolean_type_node. > + Otherwise this will lead to race-condition on gimplification > + trying to boolify comparison expression. */ > + if (TREE_TYPE (op0) == boolean_type_node) > + return NULL_TREE; > + > if (TREE_CODE (type) == BOOLEAN_TYPE) > { > arg0 = copy_node (arg0); > > The code leading here looks quite strange to me ... > > tree > fold_unary_loc (location_t loc, enum tree_code code, tree type, tree op0) > { > ... > if (TREE_CODE_CLASS (code) == tcc_unary) > { > ... > else if (COMPARISON_CLASS_P (arg0)) > { > if (TREE_CODE (type) == BOOLEAN_TYPE) > { > arg0 = copy_node (arg0); > TREE_TYPE (arg0) = type; > return arg0; > } > else if (TREE_CODE (type) != INTEGER_TYPE) > return fold_build3_loc (loc, COND_EXPR, type, arg0, > fold_build1_loc (loc, code, type, > integer_one_node), > fold_build1_loc (loc, code, type, > integer_zero_node)); > } > > so, for any tcc_unary, like NEGATE_EXPR, with BOOLEAN_TYPE, > return arg0 ... sure. Same for the 2nd case. ~ (a == b) isn't > the same as a == b ? ~1 : ~0. I _suppose_ those cases were > ment for CONVERT_EXPR_CODE_P (code) instead of all of tcc_unary, > in which case they should be dropped or moved below where we > handle conversions explicitly. > > That said - does anyone remember anything about that above code? > Trying to do some svn blame history tracking now ... Oh, the patch continues... @@ -3208,7 +3208,10 @@ verify_gimple_comparison (tree type, tre && (!POINTER_TYPE_P (op0_type) || !POINTER_TYPE_P (op1_type) || TYPE_MODE (op0_type) != TYPE_MODE (op1_type))) - || !INTEGRAL_TYPE_P (type)) + || !(TREE_CODE (type) == BOOLEAN_TYPE + || (TREE_TYPE (type) && TREE_CODE (type) == INTEGER_TYPE + && TREE_CODE (TREE_TYPE (type)) == BOOLEAN_TYPE) + || (INTEGRAL_TYPE_P (type) && TYPE_PRECISION (type) == 1))) { why that strange TREE_TYPE (TREE_TYPE ())) thing again? Drop that. @@ -3352,6 +3355,8 @@ verify_gimple_assign_unary (gimple stmt) case TRUTH_NOT_EXPR: /* We require two-valued operand types. */ if (!(TREE_CODE (rhs1_type) == BOOLEAN_TYPE + || (TREE_TYPE (rhs1_type) && TREE_CODE (rhs1_type) == INTEGER_TYPE + && TREE_CODE (TREE_TYPE (rhs1_type)) == BOOLEAN_TYPE) || (INTEGRAL_TYPE_P (rhs1_type) && TYPE_PRECISION (rhs1_type) == 1))) { likewise. Richard.
On Thu, May 26, 2011 at 12:46 PM, Richard Guenther <richard.guenther@gmail.com> wrote: > On Thu, May 26, 2011 at 12:20 PM, Kai Tietz <ktietz@redhat.com> wrote: >> Hello, >> >> this patch ensures that after gimplification also comparison expressions using FE's boolean_type_node. As we need to deal here with C/C++'s (obj-c/c++ and java's), Ada's, and Fortran's specific boolean types, this patch alters some checks in tree-cfg for Ada's sake, and we need to deal in fold-const about type-conversion of comparisons special. >> Additionally it takes care that in forwprop pass we don't do type hoising for boolean types. >> >> ChangeLog >> >> 2011-05-26 Kai Tietz >> >> * gimplify.c (gimple_boolify): Boolify all comparison >> expressions. >> (gimplify_expr): Use 'useless_type_conversion_p' for comparing >> org_type with boolean_type_node for TRUTH-expressions and comparisons. >> * fold-const.c (fold_unary_loc): Handle comparison conversions with >> boolean-type special. >> * tree-cfg.c (verify_gimple_comparison): Adjust check for boolean >> or compatible types. >> (verify_gimple_assign_unary): Likewise. >> * tree-ssa-forwprop.c (forward_propagate_comparison): Handle >> boolean case special. >> >> Tested on x86_64-pc-linux-gnu (multilib) with regression test for all standard languages (C, C++, Obj-C, Fortran, Java) plus Obj-C++ and Ada. Ok for apply? > > It obviously isn't ok to apply before a patch has gone in that will resolve > all of the FAILs you specify. Comments on the patch: > > @@ -7281,9 +7284,28 @@ gimplify_expr (tree *expr_p, gimple_seq > plain wrong if bitfields are involved. */ > { > tree type = TREE_TYPE (TREE_OPERAND (*expr_p, 1)); > + tree org_type = TREE_TYPE (*expr_p); > + > + if (!useless_type_conversion_p (org_type, boolean_type_node)) > + { > + TREE_TYPE (*expr_p) = boolean_type_node; > + *expr_p = fold_convert_loc (saved_location, org_type, *expr_p); > + ret = GS_OK; > + goto dont_recalculate; > + } > > The above should be only done for !AGGREGATE_TYPE_P. Probably then > the strange dont_recalcuate goto can go away as well. > > if (!AGGREGATE_TYPE_P (type)) > - goto expr_2; > + { > + enum gimplify_status r0, r1; > + > + r0 = gimplify_expr (&TREE_OPERAND (*expr_p, 0), pre_p, > + post_p, is_gimple_val, fb_rvalue); > + r1 = gimplify_expr (&TREE_OPERAND (*expr_p, 1), pre_p, > + post_p, is_gimple_val, fb_rvalue); > + > + ret = MIN (r0, r1); > + } > + > > why change this? > > @@ -7641,6 +7641,12 @@ fold_unary_loc (location_t loc, enum tre > } > else if (COMPARISON_CLASS_P (arg0)) > { > + /* Don't optimize type change, if op0 is of kind boolean_type_node. > + Otherwise this will lead to race-condition on gimplification > + trying to boolify comparison expression. */ > + if (TREE_TYPE (op0) == boolean_type_node) > + return NULL_TREE; > + > if (TREE_CODE (type) == BOOLEAN_TYPE) > { > arg0 = copy_node (arg0); > > The code leading here looks quite strange to me ... > > tree > fold_unary_loc (location_t loc, enum tree_code code, tree type, tree op0) > { > ... > if (TREE_CODE_CLASS (code) == tcc_unary) > { > ... > else if (COMPARISON_CLASS_P (arg0)) > { > if (TREE_CODE (type) == BOOLEAN_TYPE) > { > arg0 = copy_node (arg0); > TREE_TYPE (arg0) = type; > return arg0; > } > else if (TREE_CODE (type) != INTEGER_TYPE) > return fold_build3_loc (loc, COND_EXPR, type, arg0, > fold_build1_loc (loc, code, type, > integer_one_node), > fold_build1_loc (loc, code, type, > integer_zero_node)); > } > > so, for any tcc_unary, like NEGATE_EXPR, with BOOLEAN_TYPE, > return arg0 ... sure. Same for the 2nd case. ~ (a == b) isn't > the same as a == b ? ~1 : ~0. I _suppose_ those cases were > ment for CONVERT_EXPR_CODE_P (code) instead of all of tcc_unary, > in which case they should be dropped or moved below where we > handle conversions explicitly. > > That said - does anyone remember anything about that above code? > Trying to do some svn blame history tracking now ... Bah ... 330 rms if (TREE_CODE_CLASS (code) == '1') 330 rms { ... 330 rms else if (TREE_CODE_CLASS (TREE_CODE (arg0)) == '<') 330 rms return fold (build (COND_EXPR, type, arg0, 330 rms fold (build1 (code, type, integer_on e_node)), 330 rms fold (build1 (code, type, integer_ze ro_node)))); and the other case is from 81764 dnovillo { 81764 dnovillo if (TREE_CODE (type) == BOOLEAN_TYPE) 81764 dnovillo { 81764 dnovillo arg0 = copy_node (arg0); 81764 dnovillo TREE_TYPE (arg0) = type; 81764 dnovillo return arg0; 81764 dnovillo } (famous tree-ssa merge revision). So - lets ask who still is around. Diego? ;) Richard.
2011/5/26 Richard Guenther <richard.guenther@gmail.com>: > On Thu, May 26, 2011 at 12:20 PM, Kai Tietz <ktietz@redhat.com> wrote: >> Hello, >> >> this patch ensures that after gimplification also comparison expressions using FE's boolean_type_node. As we need to deal here with C/C++'s (obj-c/c++ and java's), Ada's, and Fortran's specific boolean types, this patch alters some checks in tree-cfg for Ada's sake, and we need to deal in fold-const about type-conversion of comparisons special. >> Additionally it takes care that in forwprop pass we don't do type hoising for boolean types. >> >> ChangeLog >> >> 2011-05-26 Kai Tietz >> >> * gimplify.c (gimple_boolify): Boolify all comparison >> expressions. >> (gimplify_expr): Use 'useless_type_conversion_p' for comparing >> org_type with boolean_type_node for TRUTH-expressions and comparisons. >> * fold-const.c (fold_unary_loc): Handle comparison conversions with >> boolean-type special. >> * tree-cfg.c (verify_gimple_comparison): Adjust check for boolean >> or compatible types. >> (verify_gimple_assign_unary): Likewise. >> * tree-ssa-forwprop.c (forward_propagate_comparison): Handle >> boolean case special. >> >> Tested on x86_64-pc-linux-gnu (multilib) with regression test for all standard languages (C, C++, Obj-C, Fortran, Java) plus Obj-C++ and Ada. Ok for apply? > > It obviously isn't ok to apply before a patch has gone in that will resolve > all of the FAILs you specify. Comments on the patch: > > @@ -7281,9 +7284,28 @@ gimplify_expr (tree *expr_p, gimple_seq > plain wrong if bitfields are involved. */ > { > tree type = TREE_TYPE (TREE_OPERAND (*expr_p, 1)); > + tree org_type = TREE_TYPE (*expr_p); > + > + if (!useless_type_conversion_p (org_type, boolean_type_node)) > + { > + TREE_TYPE (*expr_p) = boolean_type_node; > + *expr_p = fold_convert_loc (saved_location, org_type, *expr_p); > + ret = GS_OK; > + goto dont_recalculate; > + } > > The above should be only done for !AGGREGATE_TYPE_P. Probably then > the strange dont_recalcuate goto can go away as well. I thought so too, but boolification is required for all kind of comparisons. This goto is a bit hacky, but the best way to prevent here an useless recalculation. Without doing this, we get bootstrap/regression test failures, as in tree-cfg we check that comparison type is of boolean (or compatible kind). > if (!AGGREGATE_TYPE_P (type)) > - goto expr_2; > + { > + enum gimplify_status r0, r1; > + > + r0 = gimplify_expr (&TREE_OPERAND (*expr_p, 0), pre_p, > + post_p, is_gimple_val, fb_rvalue); > + r1 = gimplify_expr (&TREE_OPERAND (*expr_p, 1), pre_p, > + post_p, is_gimple_val, fb_rvalue); > + > + ret = MIN (r0, r1); > + } > + > > why change this? This part can be reverted to the goto expr_2 again. > @@ -7641,6 +7641,12 @@ fold_unary_loc (location_t loc, enum tre > } > else if (COMPARISON_CLASS_P (arg0)) > { > + /* Don't optimize type change, if op0 is of kind boolean_type_node. > + Otherwise this will lead to race-condition on gimplification > + trying to boolify comparison expression. */ > + if (TREE_TYPE (op0) == boolean_type_node) > + return NULL_TREE; > + > if (TREE_CODE (type) == BOOLEAN_TYPE) > { > arg0 = copy_node (arg0); > > The code leading here looks quite strange to me ... Issue is that Fortran (and this is the cause for this boolean_type_node check) has more then one boolean type. Gimplifier normalizes to boolean_type_node. If now a different boolean-type (with different mode) of Fortran is used, it leads to an endless-loop in type conversion. An example boolean-kind(mode-size=1) and boolean-kind(mode-size=2) are no useless type conversion. So by gimplifier type gets set to boolean-kind(mode-size=1) (if this is default boolean_type_node's definition), and then casted to boolean-kind(mode-size=2) expression. > tree > fold_unary_loc (location_t loc, enum tree_code code, tree type, tree op0) > { > ... > if (TREE_CODE_CLASS (code) == tcc_unary) > { > ... > else if (COMPARISON_CLASS_P (arg0)) > { > if (TREE_CODE (type) == BOOLEAN_TYPE) > { > arg0 = copy_node (arg0); > TREE_TYPE (arg0) = type; > return arg0; > } > else if (TREE_CODE (type) != INTEGER_TYPE) > return fold_build3_loc (loc, COND_EXPR, type, arg0, > fold_build1_loc (loc, code, type, > integer_one_node), > fold_build1_loc (loc, code, type, > integer_zero_node)); > } > > so, for any tcc_unary, like NEGATE_EXPR, with BOOLEAN_TYPE, > return arg0 ... sure. Same for the 2nd case. ~ (a == b) isn't > the same as a == b ? ~1 : ~0. I _suppose_ those cases were > ment for CONVERT_EXPR_CODE_P (code) instead of all of tcc_unary, > in which case they should be dropped or moved below where we > handle conversions explicitly. > > That said - does anyone remember anything about that above code? > Trying to do some svn blame history tracking now ... > > Richard. > >> The following tests are failing due this change (what is to be expected here and needs some additional handling in forwprop). >> FAIL: gcc.dg/binop-xor1.c scan-tree-dump-times optimized "<bb[^>]*>" 5 >> FAIL: gcc.dg/binop-xor1.c scan-tree-dump-times optimized "\^" 1 >> FAIL: gcc.dg/binop-xor3.c scan-tree-dump-times optimized "<bb[^>]*>" 1 >> FAIL: gcc.dg/binop-xor3.c scan-tree-dump-times optimized "\^" 1 >> XPASS: gcc.dg/tree-ssa/20030807-7.c scan-tree-dump-times vrp1 "if " 1 >> FAIL: gcc.dg/tree-ssa/builtin-expect-5.c scan-tree-dump forwprop1 "builtin_expect[^\n]*, 1\);\n[^\n]*if" >> FAIL: gcc.dg/tree-ssa/pr21031.c scan-tree-dump-times forwprop1 "Replaced" 2 >> FAIL: gcc.dg/tree-ssa/pr30978.c scan-tree-dump optimized "e_. = a_..D. > 0;" >> FAIL: gcc.dg/tree-ssa/ssa-fre-6.c scan-tree-dump-times fre1 "Replaced " 5 >> FAIL: gcc.dg/tree-ssa/vrp47.c scan-tree-dump-times dom1 "x[^ ]* & y" 1 >> FAIL: gcc.dg/tree-ssa/vrp47.c scan-tree-dump-times vrp1 "x[^ ]* \^ 1" 1 >> FAIL: gcc.dg/vect/pr49038.c execution test >> FAIL: gcc.dg/vect/pr49038.c -flto execution test >> FAIL: gcc.target/i386/andor-2.c scan-assembler-not sete >> >> The Ada, Obj-C, Obj-C++, C++, Fortran, and Java testsuite don't show any new regressions by this. >> >> Some failing testcases are simply caused by different folding behavior and producing simplier code. The binop-xor tests 1 and 3 might be better removed for now, or marked as being expect to fail. The cause for their failing is in doing tree-analysis via fold-const on gimplified trees, which now don't allow folding here to look through. >> To illustrate required changes for other tests, I attach here some required changes for testsuite >> >> Index: gcc.dg/tree-ssa/pr30978.c >> =================================================================== >> --- gcc.dg/tree-ssa/pr30978.c (revision 174264) >> +++ gcc.dg/tree-ssa/pr30978.c (working copy) >> @@ -10,5 +10,5 @@ >> return e; >> } >> >> -/* { dg-final { scan-tree-dump "e_. = a_..D. > 0;" "optimized" } } */ >> +/* { dg-final { scan-tree-dump " = a_..D. > 0;\n[^\n]*e_. = \\\(int\\\)" "optimized" } } */ >> /* { dg-final { cleanup-tree-dump "optimized" } } */ >> >> Index: gcc.dg/tree-ssa/builtin-expect-5.c >> =================================================================== >> --- gcc.dg/tree-ssa/builtin-expect-5.c (revision 174264) >> +++ gcc.dg/tree-ssa/builtin-expect-5.c (working copy) >> @@ -11,5 +11,5 @@ >> >> /* { dg-final { scan-tree-dump-times { if } 2 "forwprop1"} } */ >> /* { dg-final { scan-tree-dump {builtin_expect[^\n]*, 0\);\n[^\n]*if} "forwprop1"} } */ >> -/* { dg-final { scan-tree-dump {builtin_expect[^\n]*, 1\);\n[^\n]*if} "forwprop1"} } */ >> +/* { dg-final { scan-tree-dump {builtin_expect[^\n]*, 1\);} "forwprop1"} } */ >> /* { dg-final { cleanup-tree-dump "forwprop?" } } */ >> >> Index: gcc.dg/tree-ssa/pr21031.c >> =================================================================== >> --- gcc.dg/tree-ssa/pr21031.c (revision 174264) >> +++ gcc.dg/tree-ssa/pr21031.c (working copy) >> @@ -16,5 +16,5 @@ >> return 0; >> } >> >> -/* { dg-final { scan-tree-dump-times "Replaced" 2 "forwprop1"} } */ >> +/* { dg-final { scan-tree-dump-times "Replaced" 1 "forwprop1"} } */ >> /* { dg-final { cleanup-tree-dump "forwprop1" } } */ >> >> Index: gcc.dg/tree-ssa/ssa-fre-6.c >> =================================================================== >> --- gcc.dg/tree-ssa/ssa-fre-6.c (revision 174264) >> +++ gcc.dg/tree-ssa/ssa-fre-6.c (working copy) >> @@ -2,5 +2,5 @@ >> /* { dg-options "-O -fdump-tree-fre1-details" } */ >> >> int i; int foo(void) { i = 2; int j = i * 2; int k = i + 2; return j == k; } >> -/* { dg-final { scan-tree-dump-times "Replaced " 5 "fre1" } } */ >> +/* { dg-final { scan-tree-dump-times "Replaced " 6 "fre1" } } */ >> /* { dg-final { cleanup-tree-dump "fre1" } } */ >> >> Regards, >> Kai >> Regards, Kai
2011/5/26 Richard Guenther <richard.guenther@gmail.com>: > On Thu, May 26, 2011 at 12:46 PM, Richard Guenther > <richard.guenther@gmail.com> wrote: >> On Thu, May 26, 2011 at 12:20 PM, Kai Tietz <ktietz@redhat.com> wrote: >>> Hello, >>> >>> this patch ensures that after gimplification also comparison expressions using FE's boolean_type_node. As we need to deal here with C/C++'s (obj-c/c++ and java's), Ada's, and Fortran's specific boolean types, this patch alters some checks in tree-cfg for Ada's sake, and we need to deal in fold-const about type-conversion of comparisons special. >>> Additionally it takes care that in forwprop pass we don't do type hoising for boolean types. >>> >>> ChangeLog >>> >>> 2011-05-26 Kai Tietz >>> >>> * gimplify.c (gimple_boolify): Boolify all comparison >>> expressions. >>> (gimplify_expr): Use 'useless_type_conversion_p' for comparing >>> org_type with boolean_type_node for TRUTH-expressions and comparisons. >>> * fold-const.c (fold_unary_loc): Handle comparison conversions with >>> boolean-type special. >>> * tree-cfg.c (verify_gimple_comparison): Adjust check for boolean >>> or compatible types. >>> (verify_gimple_assign_unary): Likewise. >>> * tree-ssa-forwprop.c (forward_propagate_comparison): Handle >>> boolean case special. >>> >>> Tested on x86_64-pc-linux-gnu (multilib) with regression test for all standard languages (C, C++, Obj-C, Fortran, Java) plus Obj-C++ and Ada. Ok for apply? >> >> It obviously isn't ok to apply before a patch has gone in that will resolve >> all of the FAILs you specify. Comments on the patch: >> >> @@ -7281,9 +7284,28 @@ gimplify_expr (tree *expr_p, gimple_seq >> plain wrong if bitfields are involved. */ >> { >> tree type = TREE_TYPE (TREE_OPERAND (*expr_p, 1)); >> + tree org_type = TREE_TYPE (*expr_p); >> + >> + if (!useless_type_conversion_p (org_type, boolean_type_node)) >> + { >> + TREE_TYPE (*expr_p) = boolean_type_node; >> + *expr_p = fold_convert_loc (saved_location, org_type, *expr_p); >> + ret = GS_OK; >> + goto dont_recalculate; >> + } >> >> The above should be only done for !AGGREGATE_TYPE_P. Probably then >> the strange dont_recalcuate goto can go away as well. >> >> if (!AGGREGATE_TYPE_P (type)) >> - goto expr_2; >> + { >> + enum gimplify_status r0, r1; >> + >> + r0 = gimplify_expr (&TREE_OPERAND (*expr_p, 0), pre_p, >> + post_p, is_gimple_val, fb_rvalue); >> + r1 = gimplify_expr (&TREE_OPERAND (*expr_p, 1), pre_p, >> + post_p, is_gimple_val, fb_rvalue); >> + >> + ret = MIN (r0, r1); >> + } >> + >> >> why change this? >> >> @@ -7641,6 +7641,12 @@ fold_unary_loc (location_t loc, enum tre >> } >> else if (COMPARISON_CLASS_P (arg0)) >> { >> + /* Don't optimize type change, if op0 is of kind boolean_type_node. >> + Otherwise this will lead to race-condition on gimplification >> + trying to boolify comparison expression. */ >> + if (TREE_TYPE (op0) == boolean_type_node) >> + return NULL_TREE; >> + >> if (TREE_CODE (type) == BOOLEAN_TYPE) >> { >> arg0 = copy_node (arg0); >> >> The code leading here looks quite strange to me ... >> >> tree >> fold_unary_loc (location_t loc, enum tree_code code, tree type, tree op0) >> { >> ... >> if (TREE_CODE_CLASS (code) == tcc_unary) >> { >> ... >> else if (COMPARISON_CLASS_P (arg0)) >> { >> if (TREE_CODE (type) == BOOLEAN_TYPE) >> { >> arg0 = copy_node (arg0); >> TREE_TYPE (arg0) = type; >> return arg0; >> } >> else if (TREE_CODE (type) != INTEGER_TYPE) >> return fold_build3_loc (loc, COND_EXPR, type, arg0, >> fold_build1_loc (loc, code, type, >> integer_one_node), >> fold_build1_loc (loc, code, type, >> integer_zero_node)); >> } >> >> so, for any tcc_unary, like NEGATE_EXPR, with BOOLEAN_TYPE, >> return arg0 ... sure. Same for the 2nd case. ~ (a == b) isn't >> the same as a == b ? ~1 : ~0. I _suppose_ those cases were >> ment for CONVERT_EXPR_CODE_P (code) instead of all of tcc_unary, >> in which case they should be dropped or moved below where we >> handle conversions explicitly. >> >> That said - does anyone remember anything about that above code? >> Trying to do some svn blame history tracking now ... > > Oh, the patch continues... > > @@ -3208,7 +3208,10 @@ verify_gimple_comparison (tree type, tre > && (!POINTER_TYPE_P (op0_type) > || !POINTER_TYPE_P (op1_type) > || TYPE_MODE (op0_type) != TYPE_MODE (op1_type))) > - || !INTEGRAL_TYPE_P (type)) > + || !(TREE_CODE (type) == BOOLEAN_TYPE > + || (TREE_TYPE (type) && TREE_CODE (type) == INTEGER_TYPE > + && TREE_CODE (TREE_TYPE (type)) == BOOLEAN_TYPE) > + || (INTEGRAL_TYPE_P (type) && TYPE_PRECISION (type) == 1))) > { > > why that strange TREE_TYPE (TREE_TYPE ())) thing again? Drop > that. > > @@ -3352,6 +3355,8 @@ verify_gimple_assign_unary (gimple stmt) > case TRUTH_NOT_EXPR: > /* We require two-valued operand types. */ > if (!(TREE_CODE (rhs1_type) == BOOLEAN_TYPE > + || (TREE_TYPE (rhs1_type) && TREE_CODE (rhs1_type) == INTEGER_TYPE > + && TREE_CODE (TREE_TYPE (rhs1_type)) == BOOLEAN_TYPE) > || (INTEGRAL_TYPE_P (rhs1_type) > && TYPE_PRECISION (rhs1_type) == 1))) > { > > likewise. Well, those checks are necessary for Ada and its crippled boolean_type_node and computed boolean-based integer construct. Ada derives here the boolean-type to an integer with range 0..1 and the only way to find out that it is in fact such a beast is by looking into TREE_TYPE of type. See here Ada's code for getting base-type information. As such things are treated as compatible they can appear for TRUTH_NOT expressions and comparisons. Regards, Kai
On Thu, May 26, 2011 at 1:05 PM, Kai Tietz <ktietz70@googlemail.com> wrote: > 2011/5/26 Richard Guenther <richard.guenther@gmail.com>: >> On Thu, May 26, 2011 at 12:20 PM, Kai Tietz <ktietz@redhat.com> wrote: >>> Hello, >>> >>> this patch ensures that after gimplification also comparison expressions using FE's boolean_type_node. As we need to deal here with C/C++'s (obj-c/c++ and java's), Ada's, and Fortran's specific boolean types, this patch alters some checks in tree-cfg for Ada's sake, and we need to deal in fold-const about type-conversion of comparisons special. >>> Additionally it takes care that in forwprop pass we don't do type hoising for boolean types. >>> >>> ChangeLog >>> >>> 2011-05-26 Kai Tietz >>> >>> * gimplify.c (gimple_boolify): Boolify all comparison >>> expressions. >>> (gimplify_expr): Use 'useless_type_conversion_p' for comparing >>> org_type with boolean_type_node for TRUTH-expressions and comparisons. >>> * fold-const.c (fold_unary_loc): Handle comparison conversions with >>> boolean-type special. >>> * tree-cfg.c (verify_gimple_comparison): Adjust check for boolean >>> or compatible types. >>> (verify_gimple_assign_unary): Likewise. >>> * tree-ssa-forwprop.c (forward_propagate_comparison): Handle >>> boolean case special. >>> >>> Tested on x86_64-pc-linux-gnu (multilib) with regression test for all standard languages (C, C++, Obj-C, Fortran, Java) plus Obj-C++ and Ada. Ok for apply? >> >> It obviously isn't ok to apply before a patch has gone in that will resolve >> all of the FAILs you specify. Comments on the patch: >> >> @@ -7281,9 +7284,28 @@ gimplify_expr (tree *expr_p, gimple_seq >> plain wrong if bitfields are involved. */ >> { >> tree type = TREE_TYPE (TREE_OPERAND (*expr_p, 1)); >> + tree org_type = TREE_TYPE (*expr_p); >> + >> + if (!useless_type_conversion_p (org_type, boolean_type_node)) >> + { >> + TREE_TYPE (*expr_p) = boolean_type_node; >> + *expr_p = fold_convert_loc (saved_location, org_type, *expr_p); >> + ret = GS_OK; >> + goto dont_recalculate; >> + } >> >> The above should be only done for !AGGREGATE_TYPE_P. Probably then >> the strange dont_recalcuate goto can go away as well. > > I thought so too, but boolification is required for all kind of > comparisons. This goto is a bit hacky, but the best way to prevent > here an useless recalculation. Without doing this, we get > bootstrap/regression test failures, as in tree-cfg we check that > comparison type is of boolean (or compatible kind). That doesn't make sense if you look at the other two cases which both will return GS_OK and thus re-enter this. The aggregate variable size path also will use memcmp which returns int,so first boolifying doesn't make sense. >> if (!AGGREGATE_TYPE_P (type)) >> - goto expr_2; >> + { >> + enum gimplify_status r0, r1; >> + >> + r0 = gimplify_expr (&TREE_OPERAND (*expr_p, 0), pre_p, >> + post_p, is_gimple_val, fb_rvalue); >> + r1 = gimplify_expr (&TREE_OPERAND (*expr_p, 1), pre_p, >> + post_p, is_gimple_val, fb_rvalue); >> + >> + ret = MIN (r0, r1); >> + } >> + >> >> why change this? > > This part can be reverted to the goto expr_2 again. > >> @@ -7641,6 +7641,12 @@ fold_unary_loc (location_t loc, enum tre >> } >> else if (COMPARISON_CLASS_P (arg0)) >> { >> + /* Don't optimize type change, if op0 is of kind boolean_type_node. >> + Otherwise this will lead to race-condition on gimplification >> + trying to boolify comparison expression. */ >> + if (TREE_TYPE (op0) == boolean_type_node) >> + return NULL_TREE; >> + >> if (TREE_CODE (type) == BOOLEAN_TYPE) >> { >> arg0 = copy_node (arg0); >> >> The code leading here looks quite strange to me ... > > Issue is that Fortran (and this is the cause for this > boolean_type_node check) has more then one boolean type. Gimplifier > normalizes to boolean_type_node. If now a different boolean-type > (with different mode) of Fortran is used, it leads to an endless-loop > in type conversion. An example boolean-kind(mode-size=1) and > boolean-kind(mode-size=2) are no useless type conversion. So by > gimplifier type gets set to boolean-kind(mode-size=1) (if this is > default boolean_type_node's definition), and then casted to > boolean-kind(mode-size=2) expression. I said the whole code looks strange, it should be reworked instead of addign this kind of other non-obviousness. Richard. > >> tree >> fold_unary_loc (location_t loc, enum tree_code code, tree type, tree op0) >> { >> ... >> if (TREE_CODE_CLASS (code) == tcc_unary) >> { >> ... >> else if (COMPARISON_CLASS_P (arg0)) >> { >> if (TREE_CODE (type) == BOOLEAN_TYPE) >> { >> arg0 = copy_node (arg0); >> TREE_TYPE (arg0) = type; >> return arg0; >> } >> else if (TREE_CODE (type) != INTEGER_TYPE) >> return fold_build3_loc (loc, COND_EXPR, type, arg0, >> fold_build1_loc (loc, code, type, >> integer_one_node), >> fold_build1_loc (loc, code, type, >> integer_zero_node)); >> } >> >> so, for any tcc_unary, like NEGATE_EXPR, with BOOLEAN_TYPE, >> return arg0 ... sure. Same for the 2nd case. ~ (a == b) isn't >> the same as a == b ? ~1 : ~0. I _suppose_ those cases were >> ment for CONVERT_EXPR_CODE_P (code) instead of all of tcc_unary, >> in which case they should be dropped or moved below where we >> handle conversions explicitly. >> >> That said - does anyone remember anything about that above code? >> Trying to do some svn blame history tracking now ... >> >> Richard. >> >>> The following tests are failing due this change (what is to be expected here and needs some additional handling in forwprop). >>> FAIL: gcc.dg/binop-xor1.c scan-tree-dump-times optimized "<bb[^>]*>" 5 >>> FAIL: gcc.dg/binop-xor1.c scan-tree-dump-times optimized "\^" 1 >>> FAIL: gcc.dg/binop-xor3.c scan-tree-dump-times optimized "<bb[^>]*>" 1 >>> FAIL: gcc.dg/binop-xor3.c scan-tree-dump-times optimized "\^" 1 >>> XPASS: gcc.dg/tree-ssa/20030807-7.c scan-tree-dump-times vrp1 "if " 1 >>> FAIL: gcc.dg/tree-ssa/builtin-expect-5.c scan-tree-dump forwprop1 "builtin_expect[^\n]*, 1\);\n[^\n]*if" >>> FAIL: gcc.dg/tree-ssa/pr21031.c scan-tree-dump-times forwprop1 "Replaced" 2 >>> FAIL: gcc.dg/tree-ssa/pr30978.c scan-tree-dump optimized "e_. = a_..D. > 0;" >>> FAIL: gcc.dg/tree-ssa/ssa-fre-6.c scan-tree-dump-times fre1 "Replaced " 5 >>> FAIL: gcc.dg/tree-ssa/vrp47.c scan-tree-dump-times dom1 "x[^ ]* & y" 1 >>> FAIL: gcc.dg/tree-ssa/vrp47.c scan-tree-dump-times vrp1 "x[^ ]* \^ 1" 1 >>> FAIL: gcc.dg/vect/pr49038.c execution test >>> FAIL: gcc.dg/vect/pr49038.c -flto execution test >>> FAIL: gcc.target/i386/andor-2.c scan-assembler-not sete >>> >>> The Ada, Obj-C, Obj-C++, C++, Fortran, and Java testsuite don't show any new regressions by this. >>> >>> Some failing testcases are simply caused by different folding behavior and producing simplier code. The binop-xor tests 1 and 3 might be better removed for now, or marked as being expect to fail. The cause for their failing is in doing tree-analysis via fold-const on gimplified trees, which now don't allow folding here to look through. >>> To illustrate required changes for other tests, I attach here some required changes for testsuite >>> >>> Index: gcc.dg/tree-ssa/pr30978.c >>> =================================================================== >>> --- gcc.dg/tree-ssa/pr30978.c (revision 174264) >>> +++ gcc.dg/tree-ssa/pr30978.c (working copy) >>> @@ -10,5 +10,5 @@ >>> return e; >>> } >>> >>> -/* { dg-final { scan-tree-dump "e_. = a_..D. > 0;" "optimized" } } */ >>> +/* { dg-final { scan-tree-dump " = a_..D. > 0;\n[^\n]*e_. = \\\(int\\\)" "optimized" } } */ >>> /* { dg-final { cleanup-tree-dump "optimized" } } */ >>> >>> Index: gcc.dg/tree-ssa/builtin-expect-5.c >>> =================================================================== >>> --- gcc.dg/tree-ssa/builtin-expect-5.c (revision 174264) >>> +++ gcc.dg/tree-ssa/builtin-expect-5.c (working copy) >>> @@ -11,5 +11,5 @@ >>> >>> /* { dg-final { scan-tree-dump-times { if } 2 "forwprop1"} } */ >>> /* { dg-final { scan-tree-dump {builtin_expect[^\n]*, 0\);\n[^\n]*if} "forwprop1"} } */ >>> -/* { dg-final { scan-tree-dump {builtin_expect[^\n]*, 1\);\n[^\n]*if} "forwprop1"} } */ >>> +/* { dg-final { scan-tree-dump {builtin_expect[^\n]*, 1\);} "forwprop1"} } */ >>> /* { dg-final { cleanup-tree-dump "forwprop?" } } */ >>> >>> Index: gcc.dg/tree-ssa/pr21031.c >>> =================================================================== >>> --- gcc.dg/tree-ssa/pr21031.c (revision 174264) >>> +++ gcc.dg/tree-ssa/pr21031.c (working copy) >>> @@ -16,5 +16,5 @@ >>> return 0; >>> } >>> >>> -/* { dg-final { scan-tree-dump-times "Replaced" 2 "forwprop1"} } */ >>> +/* { dg-final { scan-tree-dump-times "Replaced" 1 "forwprop1"} } */ >>> /* { dg-final { cleanup-tree-dump "forwprop1" } } */ >>> >>> Index: gcc.dg/tree-ssa/ssa-fre-6.c >>> =================================================================== >>> --- gcc.dg/tree-ssa/ssa-fre-6.c (revision 174264) >>> +++ gcc.dg/tree-ssa/ssa-fre-6.c (working copy) >>> @@ -2,5 +2,5 @@ >>> /* { dg-options "-O -fdump-tree-fre1-details" } */ >>> >>> int i; int foo(void) { i = 2; int j = i * 2; int k = i + 2; return j == k; } >>> -/* { dg-final { scan-tree-dump-times "Replaced " 5 "fre1" } } */ >>> +/* { dg-final { scan-tree-dump-times "Replaced " 6 "fre1" } } */ >>> /* { dg-final { cleanup-tree-dump "fre1" } } */ >>> >>> Regards, >>> Kai >>> > > Regards, > Kai >
On Thu, May 26, 2011 at 1:11 PM, Kai Tietz <ktietz70@googlemail.com> wrote: > 2011/5/26 Richard Guenther <richard.guenther@gmail.com>: >> On Thu, May 26, 2011 at 12:46 PM, Richard Guenther >> <richard.guenther@gmail.com> wrote: >>> On Thu, May 26, 2011 at 12:20 PM, Kai Tietz <ktietz@redhat.com> wrote: >>>> Hello, >>>> >>>> this patch ensures that after gimplification also comparison expressions using FE's boolean_type_node. As we need to deal here with C/C++'s (obj-c/c++ and java's), Ada's, and Fortran's specific boolean types, this patch alters some checks in tree-cfg for Ada's sake, and we need to deal in fold-const about type-conversion of comparisons special. >>>> Additionally it takes care that in forwprop pass we don't do type hoising for boolean types. >>>> >>>> ChangeLog >>>> >>>> 2011-05-26 Kai Tietz >>>> >>>> * gimplify.c (gimple_boolify): Boolify all comparison >>>> expressions. >>>> (gimplify_expr): Use 'useless_type_conversion_p' for comparing >>>> org_type with boolean_type_node for TRUTH-expressions and comparisons. >>>> * fold-const.c (fold_unary_loc): Handle comparison conversions with >>>> boolean-type special. >>>> * tree-cfg.c (verify_gimple_comparison): Adjust check for boolean >>>> or compatible types. >>>> (verify_gimple_assign_unary): Likewise. >>>> * tree-ssa-forwprop.c (forward_propagate_comparison): Handle >>>> boolean case special. >>>> >>>> Tested on x86_64-pc-linux-gnu (multilib) with regression test for all standard languages (C, C++, Obj-C, Fortran, Java) plus Obj-C++ and Ada. Ok for apply? >>> >>> It obviously isn't ok to apply before a patch has gone in that will resolve >>> all of the FAILs you specify. Comments on the patch: >>> >>> @@ -7281,9 +7284,28 @@ gimplify_expr (tree *expr_p, gimple_seq >>> plain wrong if bitfields are involved. */ >>> { >>> tree type = TREE_TYPE (TREE_OPERAND (*expr_p, 1)); >>> + tree org_type = TREE_TYPE (*expr_p); >>> + >>> + if (!useless_type_conversion_p (org_type, boolean_type_node)) >>> + { >>> + TREE_TYPE (*expr_p) = boolean_type_node; >>> + *expr_p = fold_convert_loc (saved_location, org_type, *expr_p); >>> + ret = GS_OK; >>> + goto dont_recalculate; >>> + } >>> >>> The above should be only done for !AGGREGATE_TYPE_P. Probably then >>> the strange dont_recalcuate goto can go away as well. >>> >>> if (!AGGREGATE_TYPE_P (type)) >>> - goto expr_2; >>> + { >>> + enum gimplify_status r0, r1; >>> + >>> + r0 = gimplify_expr (&TREE_OPERAND (*expr_p, 0), pre_p, >>> + post_p, is_gimple_val, fb_rvalue); >>> + r1 = gimplify_expr (&TREE_OPERAND (*expr_p, 1), pre_p, >>> + post_p, is_gimple_val, fb_rvalue); >>> + >>> + ret = MIN (r0, r1); >>> + } >>> + >>> >>> why change this? >>> >>> @@ -7641,6 +7641,12 @@ fold_unary_loc (location_t loc, enum tre >>> } >>> else if (COMPARISON_CLASS_P (arg0)) >>> { >>> + /* Don't optimize type change, if op0 is of kind boolean_type_node. >>> + Otherwise this will lead to race-condition on gimplification >>> + trying to boolify comparison expression. */ >>> + if (TREE_TYPE (op0) == boolean_type_node) >>> + return NULL_TREE; >>> + >>> if (TREE_CODE (type) == BOOLEAN_TYPE) >>> { >>> arg0 = copy_node (arg0); >>> >>> The code leading here looks quite strange to me ... >>> >>> tree >>> fold_unary_loc (location_t loc, enum tree_code code, tree type, tree op0) >>> { >>> ... >>> if (TREE_CODE_CLASS (code) == tcc_unary) >>> { >>> ... >>> else if (COMPARISON_CLASS_P (arg0)) >>> { >>> if (TREE_CODE (type) == BOOLEAN_TYPE) >>> { >>> arg0 = copy_node (arg0); >>> TREE_TYPE (arg0) = type; >>> return arg0; >>> } >>> else if (TREE_CODE (type) != INTEGER_TYPE) >>> return fold_build3_loc (loc, COND_EXPR, type, arg0, >>> fold_build1_loc (loc, code, type, >>> integer_one_node), >>> fold_build1_loc (loc, code, type, >>> integer_zero_node)); >>> } >>> >>> so, for any tcc_unary, like NEGATE_EXPR, with BOOLEAN_TYPE, >>> return arg0 ... sure. Same for the 2nd case. ~ (a == b) isn't >>> the same as a == b ? ~1 : ~0. I _suppose_ those cases were >>> ment for CONVERT_EXPR_CODE_P (code) instead of all of tcc_unary, >>> in which case they should be dropped or moved below where we >>> handle conversions explicitly. >>> >>> That said - does anyone remember anything about that above code? >>> Trying to do some svn blame history tracking now ... >> >> Oh, the patch continues... >> >> @@ -3208,7 +3208,10 @@ verify_gimple_comparison (tree type, tre >> && (!POINTER_TYPE_P (op0_type) >> || !POINTER_TYPE_P (op1_type) >> || TYPE_MODE (op0_type) != TYPE_MODE (op1_type))) >> - || !INTEGRAL_TYPE_P (type)) >> + || !(TREE_CODE (type) == BOOLEAN_TYPE >> + || (TREE_TYPE (type) && TREE_CODE (type) == INTEGER_TYPE >> + && TREE_CODE (TREE_TYPE (type)) == BOOLEAN_TYPE) >> + || (INTEGRAL_TYPE_P (type) && TYPE_PRECISION (type) == 1))) >> { >> >> why that strange TREE_TYPE (TREE_TYPE ())) thing again? Drop >> that. >> >> @@ -3352,6 +3355,8 @@ verify_gimple_assign_unary (gimple stmt) >> case TRUTH_NOT_EXPR: >> /* We require two-valued operand types. */ >> if (!(TREE_CODE (rhs1_type) == BOOLEAN_TYPE >> + || (TREE_TYPE (rhs1_type) && TREE_CODE (rhs1_type) == INTEGER_TYPE >> + && TREE_CODE (TREE_TYPE (rhs1_type)) == BOOLEAN_TYPE) >> || (INTEGRAL_TYPE_P (rhs1_type) >> && TYPE_PRECISION (rhs1_type) == 1))) >> { >> >> likewise. > > Well, those checks are necessary for Ada and its crippled > boolean_type_node and computed boolean-based integer construct. Ada > derives here the boolean-type to an integer with range 0..1 and the > only way to find out that it is in fact such a beast is by looking > into TREE_TYPE of type. See here Ada's code for getting base-type > information. > As such things are treated as compatible they can appear for TRUTH_NOT > expressions and comparisons. No they can't as we boolify them. Richard. > Regards, > Kai >
2011/5/26 Richard Guenther <richard.guenther@gmail.com>: > On Thu, May 26, 2011 at 1:11 PM, Kai Tietz <ktietz70@googlemail.com> wrote: >> 2011/5/26 Richard Guenther <richard.guenther@gmail.com>: >>> On Thu, May 26, 2011 at 12:46 PM, Richard Guenther >>> <richard.guenther@gmail.com> wrote: >>>> On Thu, May 26, 2011 at 12:20 PM, Kai Tietz <ktietz@redhat.com> wrote: >>>>> Hello, >>>>> >>>>> this patch ensures that after gimplification also comparison expressions using FE's boolean_type_node. As we need to deal here with C/C++'s (obj-c/c++ and java's), Ada's, and Fortran's specific boolean types, this patch alters some checks in tree-cfg for Ada's sake, and we need to deal in fold-const about type-conversion of comparisons special. >>>>> Additionally it takes care that in forwprop pass we don't do type hoising for boolean types. >>>>> >>>>> ChangeLog >>>>> >>>>> 2011-05-26 Kai Tietz >>>>> >>>>> * gimplify.c (gimple_boolify): Boolify all comparison >>>>> expressions. >>>>> (gimplify_expr): Use 'useless_type_conversion_p' for comparing >>>>> org_type with boolean_type_node for TRUTH-expressions and comparisons. >>>>> * fold-const.c (fold_unary_loc): Handle comparison conversions with >>>>> boolean-type special. >>>>> * tree-cfg.c (verify_gimple_comparison): Adjust check for boolean >>>>> or compatible types. >>>>> (verify_gimple_assign_unary): Likewise. >>>>> * tree-ssa-forwprop.c (forward_propagate_comparison): Handle >>>>> boolean case special. >>>>> >>>>> Tested on x86_64-pc-linux-gnu (multilib) with regression test for all standard languages (C, C++, Obj-C, Fortran, Java) plus Obj-C++ and Ada. Ok for apply? >>>> >>>> It obviously isn't ok to apply before a patch has gone in that will resolve >>>> all of the FAILs you specify. Comments on the patch: >>>> >>>> @@ -7281,9 +7284,28 @@ gimplify_expr (tree *expr_p, gimple_seq >>>> plain wrong if bitfields are involved. */ >>>> { >>>> tree type = TREE_TYPE (TREE_OPERAND (*expr_p, 1)); >>>> + tree org_type = TREE_TYPE (*expr_p); >>>> + >>>> + if (!useless_type_conversion_p (org_type, boolean_type_node)) >>>> + { >>>> + TREE_TYPE (*expr_p) = boolean_type_node; >>>> + *expr_p = fold_convert_loc (saved_location, org_type, *expr_p); >>>> + ret = GS_OK; >>>> + goto dont_recalculate; >>>> + } >>>> >>>> The above should be only done for !AGGREGATE_TYPE_P. Probably then >>>> the strange dont_recalcuate goto can go away as well. >>>> >>>> if (!AGGREGATE_TYPE_P (type)) >>>> - goto expr_2; >>>> + { >>>> + enum gimplify_status r0, r1; >>>> + >>>> + r0 = gimplify_expr (&TREE_OPERAND (*expr_p, 0), pre_p, >>>> + post_p, is_gimple_val, fb_rvalue); >>>> + r1 = gimplify_expr (&TREE_OPERAND (*expr_p, 1), pre_p, >>>> + post_p, is_gimple_val, fb_rvalue); >>>> + >>>> + ret = MIN (r0, r1); >>>> + } >>>> + >>>> >>>> why change this? >>>> >>>> @@ -7641,6 +7641,12 @@ fold_unary_loc (location_t loc, enum tre >>>> } >>>> else if (COMPARISON_CLASS_P (arg0)) >>>> { >>>> + /* Don't optimize type change, if op0 is of kind boolean_type_node. >>>> + Otherwise this will lead to race-condition on gimplification >>>> + trying to boolify comparison expression. */ >>>> + if (TREE_TYPE (op0) == boolean_type_node) >>>> + return NULL_TREE; >>>> + >>>> if (TREE_CODE (type) == BOOLEAN_TYPE) >>>> { >>>> arg0 = copy_node (arg0); >>>> >>>> The code leading here looks quite strange to me ... >>>> >>>> tree >>>> fold_unary_loc (location_t loc, enum tree_code code, tree type, tree op0) >>>> { >>>> ... >>>> if (TREE_CODE_CLASS (code) == tcc_unary) >>>> { >>>> ... >>>> else if (COMPARISON_CLASS_P (arg0)) >>>> { >>>> if (TREE_CODE (type) == BOOLEAN_TYPE) >>>> { >>>> arg0 = copy_node (arg0); >>>> TREE_TYPE (arg0) = type; >>>> return arg0; >>>> } >>>> else if (TREE_CODE (type) != INTEGER_TYPE) >>>> return fold_build3_loc (loc, COND_EXPR, type, arg0, >>>> fold_build1_loc (loc, code, type, >>>> integer_one_node), >>>> fold_build1_loc (loc, code, type, >>>> integer_zero_node)); >>>> } >>>> >>>> so, for any tcc_unary, like NEGATE_EXPR, with BOOLEAN_TYPE, >>>> return arg0 ... sure. Same for the 2nd case. ~ (a == b) isn't >>>> the same as a == b ? ~1 : ~0. I _suppose_ those cases were >>>> ment for CONVERT_EXPR_CODE_P (code) instead of all of tcc_unary, >>>> in which case they should be dropped or moved below where we >>>> handle conversions explicitly. >>>> >>>> That said - does anyone remember anything about that above code? >>>> Trying to do some svn blame history tracking now ... >>> >>> Oh, the patch continues... >>> >>> @@ -3208,7 +3208,10 @@ verify_gimple_comparison (tree type, tre >>> && (!POINTER_TYPE_P (op0_type) >>> || !POINTER_TYPE_P (op1_type) >>> || TYPE_MODE (op0_type) != TYPE_MODE (op1_type))) >>> - || !INTEGRAL_TYPE_P (type)) >>> + || !(TREE_CODE (type) == BOOLEAN_TYPE >>> + || (TREE_TYPE (type) && TREE_CODE (type) == INTEGER_TYPE >>> + && TREE_CODE (TREE_TYPE (type)) == BOOLEAN_TYPE) >>> + || (INTEGRAL_TYPE_P (type) && TYPE_PRECISION (type) == 1))) >>> { >>> >>> why that strange TREE_TYPE (TREE_TYPE ())) thing again? Drop >>> that. >>> >>> @@ -3352,6 +3355,8 @@ verify_gimple_assign_unary (gimple stmt) >>> case TRUTH_NOT_EXPR: >>> /* We require two-valued operand types. */ >>> if (!(TREE_CODE (rhs1_type) == BOOLEAN_TYPE >>> + || (TREE_TYPE (rhs1_type) && TREE_CODE (rhs1_type) == INTEGER_TYPE >>> + && TREE_CODE (TREE_TYPE (rhs1_type)) == BOOLEAN_TYPE) >>> || (INTEGRAL_TYPE_P (rhs1_type) >>> && TYPE_PRECISION (rhs1_type) == 1))) >>> { >>> >>> likewise. >> >> Well, those checks are necessary for Ada and its crippled >> boolean_type_node and computed boolean-based integer construct. Ada >> derives here the boolean-type to an integer with range 0..1 and the >> only way to find out that it is in fact such a beast is by looking >> into TREE_TYPE of type. See here Ada's code for getting base-type >> information. >> As such things are treated as compatible they can appear for TRUTH_NOT >> expressions and comparisons. > > No they can't as we boolify them. > > Richard. Well, they do. Test can prove this by running fortran's dg tests. The logic we do in gimplification is: 1) save original type of expression. 2) set expression's type to boolean_type_node. 3) if conversion from boolean_type_node to saved type isn't useless (and here we have the issue about Fortran's different booleans with different modes, which make those kinds not useless) then cast expression's result to original type and retry gimplification. 4) Otherwise gimplify operands and continue. Regards, Kai Kai
On Thu, May 26, 2011 at 1:20 PM, Kai Tietz <ktietz70@googlemail.com> wrote: > 2011/5/26 Richard Guenther <richard.guenther@gmail.com>: >> On Thu, May 26, 2011 at 1:11 PM, Kai Tietz <ktietz70@googlemail.com> wrote: >>> 2011/5/26 Richard Guenther <richard.guenther@gmail.com>: >>>> On Thu, May 26, 2011 at 12:46 PM, Richard Guenther >>>> <richard.guenther@gmail.com> wrote: >>>>> On Thu, May 26, 2011 at 12:20 PM, Kai Tietz <ktietz@redhat.com> wrote: >>>>>> Hello, >>>>>> >>>>>> this patch ensures that after gimplification also comparison expressions using FE's boolean_type_node. As we need to deal here with C/C++'s (obj-c/c++ and java's), Ada's, and Fortran's specific boolean types, this patch alters some checks in tree-cfg for Ada's sake, and we need to deal in fold-const about type-conversion of comparisons special. >>>>>> Additionally it takes care that in forwprop pass we don't do type hoising for boolean types. >>>>>> >>>>>> ChangeLog >>>>>> >>>>>> 2011-05-26 Kai Tietz >>>>>> >>>>>> * gimplify.c (gimple_boolify): Boolify all comparison >>>>>> expressions. >>>>>> (gimplify_expr): Use 'useless_type_conversion_p' for comparing >>>>>> org_type with boolean_type_node for TRUTH-expressions and comparisons. >>>>>> * fold-const.c (fold_unary_loc): Handle comparison conversions with >>>>>> boolean-type special. >>>>>> * tree-cfg.c (verify_gimple_comparison): Adjust check for boolean >>>>>> or compatible types. >>>>>> (verify_gimple_assign_unary): Likewise. >>>>>> * tree-ssa-forwprop.c (forward_propagate_comparison): Handle >>>>>> boolean case special. >>>>>> >>>>>> Tested on x86_64-pc-linux-gnu (multilib) with regression test for all standard languages (C, C++, Obj-C, Fortran, Java) plus Obj-C++ and Ada. Ok for apply? >>>>> >>>>> It obviously isn't ok to apply before a patch has gone in that will resolve >>>>> all of the FAILs you specify. Comments on the patch: >>>>> >>>>> @@ -7281,9 +7284,28 @@ gimplify_expr (tree *expr_p, gimple_seq >>>>> plain wrong if bitfields are involved. */ >>>>> { >>>>> tree type = TREE_TYPE (TREE_OPERAND (*expr_p, 1)); >>>>> + tree org_type = TREE_TYPE (*expr_p); >>>>> + >>>>> + if (!useless_type_conversion_p (org_type, boolean_type_node)) >>>>> + { >>>>> + TREE_TYPE (*expr_p) = boolean_type_node; >>>>> + *expr_p = fold_convert_loc (saved_location, org_type, *expr_p); >>>>> + ret = GS_OK; >>>>> + goto dont_recalculate; >>>>> + } >>>>> >>>>> The above should be only done for !AGGREGATE_TYPE_P. Probably then >>>>> the strange dont_recalcuate goto can go away as well. >>>>> >>>>> if (!AGGREGATE_TYPE_P (type)) >>>>> - goto expr_2; >>>>> + { >>>>> + enum gimplify_status r0, r1; >>>>> + >>>>> + r0 = gimplify_expr (&TREE_OPERAND (*expr_p, 0), pre_p, >>>>> + post_p, is_gimple_val, fb_rvalue); >>>>> + r1 = gimplify_expr (&TREE_OPERAND (*expr_p, 1), pre_p, >>>>> + post_p, is_gimple_val, fb_rvalue); >>>>> + >>>>> + ret = MIN (r0, r1); >>>>> + } >>>>> + >>>>> >>>>> why change this? >>>>> >>>>> @@ -7641,6 +7641,12 @@ fold_unary_loc (location_t loc, enum tre >>>>> } >>>>> else if (COMPARISON_CLASS_P (arg0)) >>>>> { >>>>> + /* Don't optimize type change, if op0 is of kind boolean_type_node. >>>>> + Otherwise this will lead to race-condition on gimplification >>>>> + trying to boolify comparison expression. */ >>>>> + if (TREE_TYPE (op0) == boolean_type_node) >>>>> + return NULL_TREE; >>>>> + >>>>> if (TREE_CODE (type) == BOOLEAN_TYPE) >>>>> { >>>>> arg0 = copy_node (arg0); >>>>> >>>>> The code leading here looks quite strange to me ... >>>>> >>>>> tree >>>>> fold_unary_loc (location_t loc, enum tree_code code, tree type, tree op0) >>>>> { >>>>> ... >>>>> if (TREE_CODE_CLASS (code) == tcc_unary) >>>>> { >>>>> ... >>>>> else if (COMPARISON_CLASS_P (arg0)) >>>>> { >>>>> if (TREE_CODE (type) == BOOLEAN_TYPE) >>>>> { >>>>> arg0 = copy_node (arg0); >>>>> TREE_TYPE (arg0) = type; >>>>> return arg0; >>>>> } >>>>> else if (TREE_CODE (type) != INTEGER_TYPE) >>>>> return fold_build3_loc (loc, COND_EXPR, type, arg0, >>>>> fold_build1_loc (loc, code, type, >>>>> integer_one_node), >>>>> fold_build1_loc (loc, code, type, >>>>> integer_zero_node)); >>>>> } >>>>> >>>>> so, for any tcc_unary, like NEGATE_EXPR, with BOOLEAN_TYPE, >>>>> return arg0 ... sure. Same for the 2nd case. ~ (a == b) isn't >>>>> the same as a == b ? ~1 : ~0. I _suppose_ those cases were >>>>> ment for CONVERT_EXPR_CODE_P (code) instead of all of tcc_unary, >>>>> in which case they should be dropped or moved below where we >>>>> handle conversions explicitly. >>>>> >>>>> That said - does anyone remember anything about that above code? >>>>> Trying to do some svn blame history tracking now ... >>>> >>>> Oh, the patch continues... >>>> >>>> @@ -3208,7 +3208,10 @@ verify_gimple_comparison (tree type, tre >>>> && (!POINTER_TYPE_P (op0_type) >>>> || !POINTER_TYPE_P (op1_type) >>>> || TYPE_MODE (op0_type) != TYPE_MODE (op1_type))) >>>> - || !INTEGRAL_TYPE_P (type)) >>>> + || !(TREE_CODE (type) == BOOLEAN_TYPE >>>> + || (TREE_TYPE (type) && TREE_CODE (type) == INTEGER_TYPE >>>> + && TREE_CODE (TREE_TYPE (type)) == BOOLEAN_TYPE) >>>> + || (INTEGRAL_TYPE_P (type) && TYPE_PRECISION (type) == 1))) >>>> { >>>> >>>> why that strange TREE_TYPE (TREE_TYPE ())) thing again? Drop >>>> that. >>>> >>>> @@ -3352,6 +3355,8 @@ verify_gimple_assign_unary (gimple stmt) >>>> case TRUTH_NOT_EXPR: >>>> /* We require two-valued operand types. */ >>>> if (!(TREE_CODE (rhs1_type) == BOOLEAN_TYPE >>>> + || (TREE_TYPE (rhs1_type) && TREE_CODE (rhs1_type) == INTEGER_TYPE >>>> + && TREE_CODE (TREE_TYPE (rhs1_type)) == BOOLEAN_TYPE) >>>> || (INTEGRAL_TYPE_P (rhs1_type) >>>> && TYPE_PRECISION (rhs1_type) == 1))) >>>> { >>>> >>>> likewise. >>> >>> Well, those checks are necessary for Ada and its crippled >>> boolean_type_node and computed boolean-based integer construct. Ada >>> derives here the boolean-type to an integer with range 0..1 and the >>> only way to find out that it is in fact such a beast is by looking >>> into TREE_TYPE of type. See here Ada's code for getting base-type >>> information. >>> As such things are treated as compatible they can appear for TRUTH_NOT >>> expressions and comparisons. >> >> No they can't as we boolify them. >> >> Richard. > > Well, they do. Test can prove this by running fortran's dg tests. > > The logic we do in gimplification is: > > 1) save original type of expression. > 2) set expression's type to boolean_type_node. > 3) if conversion from boolean_type_node to saved type isn't useless > (and here we have the issue about Fortran's different booleans with > different modes, which make those kinds not useless) then cast > expression's result to original type and retry gimplification. > 4) Otherwise gimplify operands and continue. You don't make sense. Btw, >>>> + if (!useless_type_conversion_p (org_type, boolean_type_node)) >>>> + { >>>> + TREE_TYPE (*expr_p) = boolean_type_node; >>>> + *expr_p = fold_convert_loc (saved_location, org_type, is bogus it should be TREE_TYPE (*expr_p) = boolean_type_node; if (!useless_type_conversion_p (org_type, boolean_type_node)) { *expr_p = fold_convert_loc (saved_location, org_type, ... > Regards, > Kai > > Kai >
2011/5/26 Richard Guenther <richard.guenther@gmail.com>: > On Thu, May 26, 2011 at 1:20 PM, Kai Tietz <ktietz70@googlemail.com> wrote: >> 2011/5/26 Richard Guenther <richard.guenther@gmail.com>: >>> On Thu, May 26, 2011 at 1:11 PM, Kai Tietz <ktietz70@googlemail.com> wrote: >>>> 2011/5/26 Richard Guenther <richard.guenther@gmail.com>: >>>>> On Thu, May 26, 2011 at 12:46 PM, Richard Guenther >>>>> <richard.guenther@gmail.com> wrote: >>>>>> On Thu, May 26, 2011 at 12:20 PM, Kai Tietz <ktietz@redhat.com> wrote: >>>>>>> Hello, >>>>>>> >>>>>>> this patch ensures that after gimplification also comparison expressions using FE's boolean_type_node. As we need to deal here with C/C++'s (obj-c/c++ and java's), Ada's, and Fortran's specific boolean types, this patch alters some checks in tree-cfg for Ada's sake, and we need to deal in fold-const about type-conversion of comparisons special. >>>>>>> Additionally it takes care that in forwprop pass we don't do type hoising for boolean types. >>>>>>> >>>>>>> ChangeLog >>>>>>> >>>>>>> 2011-05-26 Kai Tietz >>>>>>> >>>>>>> * gimplify.c (gimple_boolify): Boolify all comparison >>>>>>> expressions. >>>>>>> (gimplify_expr): Use 'useless_type_conversion_p' for comparing >>>>>>> org_type with boolean_type_node for TRUTH-expressions and comparisons. >>>>>>> * fold-const.c (fold_unary_loc): Handle comparison conversions with >>>>>>> boolean-type special. >>>>>>> * tree-cfg.c (verify_gimple_comparison): Adjust check for boolean >>>>>>> or compatible types. >>>>>>> (verify_gimple_assign_unary): Likewise. >>>>>>> * tree-ssa-forwprop.c (forward_propagate_comparison): Handle >>>>>>> boolean case special. >>>>>>> >>>>>>> Tested on x86_64-pc-linux-gnu (multilib) with regression test for all standard languages (C, C++, Obj-C, Fortran, Java) plus Obj-C++ and Ada. Ok for apply? >>>>>> >>>>>> It obviously isn't ok to apply before a patch has gone in that will resolve >>>>>> all of the FAILs you specify. Comments on the patch: >>>>>> >>>>>> @@ -7281,9 +7284,28 @@ gimplify_expr (tree *expr_p, gimple_seq >>>>>> plain wrong if bitfields are involved. */ >>>>>> { >>>>>> tree type = TREE_TYPE (TREE_OPERAND (*expr_p, 1)); >>>>>> + tree org_type = TREE_TYPE (*expr_p); >>>>>> + >>>>>> + if (!useless_type_conversion_p (org_type, boolean_type_node)) >>>>>> + { >>>>>> + TREE_TYPE (*expr_p) = boolean_type_node; >>>>>> + *expr_p = fold_convert_loc (saved_location, org_type, *expr_p); >>>>>> + ret = GS_OK; >>>>>> + goto dont_recalculate; >>>>>> + } >>>>>> >>>>>> The above should be only done for !AGGREGATE_TYPE_P. Probably then >>>>>> the strange dont_recalcuate goto can go away as well. >>>>>> >>>>>> if (!AGGREGATE_TYPE_P (type)) >>>>>> - goto expr_2; >>>>>> + { >>>>>> + enum gimplify_status r0, r1; >>>>>> + >>>>>> + r0 = gimplify_expr (&TREE_OPERAND (*expr_p, 0), pre_p, >>>>>> + post_p, is_gimple_val, fb_rvalue); >>>>>> + r1 = gimplify_expr (&TREE_OPERAND (*expr_p, 1), pre_p, >>>>>> + post_p, is_gimple_val, fb_rvalue); >>>>>> + >>>>>> + ret = MIN (r0, r1); >>>>>> + } >>>>>> + >>>>>> >>>>>> why change this? >>>>>> >>>>>> @@ -7641,6 +7641,12 @@ fold_unary_loc (location_t loc, enum tre >>>>>> } >>>>>> else if (COMPARISON_CLASS_P (arg0)) >>>>>> { >>>>>> + /* Don't optimize type change, if op0 is of kind boolean_type_node. >>>>>> + Otherwise this will lead to race-condition on gimplification >>>>>> + trying to boolify comparison expression. */ >>>>>> + if (TREE_TYPE (op0) == boolean_type_node) >>>>>> + return NULL_TREE; >>>>>> + >>>>>> if (TREE_CODE (type) == BOOLEAN_TYPE) >>>>>> { >>>>>> arg0 = copy_node (arg0); >>>>>> >>>>>> The code leading here looks quite strange to me ... >>>>>> >>>>>> tree >>>>>> fold_unary_loc (location_t loc, enum tree_code code, tree type, tree op0) >>>>>> { >>>>>> ... >>>>>> if (TREE_CODE_CLASS (code) == tcc_unary) >>>>>> { >>>>>> ... >>>>>> else if (COMPARISON_CLASS_P (arg0)) >>>>>> { >>>>>> if (TREE_CODE (type) == BOOLEAN_TYPE) >>>>>> { >>>>>> arg0 = copy_node (arg0); >>>>>> TREE_TYPE (arg0) = type; >>>>>> return arg0; >>>>>> } >>>>>> else if (TREE_CODE (type) != INTEGER_TYPE) >>>>>> return fold_build3_loc (loc, COND_EXPR, type, arg0, >>>>>> fold_build1_loc (loc, code, type, >>>>>> integer_one_node), >>>>>> fold_build1_loc (loc, code, type, >>>>>> integer_zero_node)); >>>>>> } >>>>>> >>>>>> so, for any tcc_unary, like NEGATE_EXPR, with BOOLEAN_TYPE, >>>>>> return arg0 ... sure. Same for the 2nd case. ~ (a == b) isn't >>>>>> the same as a == b ? ~1 : ~0. I _suppose_ those cases were >>>>>> ment for CONVERT_EXPR_CODE_P (code) instead of all of tcc_unary, >>>>>> in which case they should be dropped or moved below where we >>>>>> handle conversions explicitly. >>>>>> >>>>>> That said - does anyone remember anything about that above code? >>>>>> Trying to do some svn blame history tracking now ... >>>>> >>>>> Oh, the patch continues... >>>>> >>>>> @@ -3208,7 +3208,10 @@ verify_gimple_comparison (tree type, tre >>>>> && (!POINTER_TYPE_P (op0_type) >>>>> || !POINTER_TYPE_P (op1_type) >>>>> || TYPE_MODE (op0_type) != TYPE_MODE (op1_type))) >>>>> - || !INTEGRAL_TYPE_P (type)) >>>>> + || !(TREE_CODE (type) == BOOLEAN_TYPE >>>>> + || (TREE_TYPE (type) && TREE_CODE (type) == INTEGER_TYPE >>>>> + && TREE_CODE (TREE_TYPE (type)) == BOOLEAN_TYPE) >>>>> + || (INTEGRAL_TYPE_P (type) && TYPE_PRECISION (type) == 1))) >>>>> { >>>>> >>>>> why that strange TREE_TYPE (TREE_TYPE ())) thing again? Drop >>>>> that. >>>>> >>>>> @@ -3352,6 +3355,8 @@ verify_gimple_assign_unary (gimple stmt) >>>>> case TRUTH_NOT_EXPR: >>>>> /* We require two-valued operand types. */ >>>>> if (!(TREE_CODE (rhs1_type) == BOOLEAN_TYPE >>>>> + || (TREE_TYPE (rhs1_type) && TREE_CODE (rhs1_type) == INTEGER_TYPE >>>>> + && TREE_CODE (TREE_TYPE (rhs1_type)) == BOOLEAN_TYPE) >>>>> || (INTEGRAL_TYPE_P (rhs1_type) >>>>> && TYPE_PRECISION (rhs1_type) == 1))) >>>>> { >>>>> >>>>> likewise. >>>> >>>> Well, those checks are necessary for Ada and its crippled >>>> boolean_type_node and computed boolean-based integer construct. Ada >>>> derives here the boolean-type to an integer with range 0..1 and the >>>> only way to find out that it is in fact such a beast is by looking >>>> into TREE_TYPE of type. See here Ada's code for getting base-type >>>> information. >>>> As such things are treated as compatible they can appear for TRUTH_NOT >>>> expressions and comparisons. >>> >>> No they can't as we boolify them. >>> >>> Richard. >> >> Well, they do. Test can prove this by running fortran's dg tests. >> >> The logic we do in gimplification is: >> >> 1) save original type of expression. >> 2) set expression's type to boolean_type_node. >> 3) if conversion from boolean_type_node to saved type isn't useless >> (and here we have the issue about Fortran's different booleans with >> different modes, which make those kinds not useless) then cast >> expression's result to original type and retry gimplification. >> 4) Otherwise gimplify operands and continue. > > You don't make sense. Btw, > >>>>> + if (!useless_type_conversion_p (org_type, boolean_type_node)) >>>>> + { >>>>> + TREE_TYPE (*expr_p) = boolean_type_node; >>>>> + *expr_p = fold_convert_loc (saved_location, org_type, > > is bogus it should be > > TREE_TYPE (*expr_p) = boolean_type_node; > if (!useless_type_conversion_p (org_type, boolean_type_node)) > { > *expr_p = fold_convert_loc (saved_location, org_type, Well, I spared here the type conversion to boolean_type_node for useless conversion, but it doesn't change anything about the endless recursion here. As long as fold_convert_loc modifies *expr_p's type back to original type it will loop for ever for those cases I described above. Please don't forget that boolean_type_node is FE type here and not the later 1-bit BOOL_SIZE type set in free_lang_data. Kai
On Thu, May 26, 2011 at 1:28 PM, Kai Tietz <ktietz70@googlemail.com> wrote: > 2011/5/26 Richard Guenther <richard.guenther@gmail.com>: >> On Thu, May 26, 2011 at 1:20 PM, Kai Tietz <ktietz70@googlemail.com> wrote: >>> 2011/5/26 Richard Guenther <richard.guenther@gmail.com>: >>>> On Thu, May 26, 2011 at 1:11 PM, Kai Tietz <ktietz70@googlemail.com> wrote: >>>>> 2011/5/26 Richard Guenther <richard.guenther@gmail.com>: >>>>>> On Thu, May 26, 2011 at 12:46 PM, Richard Guenther >>>>>> <richard.guenther@gmail.com> wrote: >>>>>>> On Thu, May 26, 2011 at 12:20 PM, Kai Tietz <ktietz@redhat.com> wrote: >>>>>>>> Hello, >>>>>>>> >>>>>>>> this patch ensures that after gimplification also comparison expressions using FE's boolean_type_node. As we need to deal here with C/C++'s (obj-c/c++ and java's), Ada's, and Fortran's specific boolean types, this patch alters some checks in tree-cfg for Ada's sake, and we need to deal in fold-const about type-conversion of comparisons special. >>>>>>>> Additionally it takes care that in forwprop pass we don't do type hoising for boolean types. >>>>>>>> >>>>>>>> ChangeLog >>>>>>>> >>>>>>>> 2011-05-26 Kai Tietz >>>>>>>> >>>>>>>> * gimplify.c (gimple_boolify): Boolify all comparison >>>>>>>> expressions. >>>>>>>> (gimplify_expr): Use 'useless_type_conversion_p' for comparing >>>>>>>> org_type with boolean_type_node for TRUTH-expressions and comparisons. >>>>>>>> * fold-const.c (fold_unary_loc): Handle comparison conversions with >>>>>>>> boolean-type special. >>>>>>>> * tree-cfg.c (verify_gimple_comparison): Adjust check for boolean >>>>>>>> or compatible types. >>>>>>>> (verify_gimple_assign_unary): Likewise. >>>>>>>> * tree-ssa-forwprop.c (forward_propagate_comparison): Handle >>>>>>>> boolean case special. >>>>>>>> >>>>>>>> Tested on x86_64-pc-linux-gnu (multilib) with regression test for all standard languages (C, C++, Obj-C, Fortran, Java) plus Obj-C++ and Ada. Ok for apply? >>>>>>> >>>>>>> It obviously isn't ok to apply before a patch has gone in that will resolve >>>>>>> all of the FAILs you specify. Comments on the patch: >>>>>>> >>>>>>> @@ -7281,9 +7284,28 @@ gimplify_expr (tree *expr_p, gimple_seq >>>>>>> plain wrong if bitfields are involved. */ >>>>>>> { >>>>>>> tree type = TREE_TYPE (TREE_OPERAND (*expr_p, 1)); >>>>>>> + tree org_type = TREE_TYPE (*expr_p); >>>>>>> + >>>>>>> + if (!useless_type_conversion_p (org_type, boolean_type_node)) >>>>>>> + { >>>>>>> + TREE_TYPE (*expr_p) = boolean_type_node; >>>>>>> + *expr_p = fold_convert_loc (saved_location, org_type, *expr_p); >>>>>>> + ret = GS_OK; >>>>>>> + goto dont_recalculate; >>>>>>> + } >>>>>>> >>>>>>> The above should be only done for !AGGREGATE_TYPE_P. Probably then >>>>>>> the strange dont_recalcuate goto can go away as well. >>>>>>> >>>>>>> if (!AGGREGATE_TYPE_P (type)) >>>>>>> - goto expr_2; >>>>>>> + { >>>>>>> + enum gimplify_status r0, r1; >>>>>>> + >>>>>>> + r0 = gimplify_expr (&TREE_OPERAND (*expr_p, 0), pre_p, >>>>>>> + post_p, is_gimple_val, fb_rvalue); >>>>>>> + r1 = gimplify_expr (&TREE_OPERAND (*expr_p, 1), pre_p, >>>>>>> + post_p, is_gimple_val, fb_rvalue); >>>>>>> + >>>>>>> + ret = MIN (r0, r1); >>>>>>> + } >>>>>>> + >>>>>>> >>>>>>> why change this? >>>>>>> >>>>>>> @@ -7641,6 +7641,12 @@ fold_unary_loc (location_t loc, enum tre >>>>>>> } >>>>>>> else if (COMPARISON_CLASS_P (arg0)) >>>>>>> { >>>>>>> + /* Don't optimize type change, if op0 is of kind boolean_type_node. >>>>>>> + Otherwise this will lead to race-condition on gimplification >>>>>>> + trying to boolify comparison expression. */ >>>>>>> + if (TREE_TYPE (op0) == boolean_type_node) >>>>>>> + return NULL_TREE; >>>>>>> + >>>>>>> if (TREE_CODE (type) == BOOLEAN_TYPE) >>>>>>> { >>>>>>> arg0 = copy_node (arg0); >>>>>>> >>>>>>> The code leading here looks quite strange to me ... >>>>>>> >>>>>>> tree >>>>>>> fold_unary_loc (location_t loc, enum tree_code code, tree type, tree op0) >>>>>>> { >>>>>>> ... >>>>>>> if (TREE_CODE_CLASS (code) == tcc_unary) >>>>>>> { >>>>>>> ... >>>>>>> else if (COMPARISON_CLASS_P (arg0)) >>>>>>> { >>>>>>> if (TREE_CODE (type) == BOOLEAN_TYPE) >>>>>>> { >>>>>>> arg0 = copy_node (arg0); >>>>>>> TREE_TYPE (arg0) = type; >>>>>>> return arg0; >>>>>>> } >>>>>>> else if (TREE_CODE (type) != INTEGER_TYPE) >>>>>>> return fold_build3_loc (loc, COND_EXPR, type, arg0, >>>>>>> fold_build1_loc (loc, code, type, >>>>>>> integer_one_node), >>>>>>> fold_build1_loc (loc, code, type, >>>>>>> integer_zero_node)); >>>>>>> } >>>>>>> >>>>>>> so, for any tcc_unary, like NEGATE_EXPR, with BOOLEAN_TYPE, >>>>>>> return arg0 ... sure. Same for the 2nd case. ~ (a == b) isn't >>>>>>> the same as a == b ? ~1 : ~0. I _suppose_ those cases were >>>>>>> ment for CONVERT_EXPR_CODE_P (code) instead of all of tcc_unary, >>>>>>> in which case they should be dropped or moved below where we >>>>>>> handle conversions explicitly. >>>>>>> >>>>>>> That said - does anyone remember anything about that above code? >>>>>>> Trying to do some svn blame history tracking now ... >>>>>> >>>>>> Oh, the patch continues... >>>>>> >>>>>> @@ -3208,7 +3208,10 @@ verify_gimple_comparison (tree type, tre >>>>>> && (!POINTER_TYPE_P (op0_type) >>>>>> || !POINTER_TYPE_P (op1_type) >>>>>> || TYPE_MODE (op0_type) != TYPE_MODE (op1_type))) >>>>>> - || !INTEGRAL_TYPE_P (type)) >>>>>> + || !(TREE_CODE (type) == BOOLEAN_TYPE >>>>>> + || (TREE_TYPE (type) && TREE_CODE (type) == INTEGER_TYPE >>>>>> + && TREE_CODE (TREE_TYPE (type)) == BOOLEAN_TYPE) >>>>>> + || (INTEGRAL_TYPE_P (type) && TYPE_PRECISION (type) == 1))) >>>>>> { >>>>>> >>>>>> why that strange TREE_TYPE (TREE_TYPE ())) thing again? Drop >>>>>> that. >>>>>> >>>>>> @@ -3352,6 +3355,8 @@ verify_gimple_assign_unary (gimple stmt) >>>>>> case TRUTH_NOT_EXPR: >>>>>> /* We require two-valued operand types. */ >>>>>> if (!(TREE_CODE (rhs1_type) == BOOLEAN_TYPE >>>>>> + || (TREE_TYPE (rhs1_type) && TREE_CODE (rhs1_type) == INTEGER_TYPE >>>>>> + && TREE_CODE (TREE_TYPE (rhs1_type)) == BOOLEAN_TYPE) >>>>>> || (INTEGRAL_TYPE_P (rhs1_type) >>>>>> && TYPE_PRECISION (rhs1_type) == 1))) >>>>>> { >>>>>> >>>>>> likewise. >>>>> >>>>> Well, those checks are necessary for Ada and its crippled >>>>> boolean_type_node and computed boolean-based integer construct. Ada >>>>> derives here the boolean-type to an integer with range 0..1 and the >>>>> only way to find out that it is in fact such a beast is by looking >>>>> into TREE_TYPE of type. See here Ada's code for getting base-type >>>>> information. >>>>> As such things are treated as compatible they can appear for TRUTH_NOT >>>>> expressions and comparisons. >>>> >>>> No they can't as we boolify them. >>>> >>>> Richard. >>> >>> Well, they do. Test can prove this by running fortran's dg tests. >>> >>> The logic we do in gimplification is: >>> >>> 1) save original type of expression. >>> 2) set expression's type to boolean_type_node. >>> 3) if conversion from boolean_type_node to saved type isn't useless >>> (and here we have the issue about Fortran's different booleans with >>> different modes, which make those kinds not useless) then cast >>> expression's result to original type and retry gimplification. >>> 4) Otherwise gimplify operands and continue. >> >> You don't make sense. Btw, >> >>>>>> + if (!useless_type_conversion_p (org_type, boolean_type_node)) >>>>>> + { >>>>>> + TREE_TYPE (*expr_p) = boolean_type_node; >>>>>> + *expr_p = fold_convert_loc (saved_location, org_type, >> >> is bogus it should be >> >> TREE_TYPE (*expr_p) = boolean_type_node; >> if (!useless_type_conversion_p (org_type, boolean_type_node)) >> { >> *expr_p = fold_convert_loc (saved_location, org_type, > > Well, I spared here the type conversion to boolean_type_node for > useless conversion, useless_type_conversion_p isn't symmetric, so you can't do that. > but it doesn't change anything about the endless > recursion here. As long as fold_convert_loc modifies *expr_p's type > back to original type it will loop for ever for those cases I > described above. Indeed which is why we also touch fold-const.c! > Please don't forget that boolean_type_node is FE type here and not the > later 1-bit BOOL_SIZE type set in free_lang_data. I perfectly know that. Still no valid reason. Richard. > Kai >
2011/5/26 Richard Guenther <richard.guenther@gmail.com>: > On Thu, May 26, 2011 at 1:28 PM, Kai Tietz <ktietz70@googlemail.com> wrote: >> 2011/5/26 Richard Guenther <richard.guenther@gmail.com>: >>> On Thu, May 26, 2011 at 1:20 PM, Kai Tietz <ktietz70@googlemail.com> wrote: >>>> 2011/5/26 Richard Guenther <richard.guenther@gmail.com>: >>>>> On Thu, May 26, 2011 at 1:11 PM, Kai Tietz <ktietz70@googlemail.com> wrote: >>>>>> 2011/5/26 Richard Guenther <richard.guenther@gmail.com>: >>>>>>> On Thu, May 26, 2011 at 12:46 PM, Richard Guenther >>>>>>> <richard.guenther@gmail.com> wrote: >>>>>>>> On Thu, May 26, 2011 at 12:20 PM, Kai Tietz <ktietz@redhat.com> wrote: >>>>>>>>> Hello, >>>>>>>>> >>>>>>>>> this patch ensures that after gimplification also comparison expressions using FE's boolean_type_node. As we need to deal here with C/C++'s (obj-c/c++ and java's), Ada's, and Fortran's specific boolean types, this patch alters some checks in tree-cfg for Ada's sake, and we need to deal in fold-const about type-conversion of comparisons special. >>>>>>>>> Additionally it takes care that in forwprop pass we don't do type hoising for boolean types. >>>>>>>>> >>>>>>>>> ChangeLog >>>>>>>>> >>>>>>>>> 2011-05-26 Kai Tietz >>>>>>>>> >>>>>>>>> * gimplify.c (gimple_boolify): Boolify all comparison >>>>>>>>> expressions. >>>>>>>>> (gimplify_expr): Use 'useless_type_conversion_p' for comparing >>>>>>>>> org_type with boolean_type_node for TRUTH-expressions and comparisons. >>>>>>>>> * fold-const.c (fold_unary_loc): Handle comparison conversions with >>>>>>>>> boolean-type special. >>>>>>>>> * tree-cfg.c (verify_gimple_comparison): Adjust check for boolean >>>>>>>>> or compatible types. >>>>>>>>> (verify_gimple_assign_unary): Likewise. >>>>>>>>> * tree-ssa-forwprop.c (forward_propagate_comparison): Handle >>>>>>>>> boolean case special. >>>>>>>>> >>>>>>>>> Tested on x86_64-pc-linux-gnu (multilib) with regression test for all standard languages (C, C++, Obj-C, Fortran, Java) plus Obj-C++ and Ada. Ok for apply? >>>>>>>> >>>>>>>> It obviously isn't ok to apply before a patch has gone in that will resolve >>>>>>>> all of the FAILs you specify. Comments on the patch: >>>>>>>> >>>>>>>> @@ -7281,9 +7284,28 @@ gimplify_expr (tree *expr_p, gimple_seq >>>>>>>> plain wrong if bitfields are involved. */ >>>>>>>> { >>>>>>>> tree type = TREE_TYPE (TREE_OPERAND (*expr_p, 1)); >>>>>>>> + tree org_type = TREE_TYPE (*expr_p); >>>>>>>> + >>>>>>>> + if (!useless_type_conversion_p (org_type, boolean_type_node)) >>>>>>>> + { >>>>>>>> + TREE_TYPE (*expr_p) = boolean_type_node; >>>>>>>> + *expr_p = fold_convert_loc (saved_location, org_type, *expr_p); >>>>>>>> + ret = GS_OK; >>>>>>>> + goto dont_recalculate; >>>>>>>> + } >>>>>>>> >>>>>>>> The above should be only done for !AGGREGATE_TYPE_P. Probably then >>>>>>>> the strange dont_recalcuate goto can go away as well. >>>>>>>> >>>>>>>> if (!AGGREGATE_TYPE_P (type)) >>>>>>>> - goto expr_2; >>>>>>>> + { >>>>>>>> + enum gimplify_status r0, r1; >>>>>>>> + >>>>>>>> + r0 = gimplify_expr (&TREE_OPERAND (*expr_p, 0), pre_p, >>>>>>>> + post_p, is_gimple_val, fb_rvalue); >>>>>>>> + r1 = gimplify_expr (&TREE_OPERAND (*expr_p, 1), pre_p, >>>>>>>> + post_p, is_gimple_val, fb_rvalue); >>>>>>>> + >>>>>>>> + ret = MIN (r0, r1); >>>>>>>> + } >>>>>>>> + >>>>>>>> >>>>>>>> why change this? >>>>>>>> >>>>>>>> @@ -7641,6 +7641,12 @@ fold_unary_loc (location_t loc, enum tre >>>>>>>> } >>>>>>>> else if (COMPARISON_CLASS_P (arg0)) >>>>>>>> { >>>>>>>> + /* Don't optimize type change, if op0 is of kind boolean_type_node. >>>>>>>> + Otherwise this will lead to race-condition on gimplification >>>>>>>> + trying to boolify comparison expression. */ >>>>>>>> + if (TREE_TYPE (op0) == boolean_type_node) >>>>>>>> + return NULL_TREE; >>>>>>>> + >>>>>>>> if (TREE_CODE (type) == BOOLEAN_TYPE) >>>>>>>> { >>>>>>>> arg0 = copy_node (arg0); >>>>>>>> >>>>>>>> The code leading here looks quite strange to me ... >>>>>>>> >>>>>>>> tree >>>>>>>> fold_unary_loc (location_t loc, enum tree_code code, tree type, tree op0) >>>>>>>> { >>>>>>>> ... >>>>>>>> if (TREE_CODE_CLASS (code) == tcc_unary) >>>>>>>> { >>>>>>>> ... >>>>>>>> else if (COMPARISON_CLASS_P (arg0)) >>>>>>>> { >>>>>>>> if (TREE_CODE (type) == BOOLEAN_TYPE) >>>>>>>> { >>>>>>>> arg0 = copy_node (arg0); >>>>>>>> TREE_TYPE (arg0) = type; >>>>>>>> return arg0; >>>>>>>> } >>>>>>>> else if (TREE_CODE (type) != INTEGER_TYPE) >>>>>>>> return fold_build3_loc (loc, COND_EXPR, type, arg0, >>>>>>>> fold_build1_loc (loc, code, type, >>>>>>>> integer_one_node), >>>>>>>> fold_build1_loc (loc, code, type, >>>>>>>> integer_zero_node)); >>>>>>>> } >>>>>>>> >>>>>>>> so, for any tcc_unary, like NEGATE_EXPR, with BOOLEAN_TYPE, >>>>>>>> return arg0 ... sure. Same for the 2nd case. ~ (a == b) isn't >>>>>>>> the same as a == b ? ~1 : ~0. I _suppose_ those cases were >>>>>>>> ment for CONVERT_EXPR_CODE_P (code) instead of all of tcc_unary, >>>>>>>> in which case they should be dropped or moved below where we >>>>>>>> handle conversions explicitly. >>>>>>>> >>>>>>>> That said - does anyone remember anything about that above code? >>>>>>>> Trying to do some svn blame history tracking now ... >>>>>>> >>>>>>> Oh, the patch continues... >>>>>>> >>>>>>> @@ -3208,7 +3208,10 @@ verify_gimple_comparison (tree type, tre >>>>>>> && (!POINTER_TYPE_P (op0_type) >>>>>>> || !POINTER_TYPE_P (op1_type) >>>>>>> || TYPE_MODE (op0_type) != TYPE_MODE (op1_type))) >>>>>>> - || !INTEGRAL_TYPE_P (type)) >>>>>>> + || !(TREE_CODE (type) == BOOLEAN_TYPE >>>>>>> + || (TREE_TYPE (type) && TREE_CODE (type) == INTEGER_TYPE >>>>>>> + && TREE_CODE (TREE_TYPE (type)) == BOOLEAN_TYPE) >>>>>>> + || (INTEGRAL_TYPE_P (type) && TYPE_PRECISION (type) == 1))) >>>>>>> { >>>>>>> >>>>>>> why that strange TREE_TYPE (TREE_TYPE ())) thing again? Drop >>>>>>> that. >>>>>>> >>>>>>> @@ -3352,6 +3355,8 @@ verify_gimple_assign_unary (gimple stmt) >>>>>>> case TRUTH_NOT_EXPR: >>>>>>> /* We require two-valued operand types. */ >>>>>>> if (!(TREE_CODE (rhs1_type) == BOOLEAN_TYPE >>>>>>> + || (TREE_TYPE (rhs1_type) && TREE_CODE (rhs1_type) == INTEGER_TYPE >>>>>>> + && TREE_CODE (TREE_TYPE (rhs1_type)) == BOOLEAN_TYPE) >>>>>>> || (INTEGRAL_TYPE_P (rhs1_type) >>>>>>> && TYPE_PRECISION (rhs1_type) == 1))) >>>>>>> { >>>>>>> >>>>>>> likewise. >>>>>> >>>>>> Well, those checks are necessary for Ada and its crippled >>>>>> boolean_type_node and computed boolean-based integer construct. Ada >>>>>> derives here the boolean-type to an integer with range 0..1 and the >>>>>> only way to find out that it is in fact such a beast is by looking >>>>>> into TREE_TYPE of type. See here Ada's code for getting base-type >>>>>> information. >>>>>> As such things are treated as compatible they can appear for TRUTH_NOT >>>>>> expressions and comparisons. >>>>> >>>>> No they can't as we boolify them. >>>>> >>>>> Richard. >>>> >>>> Well, they do. Test can prove this by running fortran's dg tests. >>>> >>>> The logic we do in gimplification is: >>>> >>>> 1) save original type of expression. >>>> 2) set expression's type to boolean_type_node. >>>> 3) if conversion from boolean_type_node to saved type isn't useless >>>> (and here we have the issue about Fortran's different booleans with >>>> different modes, which make those kinds not useless) then cast >>>> expression's result to original type and retry gimplification. >>>> 4) Otherwise gimplify operands and continue. >>> >>> You don't make sense. Btw, >>> >>>>>>> + if (!useless_type_conversion_p (org_type, boolean_type_node)) >>>>>>> + { >>>>>>> + TREE_TYPE (*expr_p) = boolean_type_node; >>>>>>> + *expr_p = fold_convert_loc (saved_location, org_type, >>> >>> is bogus it should be >>> >>> TREE_TYPE (*expr_p) = boolean_type_node; >>> if (!useless_type_conversion_p (org_type, boolean_type_node)) >>> { >>> *expr_p = fold_convert_loc (saved_location, org_type, >> >> Well, I spared here the type conversion to boolean_type_node for >> useless conversion, > > useless_type_conversion_p isn't symmetric, so you can't do that. Right, I am aware of that. I will move in updated patch the setting boolean before the if here. As type conversion for the cast back to original-type via fold_convert_loc is only of interest AFAICS, when this cast isn't useless (means outter is org_type, and inner is boolean_type_node). But well, we can replace the if here to the check against org_type != boolean_type_node, if you prefer this. Kai
On Thu, May 26, 2011 at 07:02, Richard Guenther <richard.guenther@gmail.com> wrote: > 81764 dnovillo { > 81764 dnovillo if (TREE_CODE (type) == BOOLEAN_TYPE) > 81764 dnovillo { > 81764 dnovillo arg0 = copy_node (arg0); > 81764 dnovillo TREE_TYPE (arg0) = type; > 81764 dnovillo return arg0; > 81764 dnovillo } > > (famous tree-ssa merge revision). > > So - lets ask who still is around. Diego? ;) I just saw this in my inbox, sorry. What's the question? Whether I remember this code in fold? Not offhand. If it looks weird and/or is not working right, feel free to toss it. Diego.
Index: gcc.dg/tree-ssa/pr30978.c =================================================================== --- gcc.dg/tree-ssa/pr30978.c (revision 174264) +++ gcc.dg/tree-ssa/pr30978.c (working copy) @@ -10,5 +10,5 @@ return e; } -/* { dg-final { scan-tree-dump "e_. = a_..D. > 0;" "optimized" } } */ +/* { dg-final { scan-tree-dump " = a_..D. > 0;\n[^\n]*e_. = \\\(int\\\)" "optimized" } } */ /* { dg-final { cleanup-tree-dump "optimized" } } */ Index: gcc.dg/tree-ssa/builtin-expect-5.c =================================================================== --- gcc.dg/tree-ssa/builtin-expect-5.c (revision 174264) +++ gcc.dg/tree-ssa/builtin-expect-5.c (working copy) @@ -11,5 +11,5 @@ /* { dg-final { scan-tree-dump-times { if } 2 "forwprop1"} } */ /* { dg-final { scan-tree-dump {builtin_expect[^\n]*, 0\);\n[^\n]*if} "forwprop1"} } */ -/* { dg-final { scan-tree-dump {builtin_expect[^\n]*, 1\);\n[^\n]*if} "forwprop1"} } */ +/* { dg-final { scan-tree-dump {builtin_expect[^\n]*, 1\);} "forwprop1"} } */ /* { dg-final { cleanup-tree-dump "forwprop?" } } */ Index: gcc.dg/tree-ssa/pr21031.c =================================================================== --- gcc.dg/tree-ssa/pr21031.c (revision 174264) +++ gcc.dg/tree-ssa/pr21031.c (working copy) @@ -16,5 +16,5 @@ return 0; } -/* { dg-final { scan-tree-dump-times "Replaced" 2 "forwprop1"} } */ +/* { dg-final { scan-tree-dump-times "Replaced" 1 "forwprop1"} } */ /* { dg-final { cleanup-tree-dump "forwprop1" } } */ Index: gcc.dg/tree-ssa/ssa-fre-6.c =================================================================== --- gcc.dg/tree-ssa/ssa-fre-6.c (revision 174264) +++ gcc.dg/tree-ssa/ssa-fre-6.c (working copy) @@ -2,5 +2,5 @@ /* { dg-options "-O -fdump-tree-fre1-details" } */ int i; int foo(void) { i = 2; int j = i * 2; int k = i + 2; return j == k; } -/* { dg-final { scan-tree-dump-times "Replaced " 5 "fre1" } } */ +/* { dg-final { scan-tree-dump-times "Replaced " 6 "fre1" } } */ /* { dg-final { cleanup-tree-dump "fre1" } } */
Hello, this patch ensures that after gimplification also comparison expressions using FE's boolean_type_node. As we need to deal here with C/C++'s (obj-c/c++ and java's), Ada's, and Fortran's specific boolean types, this patch alters some checks in tree-cfg for Ada's sake, and we need to deal in fold-const about type-conversion of comparisons special. Additionally it takes care that in forwprop pass we don't do type hoising for boolean types. ChangeLog 2011-05-26 Kai Tietz * gimplify.c (gimple_boolify): Boolify all comparison expressions. (gimplify_expr): Use 'useless_type_conversion_p' for comparing org_type with boolean_type_node for TRUTH-expressions and comparisons. * fold-const.c (fold_unary_loc): Handle comparison conversions with boolean-type special. * tree-cfg.c (verify_gimple_comparison): Adjust check for boolean or compatible types. (verify_gimple_assign_unary): Likewise. * tree-ssa-forwprop.c (forward_propagate_comparison): Handle boolean case special. Tested on x86_64-pc-linux-gnu (multilib) with regression test for all standard languages (C, C++, Obj-C, Fortran, Java) plus Obj-C++ and Ada. Ok for apply? The following tests are failing due this change (what is to be expected here and needs some additional handling in forwprop). FAIL: gcc.dg/binop-xor1.c scan-tree-dump-times optimized "<bb[^>]*>" 5 FAIL: gcc.dg/binop-xor1.c scan-tree-dump-times optimized "\^" 1 FAIL: gcc.dg/binop-xor3.c scan-tree-dump-times optimized "<bb[^>]*>" 1 FAIL: gcc.dg/binop-xor3.c scan-tree-dump-times optimized "\^" 1 XPASS: gcc.dg/tree-ssa/20030807-7.c scan-tree-dump-times vrp1 "if " 1 FAIL: gcc.dg/tree-ssa/builtin-expect-5.c scan-tree-dump forwprop1 "builtin_expect[^\n]*, 1\);\n[^\n]*if" FAIL: gcc.dg/tree-ssa/pr21031.c scan-tree-dump-times forwprop1 "Replaced" 2 FAIL: gcc.dg/tree-ssa/pr30978.c scan-tree-dump optimized "e_. = a_..D. > 0;" FAIL: gcc.dg/tree-ssa/ssa-fre-6.c scan-tree-dump-times fre1 "Replaced " 5 FAIL: gcc.dg/tree-ssa/vrp47.c scan-tree-dump-times dom1 "x[^ ]* & y" 1 FAIL: gcc.dg/tree-ssa/vrp47.c scan-tree-dump-times vrp1 "x[^ ]* \^ 1" 1 FAIL: gcc.dg/vect/pr49038.c execution test FAIL: gcc.dg/vect/pr49038.c -flto execution test FAIL: gcc.target/i386/andor-2.c scan-assembler-not sete The Ada, Obj-C, Obj-C++, C++, Fortran, and Java testsuite don't show any new regressions by this. Some failing testcases are simply caused by different folding behavior and producing simplier code. The binop-xor tests 1 and 3 might be better removed for now, or marked as being expect to fail. The cause for their failing is in doing tree-analysis via fold-const on gimplified trees, which now don't allow folding here to look through. To illustrate required changes for other tests, I attach here some required changes for testsuite Regards, Kai Index: gcc/gcc/gimplify.c =================================================================== --- gcc.orig/gcc/gimplify.c 2011-05-23 13:49:29.235177000 +0200 +++ gcc/gcc/gimplify.c 2011-05-26 10:36:40.757641800 +0200 @@ -2837,15 +2837,18 @@ gimple_boolify (tree expr) case TRUTH_NOT_EXPR: TREE_OPERAND (expr, 0) = gimple_boolify (TREE_OPERAND (expr, 0)); - /* FALLTHRU */ - case EQ_EXPR: case NE_EXPR: - case LE_EXPR: case GE_EXPR: case LT_EXPR: case GT_EXPR: /* These expressions always produce boolean results. */ TREE_TYPE (expr) = boolean_type_node; return expr; default: + if (COMPARISON_CLASS_P (expr)) + { + /* These expressions always produce boolean results. */ + TREE_TYPE (expr) = boolean_type_node; + return expr; + } /* Other expressions that get here must have boolean values, but might need to be converted to the appropriate mode. */ if (type == boolean_type_node) @@ -6758,7 +6761,7 @@ gimplify_expr (tree *expr_p, gimple_seq tree org_type = TREE_TYPE (*expr_p); *expr_p = gimple_boolify (*expr_p); - if (org_type != boolean_type_node) + if (!useless_type_conversion_p (org_type, boolean_type_node)) { *expr_p = fold_convert (org_type, *expr_p); ret = GS_OK; @@ -7203,7 +7206,7 @@ gimplify_expr (tree *expr_p, gimple_seq fold_truth_not_expr) happily uses operand type and doesn't automatically uses boolean_type as result, we need to keep orignal type. */ - if (org_type != boolean_type_node) + if (!useless_type_conversion_p (org_type, boolean_type_node)) { *expr_p = fold_convert (org_type, *expr_p); ret = GS_OK; @@ -7281,9 +7284,28 @@ gimplify_expr (tree *expr_p, gimple_seq plain wrong if bitfields are involved. */ { tree type = TREE_TYPE (TREE_OPERAND (*expr_p, 1)); + tree org_type = TREE_TYPE (*expr_p); + + if (!useless_type_conversion_p (org_type, boolean_type_node)) + { + TREE_TYPE (*expr_p) = boolean_type_node; + *expr_p = fold_convert_loc (saved_location, org_type, *expr_p); + ret = GS_OK; + goto dont_recalculate; + } if (!AGGREGATE_TYPE_P (type)) - goto expr_2; + { + enum gimplify_status r0, r1; + + r0 = gimplify_expr (&TREE_OPERAND (*expr_p, 0), pre_p, + post_p, is_gimple_val, fb_rvalue); + r1 = gimplify_expr (&TREE_OPERAND (*expr_p, 1), pre_p, + post_p, is_gimple_val, fb_rvalue); + + ret = MIN (r0, r1); + } + else if (TYPE_MODE (type) != BLKmode) ret = gimplify_scalar_mode_aggregate_compare (expr_p); else Index: gcc/gcc/fold-const.c =================================================================== --- gcc.orig/gcc/fold-const.c 2011-05-23 13:49:29.000000000 +0200 +++ gcc/gcc/fold-const.c 2011-05-26 10:39:48.721510200 +0200 @@ -7641,6 +7641,12 @@ fold_unary_loc (location_t loc, enum tre } else if (COMPARISON_CLASS_P (arg0)) { + /* Don't optimize type change, if op0 is of kind boolean_type_node. + Otherwise this will lead to race-condition on gimplification + trying to boolify comparison expression. */ + if (TREE_TYPE (op0) == boolean_type_node) + return NULL_TREE; + if (TREE_CODE (type) == BOOLEAN_TYPE) { arg0 = copy_node (arg0); @@ -7672,11 +7678,12 @@ fold_unary_loc (location_t loc, enum tre if (TREE_TYPE (op0) == type) return op0; - /* If we have (type) (a CMP b) and type is an integral type, return + /* If we have (type) (a CMP b) and type is a boolean type, return new expression involving the new type. */ - if (COMPARISON_CLASS_P (op0) && INTEGRAL_TYPE_P (type)) - return fold_build2_loc (loc, TREE_CODE (op0), type, TREE_OPERAND (op0, 0), - TREE_OPERAND (op0, 1)); + if (COMPARISON_CLASS_P (op0) && TREE_CODE (type) == BOOLEAN_TYPE) + return fold_build2_loc (loc, TREE_CODE (op0), type, + TREE_OPERAND (op0, 0), + TREE_OPERAND (op0, 1)); /* Handle cases of two conversions in a row. */ if (CONVERT_EXPR_P (op0)) Index: gcc/gcc/tree-cfg.c =================================================================== --- gcc.orig/gcc/tree-cfg.c 2011-05-20 19:44:41.000000000 +0200 +++ gcc/gcc/tree-cfg.c 2011-05-25 13:17:22.743935300 +0200 @@ -3208,7 +3208,10 @@ verify_gimple_comparison (tree type, tre && (!POINTER_TYPE_P (op0_type) || !POINTER_TYPE_P (op1_type) || TYPE_MODE (op0_type) != TYPE_MODE (op1_type))) - || !INTEGRAL_TYPE_P (type)) + || !(TREE_CODE (type) == BOOLEAN_TYPE + || (TREE_TYPE (type) && TREE_CODE (type) == INTEGER_TYPE + && TREE_CODE (TREE_TYPE (type)) == BOOLEAN_TYPE) + || (INTEGRAL_TYPE_P (type) && TYPE_PRECISION (type) == 1))) { error ("type mismatch in comparison expression"); debug_generic_expr (type); @@ -3352,6 +3355,8 @@ verify_gimple_assign_unary (gimple stmt) case TRUTH_NOT_EXPR: /* We require two-valued operand types. */ if (!(TREE_CODE (rhs1_type) == BOOLEAN_TYPE + || (TREE_TYPE (rhs1_type) && TREE_CODE (rhs1_type) == INTEGER_TYPE + && TREE_CODE (TREE_TYPE (rhs1_type)) == BOOLEAN_TYPE) || (INTEGRAL_TYPE_P (rhs1_type) && TYPE_PRECISION (rhs1_type) == 1))) { Index: gcc/gcc/tree-ssa-forwprop.c =================================================================== --- gcc.orig/gcc/tree-ssa-forwprop.c 2011-05-23 13:49:29.000000000 +0200 +++ gcc/gcc/tree-ssa-forwprop.c 2011-05-23 13:55:35.281805900 +0200 @@ -1152,7 +1152,10 @@ forward_propagate_comparison (gimple stm tree lhs = gimple_assign_lhs (use_stmt); /* We can propagate the condition into a conversion. */ - if (CONVERT_EXPR_CODE_P (gimple_assign_rhs_code (use_stmt))) + if (CONVERT_EXPR_CODE_P (gimple_assign_rhs_code (use_stmt)) + && (TREE_CODE (TREE_TYPE (lhs)) == BOOLEAN_TYPE + || (INTEGRAL_TYPE_P (TREE_TYPE (lhs)) + && TYPE_PRECISION (TREE_TYPE (lhs)) == 1))) { /* Avoid using fold here as that may create a COND_EXPR with non-boolean condition as canonical form. */