On Thu, May 26, 2011 at 1:05 PM, Kai Tietz <ktiet...@googlemail.com> wrote: > 2011/5/26 Richard Guenther <richard.guent...@gmail.com>: >> On Thu, May 26, 2011 at 12:20 PM, Kai Tietz <kti...@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 >