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